open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.77k stars 811 forks source link

Consolidate sync and async resource interfaces #3582

Open dyladan opened 1 year ago

dyladan commented 1 year ago

In #3460 we introduced resources which may be sync or async. In a future version we should consolidate the interfaces into a single point of entry.

Because the resource class can now contain async unresolved resource attributes, we can remove the detectResourcesAsync. All resource detectors can return a sync resource that may contain async attributes.

dyladan commented 1 year ago

Adding needs refinement label. There are several proposals

pichlermarc commented 1 year ago

I went through the code, and I think we can apply all of the above proposals.

The hardest one to do might be

  • remove asyncAttributesPending and waitForAsyncAttributes. Make getAttributes async

We may also be able to change the ResourceMetrics.resource to a simpler type instead of IResource as there should be no need for resource merging in the exporters. This should be easy for metrics, but doing that to ReadableSpan might require more work (due to the way that resource (not attributes) is carried through from the TracerProvider to the Tracer to each ReadableSpan and eventually to the exporter via the SpanProcessor).

I think it would be best to await the resource (once) in the export pipeline before passing it to the exporter. Not awaiting before the exporter would mean many promises created as each span has a Resource instance attached to it that needs to be awaited by the exporter before exporting. Even though all spans resources are known to be the same by the SDK, an exporter may not be able to rely on that knowledge in future SDK minor releases, therefore it may need to defensively await all of them.

david-luna commented 6 days ago

I went through the code, and I think we can apply all of the above proposals.

The hardest one to do might be

  • remove asyncAttributesPending and waitForAsyncAttributes. Make getAttributes async

We may also be able to change the ResourceMetrics.resource to a simpler type instead of IResource as there should be no need for resource merging in the exporters. This should be easy for metrics, but doing that to ReadableSpan might require more work (due to the way that resource (not attributes) is carried through from the TracerProvider to the Tracer to each ReadableSpan and eventually to the exporter via the SpanProcessor).

I think it would be best to await the resource (once) in the export pipeline before passing it to the exporter. Not awaiting before the exporter would mean many promises created as each span has a Resource instance attached to it that needs to be awaited by the exporter before exporting. Even though all spans resources are known to be the same by the SDK, an exporter may not be able to rely on that knowledge in future SDK minor releases, therefore it may need to defensively await all of them.

@pichlermarc I see this has needs:refinement label but I guess we could start working with the 3 points mentioned by @dyladan. Then discuss and decide about ResourceMetrics.resource and exporters

david-luna commented 6 days ago

@pichlermarc @dyladan I've created a draft PR. Could you have a quick look to check if aligns with what you have in mind?