-
Notifications
You must be signed in to change notification settings - Fork 847
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
Converge sync and async resources #5350
Converge sync and async resources #5350
Conversation
packages/opentelemetry-resources/test/regression/existing-detectors-1-9-1.test.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.
Thank you for working on this 🙂
A quick preliminary review - I'll have a closer look at the implementation details next week :)
experimental/packages/opentelemetry-browser-detector/test/util.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/src/detectors/BrowserDetector.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/src/detectors/platform/node/OSDetector.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/src/detectors/BrowserDetector.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/test/detectors/node/machine-id/getMachineId-bsd.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/test/detectors/node/machine-id/getMachineId-darwin.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/test/detectors/node/machine-id/getMachineId-linux.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/src/detectors/BrowserDetector.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/src/detectors/platform/node/OSDetector.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/src/detectors/platform/node/ProcessDetector.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.
Take it with an unhealthy amount of salt, but other than the suggestions I left (feel free to make your own judgment call), it looks good to me!
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 think this LGTM.
I asked a Q at #5358 (comment)
I'm not totally clear on what the user's experience with resources (usage with the SDK I mean) will be after these changes at #5358, especially when Entities come along.
Fixes: #3582
This PR makes it so that any resource detector can detect sync and async resources, returning both in a single resource object.
attributes
key, which all attributes are nested under. This will make it easier to extend for entities in the future by adding a new key to the object.To be done in follow-up: