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

refactor(instrumentation-http): migrate away from getEnv() #5469

Conversation

pichlermarc
Copy link
Member

Which problem is this PR solving?

Migrates away from getEnv() to the new functions and inline-defaults introduced in #5443. Since the HTTP instrumentation can only be used on Node.js, everything stays as-is for the end-user. This is a pure refactor.

Refs #5217

Type of change

  • refactor

How Has This Been Tested?

  • Existing tests

@pichlermarc pichlermarc requested a review from a team as a code owner February 13, 2025 16:05
@pichlermarc pichlermarc added the target:next-major-release This PR targets the next major release (`next` branch) label Feb 13, 2025
@pichlermarc pichlermarc added this to the OpenTelemetry SDK 2.0 milestone Feb 13, 2025
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.93%. Comparing base (5b988c8) to head (89fd849).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5469   +/-   ##
=======================================
  Coverage   94.93%   94.93%           
=======================================
  Files         309      309           
  Lines        8002     8002           
  Branches     1686     1686           
=======================================
  Hits         7597     7597           
  Misses        405      405           

@@ -108,7 +106,8 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
super('@opentelemetry/instrumentation-http', VERSION, config);
this._headerCapture = this._createHeaderCapture();

for (const entry of getEnv().OTEL_SEMCONV_STABILITY_OPT_IN) {
for (const entry of getStringListFromEnv('OTEL_SEMCONV_STABILITY_OPT_IN') ??
[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes prettier is trolling.

hrTime,
hrTimeDuration,
hrTimeToMilliseconds,
suppressTracing,
RPCMetadata,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should use a lint rule for this. Probably is difficult because sometimes the import order matters and is not just code style.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think a lint rule could be helpful. 👍 I don't think there's a case where we'd split importing from the same package, most of the time when import order matters it's from different packages, so we'd do:

import {a} from 'my-package';
// doing some stuff
import {b} from 'other-package';

but not

import {a} from 'my-package';
// doing some stuff
import {b} from 'my-package';

@pichlermarc pichlermarc added this pull request to the merge queue Feb 17, 2025
Merged via the queue into open-telemetry:main with commit 52d0331 Feb 17, 2025
18 checks passed
@pichlermarc pichlermarc deleted the feat/migrate-from-getenv-http-instr branch February 17, 2025 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target:next-major-release This PR targets the next major release (`next` branch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants