fix(asynchooks-scope): fix context loss using .with() #1101

[vmarchaud]

This one was kinda hard to debug and fix but it finally works !
I had to remove different test cases that were working in the past:
- cases where we expected that .bind of a function will propagate the context across async operations, which is essentially leaking because .bind is only synchronous.
- cases where we wanted to use withAsync inside of a with which doesn't work because as said above, with is only sync.

Started this refactor for #1013 but considering the bug in #1101 (that this refactor fixes) as discussed at the SIG, we should merge that first and then try to fix withAsync

Lot of edit for this one but its worth, i found a way to fix the initial issue #752 without using await so i removed withAsync which should fixes #1013 and it works with #1101. Finally !

closes #1101 #752 #1013

[codecov]

Codecov Report

Merging #1099 into master will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1099      +/-   ##
==========================================
- Coverage   92.36%   92.32%   -0.05%     
==========================================
  Files         115      115              
  Lines        3407     3389      -18     
  Branches      686      683       -3     
==========================================
- Hits         3147     3129      -18     
  Misses        260      260              

Impacted Files	Coverage Δ
...ontext-async-hooks/src/AsyncHooksContextManager.ts	96.87% <100.00%> (-0.50%)	⬇️

[vmarchaud]

Good news, i found a way to make with() works when using async/await (checkout this test), plus the code is way simpler than before !
Ready for review @open-telemetry/javascript-approvers :)

[dyladan]

This is great work thanks so much for this

[dyladan]

@open-telemetry/javascript-approvers this is very high priority. I want to get this merged and released as a patch release which means not merging any breaking changes until this is in. @mayurkale22 wdyt?

[obecny]

looks great

[mayurkale22]

Nice!

[mayurkale22]

@open-telemetry/javascript-approvers this is very high priority. I want to get this merged and released as a patch release which means not merging any breaking changes until this is in. @mayurkale22 wdyt?

I totally agree. In the next release, we can try to combine all the breaking changes (mostly related to metrics work).

[lykkin]

LGTM! Out of curiosity, what's the perf difference with the added before/after hooks?

[dyladan]

LGTM! Out of curiosity, what's the perf difference with the added before/after hooks?

We don't really have a great way to compare the performance here, but it should be relatively minimal as they are only called when async contexts are created/destroyed. Sync function calls should have no performance impact from this. Moreover, even if there is a performance impact, correctness is more important than speed.

[lykkin]

Sync function calls should have no performance impact from this.

My concern was more about the work done on entry/exit of the async context - with the introduction of the before and after hooks there is going to be some extra work for all network/fs/promise/async resource/etc work done by the runtime. These changes have perf implications for a user beyond how they interact with the OTel API. In my experience, the async hook overhead is smaller than the alternatives and the work in the hook is minimal so I'm not terribly concerned because I totally agree with you here.

Moreover, even if there is a performance impact, correctness is more important than speed.

It would be nice to have benchmarks for these kinds of things in the future, but considering it covers a gap in functionality maybe it's just gravy at this point.

[vmarchaud]

My concern was more about the work done on entry/exit of the async context - with the introduction of the before and after hooks there is going to be some extra work for all network/fs/promise/async resource/etc work done by the runtime. These changes have perf implications for a user beyond how they interact with the OTel API. In my experience, the async hook overhead is smaller than the alternatives and the work in the hook is minimal so I'm not terribly concerned because I totally agree with you here.

IMO if people want performance over UX (by UX i mean getting all traces automatically), they must not use the async hooks scope manager and trace manually the operations they want to.

It would be nice to have benchmarks for these kinds of things in the future, but considering it covers a gap in functionality maybe it's just gravy at this point.

I agree that we should have some benchmark in place, plus they are not really hard to implement. Ive opened #1123