feat(express): record exceptions

[raphael-theriault-swi]

Which problem is this PR solving?

• Record exceptions in middlewares

Short description of the changes

• Exceptions thrown by either synchronous middleware or passed to the callback in asynchronous ones are now recorded. Previously no exceptions were recorded by the instrumentation.

[codecov]

Codecov Report

Merging #1657 (55fbb98) into main (18ae75c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1657      +/-   ##
==========================================
+ Coverage   91.48%   91.49%   +0.01%     
==========================================
  Files         144      144              
  Lines        7374     7388      +14     
  Branches     1471     1474       +3     
==========================================
+ Hits         6746     6760      +14     
  Misses        628      628              

Files	Coverage Δ
...try-instrumentation-express/src/instrumentation.ts	100.00% <100.00%> (ø)
...opentelemetry-instrumentation-express/src/utils.ts	97.61% <100.00%> (+0.25%)	⬆️

[raphael-theriault-swi]

@JamieDanielson Possible to get a review on this ?

[JamieDanielson]

Thank you for the contribution @raphael-theriault-swi !

As you mention, these are definitely two different things and it might be best to try to separate them, but I'll talk through my initial thoughts here first. I tried these changes out using the express example app in this repo (looks like that example needs some updating, will try to get that on the list soon).

The exception is helpful for the error scenario. I'm thinking that is going to be useful and relatively non-controversial to get that in.

I'm a bit concerned about the change in the nesting of the Express spans - it might be exactly what is desired by most users but it might also be jarring to have such a big change to current behavior. I'm not totally sure and will try to solicit some feedback - as I also know some folks filter out middleware spans too.

For visuals, here is the express example with an exception thrown as shown in Zipkin - which clearly shows the exception and the nesting of the spans are somewhat minimal because the trace got cut off:

Here is the happy path in Zipkin with a larger trace, which shows all the nested spans. I guess that's a question to get answered - is this better for most folks? I think the timelines may show better, but the name cascade and nesting may be a little intense.

[jessitron]

Love the showing of exceptions on middleware.

I do not like the nesting, that's a conceptual change that doesn't fit. It will surprise everyone.
I don't prefer it because it makes it harder to focus on my handler. The sibling spans aren't as intrusive, I don't want to expand them to see my handler. I know that the nesting expresses the execution flow, but I don't feel like it expresses causality.

Agree with Jamie: if this is broken out, then we could discuss the nesting (thorny) separately from the exceptions (obviously better).

[pkanal]

I also want to +1 the showing exceptions in middleware, that's a great addition and I would love to see it as a separate PR 🙂

For the nesting change, as one of the most heavily used instrumentations, this would be a surprise to a lot of folks so at the most to start I would only want to see this as an opt-in scenario.

[raphael-theriault-swi]

Good point ! How do you feel about keeping the exception recording and also proper middleware execution times in and splitting off the nesting changes to another PR for discussion ? I assume the spans having meaningful durations should also be fairly uncontroversial, and it'll make splitting things much easier.

[JamieDanielson]

Good point ! How do you feel about keeping the exception recording and also proper middleware execution times in and splitting off the nesting changes to another PR for discussion ? I assume the spans having meaningful durations should also be fairly uncontroversial, and it'll make splitting things much easier.

Hmm... while I recognize that it's a bit more work, I think it is preferred to limit PRs to changing a single piece of functionality where possible. A big reason for that is if for some reason there is an unexpected side effect or issue, it can be harder to narrow down the piece of code responsible. So perhaps this ends up being 3 PRs then, and/or 2 PRs and an issue:

• PR for Feature: Record exceptions in middleware (your first bullet point)
• PR for Fix: Middleware spans have more accurate execution times (your third bullet point)
• Issue for Discussion: Respect middleware hierarchy when creating spans (your second bullet point)

[raphael-theriault-swi]

Force pushed a new version that only records exceptions. Will open the PR for span durations once main has these changes to avoid dealing with conflicts.

[raphael-theriault-swi]

@JamieDanielson @pkanal A revisit would be nice when you have some time :)

[cheempz]

Hi @Flarna @JamieDanielson @jessitron @pkanal any chance we can get this re-reviewed? It's important for our adoption of OTel :)

[raphael-theriault-swi]

Should be ready to merge

[JamieDanielson]

This is great, thanks so much for the code and the tests and the patience @raphael-theriault-swi ! Do you mind updating the PR description to show just this change, as you did with the title?

Thanks again 💥

[raphael-theriault-swi]

Done !