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

HttpInstrumentation does not create outgoing request span for http.get in ESM #4857

Closed
Lms24 opened this issue Jul 9, 2024 · 1 comment · Fixed by #4866
Closed

HttpInstrumentation does not create outgoing request span for http.get in ESM #4857

Lms24 opened this issue Jul 9, 2024 · 1 comment · Fixed by #4866
Assignees
Labels
bug Something isn't working has:reproducer This bug/feature has a minimal reproducer provided priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect

Comments

@Lms24
Copy link

Lms24 commented Jul 9, 2024

What happened?

Steps to Reproduce

I created a minimal reproduction that demonstrates the bug in this repository: https://github.com/Lms24/otel-node-http-get-esm-reproduction

To reproduce the bug, follow these instructions:

  1. run npm install
  2. run npm run start:cjs to observe http.get call span being printed to console
  3. run npm run start:esm to observe http.get call span NOT being printed to console
  4. uncomment http.request call in both, esm and cjs app.js files to observe that both start a span.

Expected Result

When making http.get calls from node:http(s), for example

import * as http from "node:http";
http.get("http://example.com", {...});

HttpInstrumentation should create a span for the outgoing request when the app is running in ESM mode, just like it does in CJS mode.

Actual Result

No span is created for http.get calls in ESM.

Additional Details

In ESM, http.request calls create a span (see commented out call in my reproduction repo).

In CJS, both, http.get and http.request calls create spans. So as far as I can tell, for CJS, everything works correctly.

OpenTelemetry Setup Code

import * as opentelemetry from "@opentelemetry/sdk-node";
import { ConsoleSpanExporter } from "@opentelemetry/sdk-trace-node";
import { HttpInstrumentation } from "@opentelemetry/instrumentation-http";
import { diag, DiagConsoleLogger, DiagLogLevel } from "@opentelemetry/api";
import moduleModule from "module";

const sdk = new opentelemetry.NodeSDK({
  traceExporter: new ConsoleSpanExporter(),
  instrumentations: [new HttpInstrumentation()],
});

moduleModule.register(
  "@opentelemetry/instrumentation/hook.mjs",
  import.meta.url
);

sdk.start();

diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.WARN);

package.json

{
  "name": "otel-http-get",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "start:cjs": "node --require ./cjs/instrument.js ./cjs/app.js",
    "start:esm": "node --import ./esm/instrument.mjs ./esm/app.mjs"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "@opentelemetry/api": "^1.9.0",
    "@opentelemetry/instrumentation-http": "^0.52.1",
    "@opentelemetry/sdk-metrics": "^1.25.1",
    "@opentelemetry/sdk-node": "^0.52.1",
    "@opentelemetry/sdk-trace-node": "^1.25.1"
  }
}

Relevant log output

No log output from ConsoleSpanExporter because no span is created/emitted.

@Lms24 Lms24 added bug Something isn't working triage labels Jul 9, 2024
@JamieDanielson JamieDanielson added priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect has:reproducer This bug/feature has a minimal reproducer provided and removed triage labels Jul 10, 2024
@trentm
Copy link
Contributor

trentm commented Jul 15, 2024

Thanks very much for the reproducer. I can repro.
The difference is in how @opentelemetry/instrumentation-http is wrapping http.get vs. http.request. With ES modules the wrapping code doesn't work for http.get. I'm working on a fix. I'll give more specific details then too.

trentm added a commit to trentm/opentelemetry-js that referenced this issue Jul 15, 2024
…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
github-merge-queue bot pushed a commit that referenced this issue Jul 24, 2024
…https.get` work when used in ESM code (#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: #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]>
mydea added a commit to getsentry/sentry-javascript that referenced this issue Sep 5, 2024
…13587)

This bumps all our internal OTEL instrumentation to their latest
version.

Mainly, this fixes two things:

* Mongoose now supports v7 & v8
open-telemetry/opentelemetry-js-contrib#2353
* A variety of bug fixes, including a problem with http.get in ESM mode
open-telemetry/opentelemetry-js#4857 - See:
https://github.com/open-telemetry/opentelemetry-js/releases/tag/experimental%2Fv0.53.0

Related:

* open-telemetry/opentelemetry-js#4975 Issue
about relaxing deps in "core" instrumentation packages
* PR to bump deps in `@prisma/instrumentation`:
prisma/prisma#25160
* PR to bump deps in `opentelemetry-instrument-remix`:
justindsmith/opentelemetry-instrumentations-js#52
* PR to bump deps in `opentelemetry-instrumentation-fetch-node`:
gas-buddy/opentelemetry-instrumentation-fetch-node#17
(which will also be superseded by
#13485 eventually)

* Closes #11499
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this issue 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
bug Something isn't working has:reproducer This bug/feature has a minimal reproducer provided priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
3 participants