-
Notifications
You must be signed in to change notification settings - Fork 819
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
Create sync resource with some attributes that resolve asynchronously #3460
Create sync resource with some attributes that resolve asynchronously #3460
Conversation
… into feat/sync-resource-detectors � Conflicts: � experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts � packages/opentelemetry-resources/src/platform/node/detect-resources.ts � packages/opentelemetry-sdk-trace-base/src/export/SimpleSpanProcessor.ts � packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3460 +/- ##
==========================================
+ Coverage 92.32% 94.04% +1.71%
==========================================
Files 153 268 +115
Lines 3608 7920 +4312
Branches 727 1641 +914
==========================================
+ Hits 3331 7448 +4117
- Misses 277 472 +195
|
packages/opentelemetry-resources/src/platform/node/detect-resources.ts
Outdated
Show resolved
Hide resolved
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.
Sorry for the late review. This can be a very nice addition. Thank you for working on this!
Just a few comments I'd like to address before merging.
packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/src/platform/browser/detect-resources.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/src/platform/node/detect-resources.ts
Outdated
Show resolved
Hide resolved
removed unnecessary await Co-authored-by: Chengzhong Wu <[email protected]>
packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/src/platform/node/detect-resources.ts
Outdated
Show resolved
Hide resolved
…with .then to always return an object even on rejection
…rns promise that is never rejected
…t resolve asynchronously (open-telemetry#3460)" This reverts commit 47444f2.
`NodeSDK.start()` become `synchronous` ref: open-telemetry/opentelemetry-js#3460
`NodeSDK.start()` become `synchronous` ref: open-telemetry/opentelemetry-js#3460
`NodeSDK.start()` become `synchronous` ref: open-telemetry/opentelemetry-js#3460
…[email protected] - open-telemetry/opentelemetry-js#3460 - Deprecated detectResources() in favor of a new method detectResourcesSync() which defers promises into the resource to be resolved later - introduced await resource.waitForAsyncAttributes?.();
…[email protected] - open-telemetry/opentelemetry-js#3460 - Deprecated detectResources() in favor of a new method detectResourcesSync() which defers promises into the resource to be resolved later - introduced await resource.waitForAsyncAttributes?.();
…[email protected] - open-telemetry/opentelemetry-js#3460 - Deprecated detectResources() in favor of a new method detectResourcesSync() which defers promises into the resource to be resolved later - introduced await resource.waitForAsyncAttributes?.();
…[email protected] - open-telemetry/opentelemetry-js#3460 - Deprecated detectResources() in favor of a new method detectResourcesSync() which defers promises into the resource to be resolved later - introduced await resource.waitForAsyncAttributes?.();
…[email protected] (#1462) - Refs: open-telemetry/opentelemetry-js#3460 - Deprecated detectResources() in favor of a new method detectResourcesSync() which defers promises into the resource to be resolved later - introduced await resource.waitForAsyncAttributes?.();
Which problem is this PR solving?
Fixes #2912
Short description of the changes
I spoke to @aabmass and agreed on taking on this issue and reopening his PR #2933 and build on it, as I think he has one of the best solutions for this issue.
@opentelemetry/sdk-node
detectResources()
in favor of a new methoddetectResourcesSync()
which defers promises into the resource to be resolved laterDetector.detect()
signature to allow it to be synchronous and updated documentation that the synchronous variant should be used@opentelemetry/sdk-node
NodeSDK.start()
and its dependencies to now be synchronous (doesn't return a promise). This is a breaking change, but that package is experimental.BatchSpanProcessorBase
andSimpleSpanProcessor
to await the resource promise before calling exporters, if the resource has asynchronous attributes AND they have not already resolved. I.e. if there is no promise in the Resource (the case for existing detectors and code), the behavior is unchanged.Type of change
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: