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

fix(instrumentation-http): Ensure instrumentation of http.get and https.get work when used in ESM code #4866

Merged
merged 11 commits into from
Jul 24, 2024

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Jul 15, 2024

The issue was that the _wraping of http.get was getting the
just-wrapped http.request by accessing moduleExports.request.
However, when wrapping an ES module the moduleExports object from IITM
is a Proxy object that allows setting a property, but not re-getting
that set property.

The fix is to use the wrapped http.request from the this._wrap call.
That required fixing a bug in the IITM code-path of
InstrumentationBase.prototype._wrap to return the wrapped property.
(The previous code was doing return Object.defineProperty(...), which
returns the moduleExports, not the defined property.)

Fixes: #4857

…https.get` work when used in ESM code

The issue was that the `_wrap`ing of `http.get` was getting the
just-wrapped `http.request` by accessing `moduleExports.request`.
However, when wrapping an ES module the `moduleExports` object from IITM
is a Proxy object that allows setting a property, but *not* re-getting
that set property.

The fix is to use the wrapped `http.request` from the `this._wrap` call.
That required fixing a bug in the IITM code-path of
`InstrumentationBase.prototype._wrap` to return the wrapped property.
(The previous code was doing `return Object.defineProperty(...)`, which
returns the moduleExports, not the defined property.)

Fixes: open-telemetry#4857
@trentm trentm self-assigned this Jul 15, 2024
@trentm trentm requested a review from a team July 15, 2024 22:45
"test": "nyc mocha test/**/*.test.ts",
"test:cjs": "nyc mocha test/**/*.test.ts",
"test:esm": "nyc node --experimental-loader=@opentelemetry/instrumentation/hook.mjs ../../../node_modules/.bin/mocha 'test/**/*.test.mjs'",
"test": "npm run test:cjs && npm run test:esm",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewer note: This test:esm script follows the same pattern used for ESM tests in opentelemetry-instrumentation.

@@ -49,7 +49,7 @@
"tdd:node": "npm run test -- --watch-extensions ts --watch",
"tdd:browser": "karma start",
"test:cjs": "nyc mocha 'test/**/*.test.ts' --exclude 'test/browser/**/*.ts'",
"test:esm": "nyc node --experimental-loader=./hook.mjs ../../../node_modules/mocha/bin/mocha 'test/node/*.test.mjs' test/node/*.test.mjs",
"test:esm": "nyc node --experimental-loader=./hook.mjs ../../../node_modules/mocha/bin/mocha 'test/node/*.test.mjs'",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewer note: I think this was a typo from when this was originally added. The additional test/node/*.test.mjs arg is redundant.

value: wrapped,
});
return wrapped;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewer note: From a quick check, I didn't see any core or contrib-repo instrumentations using the return value of this._wrap. Arguably this is a "fix" for @opentelemetry/instrumentation, but I didn't mention it in the CHANGELOG.

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

just a nit, otherwise LGTM

@hectorhdzg
Copy link
Member

@trentm sorry I'm terrible with acronyms, what IITM means?

@trentm
Copy link
Contributor Author

trentm commented Jul 17, 2024

@hectorhdzg

IITM

My bad: It is import-in-the-middle (https://github.com/nodejs/import-in-the-middle). (And often RITM for require-in-the-middle.)

Copy link
Member

@hectorhdzg hectorhdzg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

This is great, thanks for working on this! Appreciate the test updates as well. I left a few nits around the tests but the functionality itself is good. I'm now wondering about other instrumentations and what can be done there... but for now 🚀

return new InstrumentationNodeModuleDefinition(
'test-esm-module',
['*'],
moduleExports => {
this._wrap(moduleExports, 'testFunction', () => {
return () => 'patched';
const wrapRetval = this._wrap(moduleExports, 'testFunction', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const wrapRetval = this._wrap(moduleExports, 'testFunction', () => {
const wrappedReturnValue = this._wrap(moduleExports, 'testFunction', () => {

This is a total nit, but I wonder if we could use a full name for this - primarily because this tends to be a confusing functionality and I'm a sucker for detailed variable names. Maybe something like patchedModule, similar to the patchedRequest you have in the http code? Nonblocking, especially since it's a test.

@JamieDanielson
Copy link
Member

Also I think this updates open-telemetry/opentelemetry-js-contrib#1942 now for http instrumentation?

@trentm trentm added this pull request to the merge queue Jul 24, 2024
Merged via the queue into open-telemetry:main with commit d4035eb Jul 24, 2024
19 checks passed
@trentm trentm deleted the tm-fix-esm-http.get branch July 24, 2024 05:09
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…https.get` work when used in ESM code (open-telemetry#4866)

* fix(instrumentation-http): Ensure instrumentation of `http.get` and `https.get` work when used in ESM code

The issue was that the `_wrap`ing of `http.get` was getting the
just-wrapped `http.request` by accessing `moduleExports.request`.
However, when wrapping an ES module the `moduleExports` object from IITM
is a Proxy object that allows setting a property, but *not* re-getting
that set property.

The fix is to use the wrapped `http.request` from the `this._wrap` call.
That required fixing a bug in the IITM code-path of
`InstrumentationBase.prototype._wrap` to return the wrapped property.
(The previous code was doing `return Object.defineProperty(...)`, which
returns the moduleExports, not the defined property.)

Fixes: open-telemetry#4857

* correct typo in the changelog message

* does this fix the test:esm script running on windows?

* remove other console.logs (presumably dev leftovers) from tests in this file

* test name suggestion

Co-authored-by: Jamie Danielson <[email protected]>

* test name suggestion

Co-authored-by: Jamie Danielson <[email protected]>

* test name suggestion

Co-authored-by: Jamie Danielson <[email protected]>

* test name suggestion

Co-authored-by: Jamie Danielson <[email protected]>

* var naming suggestion: expand cres and creq, the abbrev isn't obvious

---------

Co-authored-by: Jamie Danielson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpInstrumentation does not create outgoing request span for http.get in ESM
4 participants