bug(metrics-sdk): destructing `ObservableResultImpl` breaks collecting metrics value

[vmarchaud]

trafficstars

Metrics SDK: 0.32.0

metrics.workersQueueWaitTime.addCallback(({ observe }) => {
    observe(this.drawWorkers.waitTime.p50, { percentile: 'p50', type: 'draw' })
})

yield:

TypeError: Cannot read properties of undefined (reading '_descriptor')
        at observe (/sensored/packages/api/node_modules/@opentelemetry/sdk-metrics/src/ObservableResult.ts:38:14)
        at /sensored/packages/api/src/services/renderer.ts:154:9
        at /sensored/packages/api/node_modules/@opentelemetry/sdk-metrics/src/state/ObservableRegistry.ts:108:58

Because destructing set the this context of observe to the one in the callback instead of the ObservableResultImpl. The fix is quite straightforward:

metrics.workersQueueWaitTime.addCallback((result) => {
    result.observe(this.drawWorkers.waitTime.p50, { percentile: 'p50', type: 'draw' })
})

I'm not sure what we should do here as i don't expect end user to understand why this doesnt work, but at the same time not a lot of people will hit this. WDYT ?

cc @legendecas since you worked on the metrics sdk you might have an opinion on this ?

[legendecas]

Most of our SDK class methods are not safe to be called without a proper receiver being set, e.g.

const { getTracer } = tracerProvider;
getTracer('my-tracer'); // throws TypeError 

I believe this is a common pattern in the language. Is this callback style widely adopted?

[vmarchaud]

I'm not sure actually, we use it internally at my company for some case but not everytime

[legendecas]

I'd recommend avoiding extracting methods from a class instance, as we can not guarantee class methods are safe to be called without a receiver. Does this sound reasonable to you?

[vmarchaud]

Yeah it is but i'm not sure how we should document this, i don't think end user will not think this is a bug from otel

[weyert]

I didn’t know you could destruct class instance like this 🤔

[legendecas]

We can explicitly declare the this parameter for each of our public class methods so that TypeScript tools can complain on this mismatch (playground demo).

However, this requires manual maintenance on the type of those methods, as tsc can not generate it for us (for now).

[dyladan]

I didn’t know you could destruct class instance like this 🤔

you can't always as demonstrated

In JS I think it is somewhat common to destructure callback arguments like that. I've definitely done it and seen it done. I don't think there are many places in our API where we would have to solve this problem

[github-actions[bot]]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[legendecas]

@dyladan I don't think there are many places in our API where we would have to solve this problem

Yeah, we don't have many callbacks in the API package. Manually declaring the this parameter type can be a good approach to address this issue. Submitted https://github.com/open-telemetry/opentelemetry-js/pull/3497.