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

Jump to bottom

fix(instrumentation-http): fix http/https ESM instr for 'import defaultExport from' style #5075

Merged
merged 2 commits into from
Oct 22, 2024
Merged

fix(instrumentation-http): fix http/https ESM instr for 'import defaultExport from' style #5075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
699978c
fix(instrumentation-http): fix http/https ESM instr for 'import defau…
trentm Oct 16, 2024
8e6afd9
Merge branch 'main' into tm-instr-http-esm
trentm Oct 22, 2024
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 @@ -49,6 +49,7 @@ All notable changes to experimental packages in this project will be documented
* `OTEL_EXPORTER_OTLP_LOGS_INSECURE`
* fix(sdk-node): use warn instead of error on unknown OTEL_NODE_RESOURCE_DETECTORS values [#5034](https://github.com/open-telemetry/opentelemetry-js/pull/5034)
* fix(exporter-logs-otlp-proto): Use correct config type in Node constructor
* fix(instrumentation-http): Fix instrumentation of `http.get`, `http.request`, `https.get`, and `https.request` when used from ESM code and imported via the `import defaultExport from 'http'` style. [#5024](https://github.com/open-telemetry/opentelemetry-js/issues/5024) @trentm

### :books: (Refine Doc)

Expand Down
18 changes: 16 additions & 2 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 @@ -235,17 +235,24 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
'http',
['*'],
(moduleExports: Http): Http => {
const isESM = (moduleExports as any)[Symbol.toStringTag] === 'Module';
if (!this.getConfig().disableOutgoingRequestInstrumentation) {
const patchedRequest = this._wrap(
moduleExports,
'request',
this._getPatchOutgoingRequestFunction('http')
) as unknown as Func<http.ClientRequest>;
this._wrap(
const patchedGet = this._wrap(
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, my IDE complains here as we're using a void return type. It looks though that the types for shimmer are actually incorrect here as it does, in fact, return something: https://github.com/othiym23/shimmer/blob/b2b29d760aa664767f71f37dbcbfac0b9250421b/index.js#L57

Maybe we should note this in a comment for future reference. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pichlermarc Hrm, I don't get that complaint in VS Code (nor in 'npm run lint' or warnings from 'npm run compile'). I'm a little curious what our config differences are here. I haven't spent time polishing VS Code config.

It looks though that the types for shimmer are actually incorrect here

So I guess this would be a bug against https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/shimmer/index.d.ts
Without some way to show that error or warning, I don't yet want to create the issue for that.

moduleExports,
'get',
this._getPatchOutgoingGetFunction(patchedRequest)
);
if (isESM) {
// To handle `import http from 'http'`, which returns the default
// export, we need to set `module.default.*`.
(moduleExports as any).default.request = patchedRequest;
(moduleExports as any).default.get = patchedGet;
}
}
if (!this.getConfig().disableIncomingRequestInstrumentation) {
this._wrap(
Expand Down Expand Up @@ -275,17 +282,24 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
'https',
['*'],
(moduleExports: Https): Https => {
const isESM = (moduleExports as any)[Symbol.toStringTag] === 'Module';
if (!this.getConfig().disableOutgoingRequestInstrumentation) {
const patchedRequest = this._wrap(
moduleExports,
'request',
this._getPatchHttpsOutgoingRequestFunction('https')
) as unknown as Func<http.ClientRequest>;
this._wrap(
const patchedGet = this._wrap(
moduleExports,
'get',
this._getPatchHttpsOutgoingGetFunction(patchedRequest)
);
if (isESM) {
// To handle `import https from 'https'`, which returns the default
// export, we need to set `module.default.*`.
(moduleExports as any).default.request = patchedRequest;
(moduleExports as any).default.get = patchedGet;
}
}
if (!this.getConfig().disableIncomingRequestInstrumentation) {
this._wrap(
Expand Down
Loading
Loading