Consolidate sync and async resource interfaces

[dyladan]

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]

Adding needs refinement label. There are several proposals

• remove asyncAttributesPending and waitForAsyncAttributes. Make getAttributes async
• remove async detector interface and keep only sync
• remove compatibility code (and tests) between old and new resource

[pichlermarc]

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]

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]

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

[seemk]

Letting async spill over to Resource due to the highly coupled configuration of NodeSDK was maybe a mistake in the first place. There are only a few detectors (GCP, Azure, AWS, Alibaba) requiring asynchronous detection, none of these are shipped with the Node SDK out of the box.

Look at the resource related configuration fields of NodeSDK, does it look intuitive what are you actually configuring without looking at the code?

export interface NodeSDKConfiguration {
  resource: IResource;
  resourceDetectors: Array<Detector | DetectorSync>;
  autoDetectResources: boolean;
  mergeResourceWithDefaults?: boolean;
}

Why does it need a resourceDetectors field? If you need extra detectors you can just import them (which you need to do anyway) , call detect on them and pass the resource to NodeSDK.

With #3460 there's suddenly loads of code that needs to know about async attributes (span processors).

My suggestions are:

• Drop async from Resource.
• Keep Node SDK's start function synchronous.
• Drop resourceDetectors configuration option from NodeSDK.
• Do resource detection as a separate call, e.g.

import { MyAsyncDetector, MySyncDetector } from 'xyz';
import { detectResources, detectResourcesAsync } from '@opentelemetry/resources';

const sdk = new NodeSDK({ resource: await detectResources([new MyAsyncDetector()]) });
// or
const sdk = new NodeSDK({ resource: detectResourcesSync([new MySyncDetector()]) });

• Resource detection can still be either sync or async - this is up to the users of NodeSDK to decide.
• If users want asynchronous detection, then they shoud just use top level await, which seems to be a simpler tradeoff than letting async Resource appear in every part of the SDK.

There are other SDK implementations besides NodeSDK and the peculiarities of its configuration fields are seeping into the JS SDK in general.

[pichlermarc]

@seemk hmm, from what I understand this is what this issue in spirit is about.

IResource.attributes becomes IResource.getAttributes() that is async - it's only intended to be awaited by the export pipeline (before being passed to the exporter - that part is async already anyway today) so the user never needs to concern themselves with retrieving the actual attributes.

IResource.asyncAttributesPending and IResource.awaitAsyncAttributes() get removed - an async IResource.getAttributes() does everything they did but easier. These two were only there to not break users within the 1.x range, but we can make that breaking change going to 2.0.

IResource.merge() remains sync - users may merge synchronous and asynchronous resources as they wish, with no distinction there's between them.

detectResources() becomes sync. That means every SDK setup can get consolidated to be

// no matter if you use NodeSDK
const sdk = new NodeSDK({ resource: detectResources([new MyAsyncDetector(), new MySyncDetector()]) });

// a MeterProvider
const meterProvider = new MeterProvider({ resource: detectResources([new MyAsyncDetector(), new MySyncDetector()]) });

// or a TracerProvider
const tracerProvider = new NodeTracerProvider({ resource: detectResources([new MyAsyncDetector(), new MySyncDetector()]) });

Since awaiting is done in the export pipeline, sdk.start() remains synchronous. Everything gets simpler for the end-user and they don't ever have to care about top-level await.

I think we should reduce complexity and getting rid of stuff we don't need, but async resources are a very often recurring feature request. Consolidating the interface like this and having the export pipeline await resource.getAttributes() is a trade-off for sure, but I think if we remove the current footguns from the interface it becomes a lot easier for everybody to understand what's going on.