fix(instrumentation-http): fix http/https ESM instr for 'import defaultExport from' style #5075
Conversation
…ltExport from' style 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. Fixes: open-telemetry#5024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5075 +/- ##
==========================================
- Coverage 93.27% 93.26% -0.02%
==========================================
Files 317 317
Lines 8195 8195
Branches 1641 1641
==========================================
- Hits 7644 7643 -1
- Misses 551 552 +1 |
| request: httpRequestC, | ||
| get: httpGetC, | ||
| }, | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Reviewer note: Be sure to use the "Hide whitespace" option for looking at this diff. There is an indentation change, so the diff is much smaller without the whitespace.
| 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( |
There was a problem hiding this comment.
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. 🙂
There was a problem hiding this comment.
@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
shimmerare 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.
…ltExport from' style (open-telemetry#5075)
Fix instrumentation of
http.get,http.request,https.get, andhttps.requestwhen used from ESM code and imported via theimport defaultExport from 'http'style.Fixes: #5024