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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 17, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51f2454
refactor(instrumentation-http): migrate away from getEnv()
pichlermarc Feb 6, 2025
9548ba0
Update experimental/CHANGELOG.md
pichlermarc Feb 13, 2025
dc8fdc8
Merge branch 'main' into feat/migrate-from-getenv-http-instr
pichlermarc Feb 13, 2025
89fd849
Merge branch 'main' into feat/migrate-from-getenv-http-instr
pichlermarc Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions experimental/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ All notable changes to experimental packages in this project will be documented
* refactor(exporter-prometheus): remove unnecessary isNaN() check [#5377](https://github.com/open-telemetry/opentelemetry-js/pull/5377) @cjihrig
* refactor(sdk-node): move code to auto-instantiate propagators into utils [#5355](https://github.com/open-telemetry/opentelemetry-js/pull/5355) @pichlermarc
* chore: unpin `@opentelemetry/semantic-conventions` dep to allow better de-duplication in installs [#5439](https://github.com/open-telemetry/opentelemetry-js/pull/5439) @trentm
* refactor(instrumentation-http): migrate away from getEnv() [????](https://github.com/open-telemetry/opentelemetry-js/pull/????) @pichlermarc

## 0.57.0

Expand Down
13 changes: 6 additions & 7 deletions experimental/packages/opentelemetry-instrumentation-http/src/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,14 @@ import {
ValueType,
} from '@opentelemetry/api';
import {
getStringListFromEnv,
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';

RPCType,
setRPCMetadata,
} from '@opentelemetry/core';
import type * as http from 'http';
import type * as https from 'https';
Expand All @@ -46,12 +50,6 @@ import {
InstrumentationNodeModuleDefinition,
safeExecuteInTheMiddle,
} from '@opentelemetry/instrumentation';
import {
RPCMetadata,
RPCType,
setRPCMetadata,
getEnv,
} from '@opentelemetry/core';
import { errorMonitor } from 'events';
import {
ATTR_HTTP_REQUEST_METHOD,
Expand Down Expand Up @@ -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.

if (entry.toLowerCase() === 'http/dup') {
// http/dup takes highest precedence. If it is found, there is no need to read the rest of the list
this._semconvStability = SemconvStability.DUPLICATE;
Expand Down
Loading