[instrumentation-express] `http.route == "/"` if every path matches

[JohannesHuster]

What version of OpenTelemetry are you using?

@opentelemetry/instrumentation-express: 0.35.0 (The issue is about this dependency)
@opentelemetry/instrumentation-http: 0.48.0
@opentelemetry/sdk-node: 0.48.0
@opentelemetry/sdk-trace-node: 1.21.0

What version of Node are you using?

20.11.0

What did you do?

app.use((req, res) => res.send('Hello')); // Or a more relevant example: app.use(cors());

Send any request to the server (or an OPTIONS request for the example in the comment).

Here is a repo with more and other (see Additional context) examples, with spans being printed to console: otel-express-routes

What did you expect to see?

For the root span something like:

{
  "attributes": {
    "http.route": "*" // or "/*"
  },
  "name": "OPTIONS *", // or "OPTIONS /*"
}

What did you see instead?

{
  "attributes": {
    "http.route": "/"
  },
  "name": "OPTIONS /",
}

Since name depends on http.route, this is really only about the latter.

Additional context

• The same behavior (http.route == "/") can be observed for root spans
  • when using "/*" as path with app.use or app.<METHOD>
  • and for default responses (404, 500).
• Middleware and router spans also include http.route == "/", when matching every path; e.g. app.use(router).
• Similarly, if a router is being added like app.use('/abc', router1); this leads to http.route == "/abc" for corresponding router spans, but "/abc" and "abc/*" routes are being matched. "/abcd" or similar is not matched.

What to do about it?

Looking at HTTP Server semantic conventions and Express 4 routing (plus testing a lot of paths), I think the following could be good alternatives for the cases mentioned above, because they match actual Express routing behavior, as expressed in Express 4 syntax.

• "*" or "/*" when every path is matched.
• "/some/prefix(/*)?" when "/some/prefix" and every path starting with "/some/prefix/" matches.

I don't have any prior experience with OTel semantic conventions, though. What do you think?

I have these changes (using "*" and "(/*)?") running locally and would be happy to provide a PR. I would just have to try and break it a bit more and have a look at extending the tests to cover these cases.

My current changes would also remove double slashes (#1547), but this option would work just as well well for that, I think.

[JohannesHuster]

I might have put a bit too much responsibility on http.route for non-request spans and described my current understanding in a related issue: #1993

[mydea]

We noticed that this is pretty weird right now, as a 404 route will have a http.route: "/" attribute now, which is definitely not correct. IMHO if no route matches, no http.route attribute should be set?

[JohannesHuster]

@mydea If a server answers a request (with any status code), some route must have matched. For a response with status code 404 the match is often with some kind of catch all route as the last route to check. Although you might just as well get it for a matched route like "/articles/:id".

But I agree that http.route: "/" is not correct to describe a catch all route and it's confusing to get that.