Express instrumentation does not normalize paths (removing double slashes)

[rrva]

What version of OpenTelemetry are you using?

auto-instrumentations-node 0.37.1
instrumentation-express 0.32.4

What version of Node are you using?

18

What did you do?

define some routes like this:

server.use(`/foo/`,
router.use('/v5/',
router.use(
    '/bar/',
router.get('/refresh',

What did you expect to see?

http.route attribute in traces should contain:

/foo/v5/bar/refresh

Instrumentation should normalise the path in the same way that Express does (Express knows that declaring /v5/ inside /foo/ means /foo/v5/ not /foo//v5/ )

What did you see instead?

/foo//v5//bar//refresh

Additional context

express 4.18.2

[unflxw]

Thanks for reporting this @rrva! Adding a link to the CNCF Slack conversation for additional context: https://cloud-native.slack.com/archives/C01NL1GRPQR/p1687429185406239

[pichlermarc]

@pkanal @JamieDanielson fyi

[pkanal]

I took a closer look at this and I think http.route actually gets set in the HTTP instrumentation. The value for route is set by rpcMetadata which comes from the trace package. So I'm wondering whether the normalization would need to happen either in the trace package or the HTTP instrumentation package?

[pichlermarc]

Hmm, I think rpcMetadata.route should be pre-formatted in the server framework's instrumentation (somewhere before this line in the express instrumentation), as the http-instrumentation can't know how the server framework usually formats the route string. 🤔

The semconv spec mentions that we should use

The matched route (path template in the format used by the respective server framework).

While unlikely, if // as a substring of route is intended in some other server framwork for some reason, we'd modify the intended value of http.route in the http instrumentation if we were to normalize there.

EDIT: looks like I forgot to link the line in the express instrumentation

[unflxw]

Like @pichlermarc said, the value of http.route cannot come from the HTTP instrumentation, as the HTTP instrumentation cannot understand Express routing by itself. But it is set in the root span, which is created by the HTTP instrumentation, through this RPC metadata mechanism, as mentioned above by @pkanal.

Here's where this metadata is set in the Express instrumentation, with the route being obtained by joining the values in the _LAYERS_STORE_PROPERTY in the Express request (req) object. I think (though I am not sure) that this naïve join is the problem, joining ["/foo/", "/bar/"] into "/foo//bar/".

[Flarna]

rpcMetadata is a communication channel between HTTP instrumentation and server frameworks like express to allow to put the route onto the root span without a requirement that one instrumentation knows of the other and without the requirement that e.g. express span is a direct child of http span.

1. The HTTP instrumentation creates a span and a rpcMetadata object. the rpcMetadata object is set on context
2. server framework picks rpcMetadata up from context and modifies it by setting the ``rpcMetadata.route`
3. at end of http span the http instrumentation checks the rpcMetadata object and adapts span name + sets http.route attribute

So it's the server framework instrumentation which detects the route and it is also responsible to format it correct.

express and others besides connect are implemented more complex then needed for historic reasons. #1534 shows how it should look like.

[JamieDanielson]

Thanks for the clarification all! I was initially thinking something simple like this regex replace but wondering if it's still too naive...

const route = (req[_LAYERS_STORE_PROPERTY] as string[])
          .filter(path => path !== '/' && path !== '/*')
          .join('')
          // remove duplicate slashes to normalize route
          .replace(/\/{2,}/g, '/');

There is something similar happening for connect in #1555 that has a lot more logic compared to this simple regex replace 🤔 . I'll have to look at that a bit more to see if it's closer to what we need here. Also I see #1557 was created for express that is similar to that connect PR #1534 so these may end up being somewhat tied together in implementation.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.