Skip to content

Conversation

@pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Feb 11, 2025

Which problem is this PR solving?

We're planning to remove getEnv() as it does not scale well when everyone has to update @opentelemetry/core to get their env vars. For OTEL_* there will be a replacement utility function, but until this one is available, we can use process.env directly.

Refs open-telemetry/opentelemetry-js#5443

Short description of the changes

Moves code for parsing OTEL_LOG_LEVEL to @opentelemetry/auto-instrumentations-node here. It may be changed later as we add similar code to @opentelemetry/sdk-node.

@codecov
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.41%. Comparing base (8c56f8d) to head (e96a0a5).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2708      +/-   ##
==========================================
- Coverage   92.45%   92.41%   -0.05%     
==========================================
  Files         171      171              
  Lines        8140     8146       +6     
  Branches     1652     1654       +2     
==========================================
+ Hits         7526     7528       +2     
- Misses        614      618       +4     
Files with missing lines Coverage Δ
...tapackages/auto-instrumentations-node/src/utils.ts 98.31% <100.00%> (+0.08%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

I'm a little more hesitant on this change, given that this package, sdk-node, and https://github.com/open-telemetry/opentelemetry-lambda/blob/faa6c56645b77d58f170f57ab016e845d815c0a8/nodejs/packages/layer/src/wrapper.ts#L470 would all need this change.

It might still be the easiest transition away from getEnv(), so I'm approving.

@pichlermarc
Copy link
Member Author

I'm a little more hesitant on this change, given that this package, sdk-node, and https://github.com/open-telemetry/opentelemetry-lambda/blob/faa6c56645b77d58f170f57ab016e845d815c0a8/nodejs/packages/layer/src/wrapper.ts#L470 would all need this change.

It might still be the easiest transition away from getEnv(), so I'm approving.

My original draft of the PR in core actually had a utility function to do this. I'll look into adding it back when I open the PR to update @opentelemetry/sdk-node - if there's three places in which this is used, I think there's a good case to add a utility like this. 👍

@pichlermarc pichlermarc merged commit fd547c8 into open-telemetry:main Feb 13, 2025
10 checks passed
deejay1 pushed a commit to deejay1/opentelemetry-js-contrib that referenced this pull request Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants