-
Notifications
You must be signed in to change notification settings - Fork 619
refactor(resource-detector-gcp): migrate away from getEnv() #2705
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
refactor(resource-detector-gcp): migrate away from getEnv() #2705
Conversation
| attributes[SEMRESATTRS_CLOUD_PROVIDER] = CLOUDPROVIDERVALUES_GCP; | ||
|
|
||
| if (getEnv().KUBERNETES_SERVICE_HOST) | ||
| if (process.env.KUBERNETES_SERVICE_HOST) |
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.
Note for reviewers: this previously was an empty string when it was unset - now it's undefined. The semantics should be the same other than that.
| attributes[SEMRESATTRS_CONTAINER_NAME] = env.CONTAINER_NAME; | ||
| attributes[SEMRESATTRS_K8S_NAMESPACE_NAME] = process.env.NAMESPACE ?? ''; | ||
| attributes[SEMRESATTRS_K8S_POD_NAME] = process.env.HOSTNAME ?? ''; | ||
| attributes[SEMRESATTRS_CONTAINER_NAME] = process.env.CONTAINER_NAME ?? ''; |
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.
Note for reviewers:
See here for previous defaults:
NAMESPACE->
https://github.com/open-telemetry/opentelemetry-js/blob/15ba2d7da3bd6663ee6008e189f004bab93ff6fe/packages/opentelemetry-core/src/utils/environment.ts#L164HOSTNAME-> https://github.com/open-telemetry/opentelemetry-js/blob/15ba2d7da3bd6663ee6008e189f004bab93ff6fe/packages/opentelemetry-core/src/utils/environment.ts#L162CONTAINER_NAME-> https://github.com/open-telemetry/opentelemetry-js/blob/15ba2d7da3bd6663ee6008e189f004bab93ff6fe/packages/opentelemetry-core/src/utils/environment.ts#L88
3153771 to
8602d5b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2705 +/- ##
==========================================
- Coverage 92.40% 92.40% -0.01%
==========================================
Files 171 171
Lines 8142 8140 -2
Branches 1649 1652 +3
==========================================
- Hits 7524 7522 -2
Misses 618 618
|
Which problem is this PR solving?
We're planning to remove
getEnv()as it does not scale well when everyone has to update@opentelemetry/coreto get their env vars. ForOTEL_*there will be a replacement utility function. For all others, when the package does not need to be browser-compatible (which seems to be the case here) usingprocess.envdirectly should be fine.Refs open-telemetry/opentelemetry-js#5443
Short description of the changes
This PR removes
getEnv()withprocess.envand inlines the defaults from@opentelemetry/core'sDEFAULT_ENVIRONMENT