feat(express): Skip update HTTP's span name and update RpcMetadata's route instead

[chigia001]

Which problem is this PR solving?

Each framework has a different approach to updating Span's name and HTTP_ROUTE attribute. We want to delegate this to instrumentation-http for consistent's name and attribute
This PR fixes the above issue for express

• This will partially fix issue Setting HTTP_ROUTE inconsistent accross HTTP server frameworks #1381

Short description of the changes

• Skip modifying HTTP's Span name directly in instrumentation-connect code
  • This will be also a breaking change as @opentelemetry/instrumentation-express's spanNameHook. This config previously support overide HTTP route span's name, which is allow in spec => this might be something we can introduce back to @opentelemetry/instrumentation-http
  • Due to instrumentation-express alway working in conjunction with instrumentation-http, althrough instrumentation-express can update HTTP span's name, instrumentation-http will overide this name later when rpcMetadata.route is set. So there should be no visible change to end-user.
• Update route property of RpcMetadata
• Refactor some test due to these tests are not properly clean up (server.close() is not called) when the test is failing => this cause running test with npm run test -- --scope @opentelemetry/instrumentation-express seem stuck without any indicator

[chigia001]

PS: I also encounter issue with npm run test -- --scope @opentelemetry/instrumentation-express when initate with npm install.

The package include a conflict eslint ([email protected]) version that incompatiple with eslint-plugin-plugin in root dir. The dependency chain of that version:

[email protected] extraneous
node_modules/standard/node_modules/eslint
  eslint@"~6.8.0" from [email protected]
  node_modules/standard
    standard@"^14.3.4" from [email protected]
    node_modules/parse-env-string
      parse-env-string@"^1.0.0" from [email protected]
      node_modules/test-all-versions

need to hoist those dependency for the installation to work.

[codecov]

Codecov Report

Merging #1557 (67d8632) into main (a18b074) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1557      +/-   ##
==========================================
- Coverage   96.06%   95.95%   -0.11%     
==========================================
  Files          14       21       +7     
  Lines         914     1186     +272     
  Branches      199      255      +56     
==========================================
+ Hits          878     1138     +260     
- Misses         36       48      +12     

Impacted Files	Coverage Δ
...try-instrumentation-express/src/instrumentation.ts	100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

[JamieDanielson]

Thank you for this, it looks great!

I think the only concern I have on this is related to the breaking change for using the spanNameHook to override the HTTP span name. I'm not sure how common it is to use or if we need a deprecation period. We can still override Express span names with the hook, which I would think is the true purpose of this hook anyway but I know folks like to have options to rename auto-generated span names 🤔 .

This will be also a breaking change as @opentelemetry/instrumentation-express's spanNameHook. This config previously support overide HTTP route span's name, which is allow in spec => this might be something we can introduce back to @opentelemetry/instrumentation-http

[chigia001]

Thank you for this, it looks great!

I think the only concern I have on this is related to the breaking change for using the spanNameHook to override the HTTP span name. I'm not sure how common it is to use or if we need a deprecation period. We can still override Express span names with the hook, which I would think is the true purpose of this hook anyway but I know folks like to have options to rename auto-generated span names 🤔 .

This will be also a breaking change as @opentelemetry/instrumentation-express's spanNameHook. This config previously support overide HTTP route span's name, which is allow in spec => this might be something we can introduce back to @opentelemetry/instrumentation-http

Due to how instrumentation-http is working right now, once rpcMetadata.route is set, instrumentation-http will update the HTTP span name anyway, so the custom name hook for HTTP span in instrumentation-express don't have any effect. So it should not have any visibile effect to the end-user.
span attribute update from rpcMetadata.route
span name update form span attributes

[Flarna]

The applyCustomAttributesOnSpan hooks in http instrumentation is called after the span name update based on http.route and it gets the span as argument.

So one could actually rename the span as of now. Missing piece for real world is likely that this hook doesn't know the http.route and span has no API to read attributes nor name.

In general I think the current solution that express instrumenation is not touching the parent span is the way to go. Having the rpc data as a path to transfer data in a define way is fine.
But an instrumentation should not tinker with foreign spans. If several instrumentations start doing this we may end up in race who offers the "latest" hook.

If we need span name customization for http spans it's should be offered by http instrumentation.

[JamieDanielson]

If we need span name customization for http spans it's should be offered by http instrumentation.

Agreed 💯 .

Thank you both for clarification! This looks good to me.

[chigia001]

Don't know wy this PR fail unit test, let me investigate a little bit before merge.

[chigia001]

https://github.com/open-telemetry/opentelemetry-js-contrib/actions/runs/5504980559/jobs/10031782721
I think the issue is with cache. Right now we cache both node_modules + package-lock.json but we don't include these files/folder in repo, Once it generate an mess up cache file it very hard to re-produce those file in local to verify.

Not sure what is the next step we should do for this PR to be merged?