-
Notifications
You must be signed in to change notification settings - Fork 804
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(resources): wait for async attributes for detecting resources #4687
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested the change locally. It solved the issue, the message is not logged and host.id is present on spans 👌
Hey @pichlermarc, sorry for mentioning you; I'm not sure who would be the best person to ask for a review here, It still requires a code owner approval, is everything good here? I am happy to help with pushing this forward |
This PR 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. |
any update for this MR? today i'm using this workaround: #4638 (comment) but after the merge of this change, i would like to remove them. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4687 +/- ##
==========================================
+ Coverage 93.26% 93.27% +0.01%
==========================================
Files 317 317
Lines 8194 8195 +1
Branches 1640 1641 +1
==========================================
+ Hits 7642 7644 +2
+ Misses 552 551 -1
|
Failure on
|
…en-telemetry#4687) Co-authored-by: David Luna <[email protected]>
*Issue #, if available:* *Description of changes:* Due to a [bug](open-telemetry/opentelemetry-js#4687) in upstream, the `detectResourcesSync` doesn't wait for the async attributes. This happens if we use the async version of some resource detector. The [fix](open-telemetry/opentelemetry-js#4687) is in another upstream release. We don't want to upgrade the upstream version for now. To avoid this issue, we switch to the sync version of the detectors and this should have no impact for the functionalities (as `detectResourcesSync` waits for all async detectors anyway and works for both sync and async detectors). In the future, even if we upgrade upstream version, we don't need to switch back to async detectors. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Which problem is this PR solving?
Recently,
HostDetector
was added as default for detectors inNodeSDK
, this produces the error message:Accessing resource attributes before async attributes settled
, asdetectResourcesSync
is not waiting for async attributes to be resolved forHostDetector
, what causes the lack ofmachine_id
passed to it.Fixes #4638
Short description of the changes
I've updated the test case for the issue and added an await statement to wait for async attributes to be resolved for the
detectResourcesSync
function.Type of change
How Has This Been Tested?
I've added unit tests, the configuration for reproducing is described in the linked issue #4638