Skip to content
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

feat(sdk-node): add serviceInstanceIdDetector to NodeSDK #4626

Merged
merged 6 commits into from
Apr 17, 2024

Conversation

maryliag
Copy link
Contributor

@maryliag maryliag commented Apr 10, 2024

Follow up from #4608

Adds the resource detector ServiceInstanceIDDetector on the NodeSDK constructor.
It only gets added by default if the value serviceinstance is part of the list OTEL_NODE_RESOURCE_DETECTORS

@maryliag maryliag force-pushed the serviceid-detector branch from 6dcde28 to 1991b3c Compare April 11, 2024 13:05
@maryliag maryliag marked this pull request as ready for review April 11, 2024 13:08
@maryliag maryliag requested a review from a team April 11, 2024 13:08
@maryliag maryliag force-pushed the serviceid-detector branch 2 times, most recently from 109f488 to c1021db Compare April 15, 2024 12:58
@maryliag maryliag changed the title feat(sdk-node): add serviceInstanceIDDetector to NodeSDK feat(sdk-node): add serviceInstanceIdDetector to NodeSDK Apr 15, 2024
@maryliag maryliag force-pushed the serviceid-detector branch from c1021db to 914fd8c Compare April 15, 2024 13:07
@maryliag
Copy link
Contributor Author

Rebased branch. @pichlermarc let me know if this implementation is good, so I can add proper documentation on how to use the new detector and the OTEL_NODE_RESOURCE_DETECTORS

@maryliag maryliag force-pushed the serviceid-detector branch from 95b1573 to 25fdcf8 Compare April 15, 2024 17:55
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good 👍
Just a few comments about docs/env var use 🙂

@maryliag maryliag force-pushed the serviceid-detector branch 2 times, most recently from 1325021 to 6accadc Compare April 16, 2024 17:23
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few nits then this should be good to merge.

packages/opentelemetry-core/src/utils/environment.ts Outdated Show resolved Hide resolved
packages/opentelemetry-core/src/utils/environment.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
maryliag and others added 6 commits April 17, 2024 08:38
Follow up from open-telemetry#4608

Adds the resource detector ServiceInstanceIDDetector on the NodeSDK constructor.
It only gets added by default on any of those conditions:
- the value `serviceinstance` is part of the list `OTEL_NODE_RESOURCE_DETECTORS`
- `OTEL_NODE_EXPERIMENTAL_DEFAULT_SERVICE_INSTANCE_ID` is set to `true`
@maryliag maryliag force-pushed the serviceid-detector branch from 6accadc to 0480298 Compare April 17, 2024 12:38
@maryliag
Copy link
Contributor Author

nits solved! 😄

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@pichlermarc pichlermarc merged commit 73fddf9 into open-telemetry:main Apr 17, 2024
18 checks passed
@maryliag maryliag deleted the serviceid-detector branch April 17, 2024 14:31
@maryliag maryliag restored the serviceid-detector branch April 17, 2024 20:20
@maryliag maryliag deleted the serviceid-detector branch April 19, 2024 15:09
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…try#4626)

* feat(sdk-node): add serviceInstanceIDDetector to NodeSDK

Follow up from open-telemetry#4608

Adds the resource detector ServiceInstanceIDDetector on the NodeSDK constructor.
It only gets added by default on any of those conditions:
- the value `serviceinstance` is part of the list `OTEL_NODE_RESOURCE_DETECTORS`
- `OTEL_NODE_EXPERIMENTAL_DEFAULT_SERVICE_INSTANCE_ID` is set to `true`

* remove OTEL_NODE_EXPERIMENTAL_DEFAULT_SERVICE_INSTANCE_ID

Signed-off-by: maryliag <[email protected]>

* update readme on how to use `OTEL_NODE_RESOURCE_DETECTORS`

* feedback from review

* Update experimental/packages/opentelemetry-sdk-node/README.md

Co-authored-by: Marc Pichler <[email protected]>

* feedback from review

---------

Signed-off-by: maryliag <[email protected]>
Co-authored-by: Marc Pichler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants