feat(instrumentation-express)!: propagate context and measure full handler spans

[raphael-theriault-swi]

Which problem is this PR solving?

Currently the Express instrumentation does not propagate context and does not properly instrument request handlers.

Short description of the changes

Context is propagated for middleware and request handler spans, while their next callback is reset to the parent context to avoid extreme nesting as suggested in #2022. Request handler spans are also no longer ended prematurely.

Router spans are kept as-is since they are broken and propagating context for them would make things even more confusing than they already are.

Example traces

Before

After

Related issues

• Fixes @opentelemetry/instrumentation-express handler does not run in correct context #2022
• Fixes @opentelemetry/instrumentation-express handler does not account for async work #2023
• Fixes Auto instrumentation of express app is not sending exception event in trace #2146
• Fixes express instrumentation - span end prematurely for non-middleware instrumentations #2442
• Fixes Can't update span name when using OpenTelemetry Express Instrumentation for Node.js #2469
• Fixes Can't get active span in Express instrumentation. #2617
• Supersedes fix(instrumentation-express): Finish spans after execution #2603

[raphael-theriault-swi]

cc @pichlermarc fixes discussed wednesday

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.52%. Comparing base (b947650) to head (172695e).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2638   +/-   ##
=======================================
  Coverage   91.52%   91.52%           
=======================================
  Files         171      171           
  Lines        8150     8153    +3     
  Branches     1653     1653           
=======================================
+ Hits         7459     7462    +3     
  Misses        691      691           

Files with missing lines	Coverage Δ
...try-instrumentation-express/src/instrumentation.ts	98.61% <100.00%> (+0.02%)	⬆️

[pichlermarc]

this looks good, thank you for working on this. 🙌
cc @JamieDanielson and @pkanal (component owners) for comments :)

[mikesir87]

Apologies for the outsider coming in on this PR, but reporting that I've manually applied this PR into a local working copy to validate it works with my sample apps and it worked! In fact, it already helped me catch a case or two where I was missing an await call on a Kafka publish or DB update that was running after the request handler had returned. Excited to see this contribution! 😄

[faranahmadk]

Waiting for the merge... 🤞

[dyladan]

Can you expand on what you mean by not working properly or create an issue so this doesn't get lost?

[raphael-theriault-swi]

You can see in the nested routers test that the created router spans never include the /posts section of the route. If I were to propagate context and nest them we'd end up with /api/user/:id -> /:postId -> /api/user/posts/:postId which is really confusing.

[raphael-theriault-swi]

@JamieDanielson @pkanal Sorry, gonna bother you again for an approval on this one :)

[JamieDanielson]

Thank you for working on this! Should we mark this as a breaking change (still experimental but mostly for signaling) since it updates parent span ID for some spans?

[pichlermarc]

@JamieDanielson good point, I changed the title so that it's categorized as such. 🙂