fix(resources): ensure async resource detectors are awaited and merged#5834
Conversation
|
|
| * Call this method to construct SDK components and register them with the OpenTelemetry API. | ||
| */ | ||
| public start(): void { | ||
| public async start(): Promise<void> { |
There was a problem hiding this comment.
This is a breaking change for anyone depending on the SDK. Are there really no alternatives to making the public SDK start method async?
There was a problem hiding this comment.
Thank you for the feedback!
Yes, making start() async is a breaking change, but it’s required to ensure all async detectors are resolved before merging the resource. If we keep it synchronous, we risk losing attributes from any async detectors (especially common with cloud metadata).
There was a problem hiding this comment.
Making the start async is not only a breaking change, but drastically complicates SDK setup as things must happen in a very particular order. Async resources are meant to be awaited by the exporter before being exported. I'm not sure where they're being read. Do you have a span processor or similar trying to read the async resource? I would much rather handle this in the SDK instead of making startup async again as that caused way too many headaches in the past.
| * @returns A Promise resolving to a single merged Resource | ||
| */ | ||
| export const detectResources = ( | ||
| export const detectResources = async ( |
There was a problem hiding this comment.
#5350 resource detectors should likely remain merged and synchronous as this is something that was updated from 1.x -> 2.x intentionally.
There was a problem hiding this comment.
The problem is that if a resource detector is async, the sync merge in 2.x means we may lose attributes if the Promise isn’t awaited.
Is it acceptable to risk incomplete resources, or is there a recommended pattern to handle async detectors in a fully backward-compatible way?
There was a problem hiding this comment.
In SDK 2.0 instead of the whole resource being a promise, any individual attribute value can be a promise. The promises are all awaited on first export so nothing is lost. The ResourceDetector should always be synchronous, but may return async values.
There was a problem hiding this comment.
Looks like a temp work around is found for this by the user, so we should avoid the startup async.
dyladan
left a comment
dyladan
left a comment
There was a problem hiding this comment.
I would not want to make startup async if it can be at all avoided
|
Closing this PR:
We're happy to triage a new issue and discuss a fix if a demonstrator is provided that shows the incorrect behavior. |
4 participants
Which problem is this PR solving?
This PR fixes an issue where resource attributes from asynchronous detectors were sometimes missing or incomplete when initializing the SDK. The root cause was that the detectResources function did not properly await the results of async resource detectors, which could lead to partial resource information—especially with cloud metadata or custom detectors.
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
5829
#5828
Short description of the changes
Refactored detectResources to be an async function that loops through all detectors, awaiting each one in sequence and merging their results.
Updated SDK usage to await the resource detection result before merging resources.
Ensures all resource attributes, including those provided asynchronously, are present in the merged Resource before SDK initialization completes.
Added/updated tests to confirm async detectors are handled correctly and their attributes are present.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: