feat: expose withSpanAsync on NodeTracer #752

[vmarchaud]

This doens't work because of some bug inside the AsyncHookContextManager but i'm opening the PR in the mean time to get feedback on how we can accomplish this.

The main issue is that i need to use // @ts-ignore there since the withAsync is not exposed at the api level, don't know if we have other solutions here
WDYT ?

[vmarchaud]

@dyladan I've changed the return type of withSpanAsync to void. I failed to find a type that would say that we return what U return (and i believe thats the point of the Awaited new type being working on in TS).

I don't think we should have the behavior to return the value returned by the callback too, the aim of the API isn't to pass value but to set a context, WDYT ?

[dyladan]

@dyladan I've changed the return type of withSpanAsync to void. I failed to find a type that would say that we return what U return (and i believe thats the point of the Awaited new type being working on in TS).

I don't think we should have the behavior to return the value returned by the callback too, the aim of the API isn't to pass value but to set a context, WDYT ?

I think it is a very common use case that you want the return value of the called function. tracer.withAsync(() => getFromAPI());.

Think something like this should work?

<T extends () => any, U = ReturnType<T> extends Promise<infer V> ? V : ReturnType<T>>(context: Context, fn: T) => Promise<U>;

Or you could factor out the confusing typing

type Unpacked<T> = T extends Promise<infer U>
  ? U
  : T;

class Foo {
  async withAsync<T extends () => any, U = Unpacked<ReturnType<T>>>(context: Context, fn: T): Promise<U> {
    return fn();
  }
}

edit: you could force the function to be async too like this:

type Unpacked<T> = T extends Promise<infer U>
  ? U
  : T;

class Foo {
  async withAsync<T extends () => Promise<any>, U = Unpacked<ReturnType<T>>>(context: Context, fn: T): Promise<U> {
    return fn();
  }
}

[lykkin]

Could this functionality be integrated into the existing withSpan method by detecting if a promise has been passed back? Not sure if I missed the discussion around this while catching up, but the ergonomics would be better if you didn't have to change the method call when you swap a callback in withSpan to an async function.

You should be able to call fn, and clear the context in a .finally (or then/catch pair) off the return value. I would imagine the context propagation for the interstitial promises could be handled by async_hooks.

[dyladan]

There has been a lot of discussion around this and we decided to keep them separate for now. This decision came down to performance impact and the higher likelihood for subtle bugs using the more complex async mechanism.

[codecov]

Codecov Report

Merging #1011 into master will decrease coverage by 21.09%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1011       +/-   ##
===========================================
- Coverage   94.98%   73.88%   -21.10%     
===========================================
  Files         238       71      -167     
  Lines        9085     1570     -7515     
  Branches      822      271      -551     
===========================================
- Hits         8629     1160     -7469     
+ Misses        456      410       -46     

Impacted Files	Coverage Δ
packages/opentelemetry-core/test/utils/url.test.ts	0.00% <0.00%> (-100.00%)	⬇️
...ckages/opentelemetry-core/test/platform/id.test.ts	0.00% <0.00%> (-100.00%)	⬇️
...s/opentelemetry-core/src/platform/node/sdk-info.ts	0.00% <0.00%> (-100.00%)	⬇️
...pentelemetry-core/src/platform/node/performance.ts	0.00% <0.00%> (-100.00%)	⬇️
...pentelemetry-core/test/internal/validators.test.ts	0.00% <0.00%> (-100.00%)	⬇️
...ntelemetry-core/test/trace/NoRecordingSpan.test.ts	0.00% <0.00%> (-100.00%)	⬇️
...telemetry-core/test/platform/hex-to-base64.test.ts	0.00% <0.00%> (-100.00%)	⬇️
...elemetry-core/test/trace/spancontext-utils.test.ts	0.00% <0.00%> (-100.00%)	⬇️
...lemetry-core/test/trace/ProbabilitySampler.test.ts	0.00% <0.00%> (-100.00%)	⬇️
...try-core/test/trace/fixtures/test-package/index.js	0.00% <0.00%> (-100.00%)	⬇️
... and 183 more

[vmarchaud]

I've rebased on #1099 to run tests with it (and it works). waiting for it to be merged to set this one as ready for review

[vmarchaud]

As a solution has been to fix .with in #1099, we dont need this anymore