fix(plugin-express): fix double span end #908

[vmarchaud]

The "fix" is quite simple but i've added a big comments to explain what's happening inside express.
I've stated that it's would be quite memory consuming to listen for end event because we would keep the context alive for each middleware while waiting for the response to be sent. In this case we are only keep it for one tick which should be fine in terms of impact.
What do you guys think ?

[codecov-io]

Codecov Report

Merging #910 into master will not change coverage.
The diff coverage is 95.00%.

@@           Coverage Diff           @@
##           master     #910   +/-   ##
=======================================
  Coverage   94.75%   94.75%           
=======================================
  Files         248      248           
  Lines       11288    11288           
  Branches     1079     1081    +2     
=======================================
  Hits        10696    10696           
  Misses        592      592           

Impacted Files	Coverage Δ
...ckages/opentelemetry-plugin-express/src/express.ts	97.89% <94.11%> (-0.96%)	⬇️
packages/opentelemetry-plugin-express/src/utils.ts	97.22% <100.00%> (+1.76%)	⬆️
...ckages/opentelemetry-plugin-express/src/version.ts	100.00% <100.00%> (ø)
.../opentelemetry-plugin-express/test/express.test.ts	99.18% <100.00%> (ø)

[mayurkale22]

I don't have a super strong opinion on this fix, going to leave it to @open-telemetry/javascript-approvers to chime in.

[markwolff]

Does express expose a different async behavior than the standard http module? AFAIK the span should be ended before the followup callback is called anyways. Or callbackHasBeenCalled = true be changed to call a function ending the span?

[vmarchaud]

@markwolff

Does express expose a different async behavior than the standard http module

Yes, because the express is implemented as a stack of function, each of them requiring to call the next with the next callback. But it can also end the whole request using the standard http module.

AFAIK the span should be ended before the followup callback is called anyways

The issue is when the user code doesnt callback when it send the http response.

[vmarchaud]

@open-telemetry/javascript-approvers This one require some reviews please :)

[dyladan]

Only for asynchronous spans

[vmarchaud]

I don't why but at some point i though that it wasn't possible for sync callback too so i'd removed the acurate reporting for all spans, but you are right. I've repushed the previous implementation that report accurate duration for sync middlewares

[obecny]

lgtm