Customizing the trace name

[Fenntasy]

Hi there,

I'm opening this issue to discuss a work I've begun to work on my Remix app.

Here's my starting point, your instrumentation is a great work but it's annoying to have all traces be named like GET *.
After investigating this is due to the underlying Express implementation since the * is the Request.Route and since Remix is working with an app.all("*", createRequestHandler), it's logical that it uses it.

So I've now a work in progress version where for this kind of file structure:

routes
├── __layout
│   └── index.tsx
│   └── another.route.tsx
├── __layout.tsx
├── dashboard
│   ├── index.tsx
│   └── organizations.$name.tsx
├── dashboard.tsx
└── settings.tsx

I can have traces name like:

GET /
GET /another/route
GET /dashboard
GET /dashboard/organizations/$name
GET /settings

I would prefer, for my taste to also be able to have

GET /dashboard/organizations/:name

because I'm using express for other parts of my app so keeping a consistency with Express would be cool.

This could be done with options like so:

new RemixInstrumentation({
  displayExpressRoutes: true
})

Would you be open to a PR with all that?
Should the whole renaming of the trace be optional?

Thanks for your work in any case it's been a great help!

[Fenntasy]

After a night, it also could be possible to just add hooks to the different calls

[justindsmith]

Hey @Fenntasy! Glad this package has been helpful so far.

This package doesn’t actually instrument the express routes, it only emits remix.request, LOADER $route, and ACTION $route spans. Not the GET $route spans you’re mentioning. You might have to check the express plugin package if those the spans you’re trying to change.

Let me know if that helps. If you ARE trying to change any of the span names from this package I’m happy to discuss possible ways to configure that.

[Fenntasy]

I understand it doesn't intrument the express routes, my goal is to avoid having traces with GET * for every Remix routes since it's not really helping.

Here's what I've done at the moment for my needs (look for START CHANGES):

import { getRPCMetadata } from "@opentelemetry/core";
import { matchServerRoutes } from "@remix-run/server-runtime/dist/routeMatching";
import { createRoutes } from "@remix-run/server-runtime/dist/routes";

//...

private _patchCreateRequestHandler(): (
    original: typeof remixRunServerRuntime.createRequestHandler,
  ) => any {
    const tracer = this.tracer;
    return function createRequestHandler(original) {
      return function patchCreateRequestHandler(
        this: any,
      ): remixRunServerRuntime.RequestHandler {
        const [{ routes }] = arguments as unknown as Parameters<
          typeof remixRunServerRuntime.createRequestHandler
        >;
        const originalRequestHandler: remixRunServerRuntime.RequestHandler =
          original.apply(this, arguments as any);

        return (
          request: Request,
          loadContext?: remixRunServerRuntime.AppLoadContext,
        ) => {
          // START CHANGES
          const url = new URL(request.url);
          const matches = matchServerRoutes(createRoutes(routes), url.pathname);
          if (matches && matches.length > 0) {
            const route = matches[matches.length - 1].route;
            const rpcMetadata = getRPCMetadata(opentelemetry.context.active());
            if (rpcMetadata) {
              rpcMetadata.span.updateName(
                `${request.method} ${displayRoute(route.id)}`,
              );
              rpcMetadata.route = displayRoute(route.id);
            }
          }
          // END CHANGES

          const span = tracer.startSpan(
            `remix.request`,
            {
              attributes: {
                [SemanticAttributes.CODE_FUNCTION]: "requestHandler",
              },
            },
            opentelemetry.context.active(),
          );
          addRequestAttributesToSpan(span, request);

          const originalResponsePromise = opentelemetry.context.with(
            opentelemetry.trace.setSpan(opentelemetry.context.active(), span),
            () => originalRequestHandler(request, loadContext),
          );
          return originalResponsePromise
            .then((response) => {
              addResponseAttributesToSpan(span, response);
              return response;
            })
            .catch((error) => {
              addErrorAttributesToSpan(span, error);
              throw error;
            })
            .finally(() => {
              span.end();
            });
        };
      };
    };
  }

with displayRoutes being

const displayRoute = (route: string): string => {
  if (route === "root") {
    return "/";
  }
  return route
    .replace("routes/", "/")
    .replace(/\/__[^\/]*\/?/, "/")
    .replace(".", "/")
    .replace("$", ":");
};

The meat is it is what's also done by the Express intrumentation:

const rpcMetadata = getRPCMetadata(opentelemetry.context.active());
if (rpcMetadata) {
  rpcMetadata.span.updateName(
    `${request.method} ${displayRoute(route.id)}`,
  );
  rpcMetadata.route = displayRoute(route.id);
}

rpcMetadata.span.updateName make the traces be displayed with the route instead of *
rpcMetadata.route is what's used by the HTTP intrumentation for setting the http.route attribute in the main trace.

displayRoute is just there to display the routes as express would but it's not necessary, just that we use express on our side so it's more convient.

[justindsmith]

Ah, okay, so you want to update the parent active span name from the express instrumentation plugin based on information from remix.

I'm not sure updating another instrumentation's context is advised, as it can rewrite history a bit.. but perhaps there are use cases where it is helpful for organizing your data how you need.

In this case, your code to get / modify the active span is good, but thinking through how to configure this there are a few options:

1. Use some frameworks-specific updateParentSpanNameForExpress boolean flag, which if set performs all the logic you have. This makes your use cases very simple to enable, but then means we have to add new flags for all upstream frameworks that might be used (koa, webworkers, etc)
2. Have a updateParentSpanName configuration option that accepts a function which returns a string representing the new parent span name. That function would be passed the request and route information and can format however the client chooses. This is a bit more work for the user, who has to know how to write the function, but easy to document in readme files. It's a more flexible option than (1) since it can support any upstream framework formats (or custom formats for whatever reason).
3. Have an updateParentSpan configuration option that accepts a function which returns nothing. That function would be passed the parent span, the request, and the route information and the user can use span.updateName in the function to modify. This is more cognitive work for the user to know how to update the name property, but is much more flexible than option (2) because now you could even add attributes to the parent span context.
4. Introduce a pre (and post?) lifecycle hook that runs the function before processing. This function would receive the request and route, but wouldn't receive the active span. The user would need to use the otel apis to get active span if they want. Much more cognitive load, but much more flexible.

I'm not keen on (1) since I don't like baking in other framework knowledge into the code. I'm open to (2) or (3) as a way to make it framework-agnostic and more flexible, without adding too much work on the client side. Option (4) is probably too much complexity back to the user for right now. Leaning a bit more towards (3) since it's just a line more for the client, but opens up a lot more flexibility down the road if people need.

Only part I'm not sure of is if instead of the span you have to pass the rpcMetadata because it looks like you're updating multiple properties on that object.

What do you think?