fix(ioredis): fix instrumentation of ESM-imported ioredis #1694

trentm · 2023-09-21T20:22:01Z

With an ESM import the top-level object is a Module Namespace Object (https://tc39.es/ecma262/#sec-module-namespace-objects) that has no prototype. For compat, Node.js assigns the usual CommonJS module.exports to <module>.default, so use that when this is an ESM module.
Also, TypeScript translates class properties to assignments in the constructor after the super() call. Because super() can call init() and enable() synchronously, it calls back into 'IORedisInstrumentation' before 'traceSendCommand' was defined. Defining it as a method fixes that issue.

Checklist

Decide whether and how to add a test for this.

- With an ESM import the top-level object is a Module Namespace Object (https://tc39.es/ecma262/#sec-module-namespace-objects) that has no prototype. For compat, Node.js assigns the usual CommonJS module.exports to `<module>.default`, so use that when this is an ESM module. - Also, TypeScript translates class properties to assignments in the constructor after the super() call. Because super() can call init() and enable() synchronously, it calls back into 'IORedisInstrumentation' before 'traceSendCommand' was defined. Defining it as a method fixes that issue. Fixes: open-telemetry#1692

trentm · 2023-09-21T20:31:01Z

Note: I still have this in draft, because:

1. unit tests

I haven't added any unit tests for this. Adding a unit test for ESM is a little bit of work. I'd appreciate thoughts/suggestions from others. Options:

Skip unit tests for this. ;)
Have ioredis.test.ts spawn a separate process that runs a small ioredis-using .mjs file with the node --experimental-loader=... argument.
Add a separate npm run test:esm similar to here: https://github.com/open-telemetry/opentelemetry-js/blob/f2fc0d8787a82f33b1ce7e315759ddc61caf4f52/experimental/packages/opentelemetry-instrumentation/package.json#L51-L53

2. lint

I was unable to npm run lint locally:

% npm run lint

> @opentelemetry/instrumentation-ioredis@0.35.1 lint
> eslint . --ext .ts

TypeError: Error while loading rule 'prettier/prettier': context.getPhysicalFilename is not a function
Occurred while linting /Users/trentm/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-ioredis/src/index.ts
    at Object.create (/Users/trentm/tm/opentelemetry-js-contrib2/node_modules/eslint-plugin-prettier/eslint-plugin-prettier.js:124:40)
    at createRuleListeners (/Users/trentm/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-ioredis/node_modules/eslint/lib/linter/linter.js:746:21)
    at /Users/trentm/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-ioredis/node_modules/eslint/lib/linter/linter.js:916:31
    at Array.forEach (<anonymous>)
    at runRules (/Users/trentm/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-ioredis/node_modules/eslint/lib/linter/linter.js:861:34)
    at Linter._verifyWithoutProcessors (/Users/trentm/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-ioredis/node_modules/eslint/lib/linter/linter.js:1157:31)
    at Linter._verifyWithConfigArray (/Users/trentm/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-ioredis/node_modules/eslint/lib/linter/linter.js:1255:21)
    at Linter.verify (/Users/trentm/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-ioredis/node_modules/eslint/lib/linter/linter.js:1210:25)
    at Linter.verifyAndFix (/Users/trentm/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-ioredis/node_modules/eslint/lib/linter/linter.js:1400:29)
    at verifyText (/Users/trentm/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-ioredis/node_modules/eslint/lib/cli-engine/cli-engine.js:230:48)

I don't know what that is about.

trentm · 2023-09-21T21:33:18Z

I've resolved lint... by manually pointing to the eslint installed at the top level:

$ cd plugins/node/opentelemetry-instrumentation-ioredis
$ ../../../node_modules/.bin/eslint . --ext .ts

FWIW, just running npm run lint in that dir breaks on, I think, prettier/eslint-plugin-prettier#434 because there is an old incompatible eslint installed in this sub-package dir because of a transitive dep of test-all-versions:

% npm ls eslint
@opentelemetry/instrumentation-ioredis@0.35.1 /Users/trentm/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-ioredis
└─┬ test-all-versions@5.0.1
  └─┬ parse-env-string@1.0.0
    └─┬ standard@14.3.4
...
      └── eslint@6.8.0

Flarna · 2023-09-22T07:02:45Z

regarding the old eslint - this seems to be known issue since a while see here.

codecov · 2023-09-22T07:21:57Z

Codecov Report

Merging #1694 (cc31725) into main (659d7ba) will increase coverage by 0.00%.
The diff coverage is 85.71%.

❗ Current head cc31725 differs from pull request most recent head f6e6414. Consider uploading reports for the commit f6e6414 to get more accurate results

@@           Coverage Diff           @@
##             main    #1694   +/-   ##
=======================================
  Coverage   91.69%   91.70%           
=======================================
  Files         139      139           
  Lines        7139     7134    -5     
  Branches     1436     1437    +1     
=======================================
- Hits         6546     6542    -4     
+ Misses        593      592    -1

Files	Coverage Δ
...try-instrumentation-ioredis/src/instrumentation.ts	`90.21% <85.71%> (-1.65%)`	⬇️

... and 1 file with indirect coverage changes

trentm · 2023-09-22T16:36:47Z

regarding the old eslint - this seems to be known issue since a while see here.

@Flarna Oh. Thanks for chasing that down! I hadn't made the connection that this had to have been a "dependencies" entry. I'll see if I can get that fixed upstream.

trentm · 2023-09-22T19:29:22Z

I'll see if I can get that fixed upstream.

Fixed. parse-env-string@1.0.1 is published. Now if I:

$ cd plugins/node/opentelemetry-instrumentation-ioredis
$ npm update
$ npm ls parse-env-string  # verify it is at version 1.0.1

then npm run lint now works. \o/

blumamir

overall LGTM.
I never worked with ESM before, so would appreciate another review from someone with more knowledge (maybe @pichlermarc ?)

Is module[Symbol.toStringTag] === 'Module' the canonical way to test for ESM vs CJS?

plugins/node/opentelemetry-instrumentation-ioredis/src/instrumentation.ts

trentm · 2023-09-25T17:51:42Z

Is module[Symbol.toStringTag] === 'Module' the canonical way to test for ESM vs CJS?

I'm not sure about it being canonical.

I touched briefly on this in an "Aside" section of this long comment: #1692 (comment)
Because the comment is long, it is easy to not see it.

By ECMAScript spec (https://tc39.es/ecma262/#sec-module-namespace-objects) the Module Namespace Object returned from an ESM import has a well-known symbol @@toStringTag value of "Module":

The initial value of the @@toStringTag property is the String value "Module".

That @@toStringTag symbol is represented by Symbol.toStringTag. This is what makes Object.prototype.toString.call() on a Number or Date include that name:

> var d = new Date()
undefined
> Object.prototype.toString.call(d)
'[object Date]'

It is spoofable:

% cat foobar.js
class FooBar {
  get [Symbol.toStringTag]() {
    return 'Module';
  }
}
module.exports = new FooBar();

% node
> var mod = require('./foobar.js')
> mod[Symbol.toStringTag]
'Module'

I'm not sure if we need to worry about that kind of spoofing -- accidental or intentional.

Another way to differentiate would be to pass in a thisIsESMFromImportInTheMiddle boolean from where the hookFn is setup for RITM vs IITM: https://github.com/open-telemetry/opentelemetry-js/blob/f2fc0d8787a82f33b1ce7e315759ddc61caf4f52/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts#L244-L275 (I'm not suggesting that is the actual variable name. :)

(I'm not positive of the longevity of doing this. If I'm understanding recent changes in Node.js core (https://nodejs.org/en/blog/release/v20.6.0#new-nodemodule-api-register-for-module-customization-hooks-new-initialize-hook) we may eventually just have a single hook on a single Node.js core loader that handles loading both CommonJS and ESM.)

pichlermarc · 2023-10-12T14:32:03Z

I'm not sure if we need to worry about that kind of spoofing -- accidental or intentional.

The decision from the SIG meeting: we have come to the conclusion that this being spoofed does not need to be considered at this point in time. We also came to the conclusion that this seems like the best way to test for ESM vs CJS.

pichlermarc · 2023-10-12T14:59:18Z

1. unit tests

I haven't added any unit tests for this. Adding a unit test for ESM is a little bit of work. I'd appreciate thoughts/suggestions from others. Options:
1. Skip unit tests for this. ;)

🙂

2. Have ioredis.test.ts spawn a separate process that runs a small ioredis-using .mjs file with the `node --experimental-loader=...` argument.

Do you mean something like

import {IORedisInstrumentation} from "@opentelemetry/instrumentation-ioredis";
new IORedisInstrumentation()
import 'ioredis'
setTimeout(() => 0, 1000)

I think this could work as a start.

3. Add a separate `npm run test:esm` similar to here: [open-telemetry/opentelemetry-js@`f2fc0d8`/experimental/packages/opentelemetry-instrumentation/package.json#L51-L53](https://github.com/open-telemetry/opentelemetry-js/blob/f2fc0d8787a82f33b1ce7e315759ddc61caf4f52/experimental/packages/opentelemetry-instrumentation/package.json#L51-L53)

That would also work, yes. I think the tests in @opentelemetry/instrumentation are completely separate ones so they don't test all the same things the CJS tests test do. But I think a simple test like this or the one from 2. would work to get this fix out, then we can think of how to do this in a broader scope in #1731

pichlermarc

Overall this looks good to me, thank you for taking care of this @trentm 🙂
For testing, if possible, I think I'd prefer the options 2 or 3 that you mentioned;

However, I'd also be okay to skip them for now to get the fix out and then we come up with a general solution in #1731

…ests Running scripts out-of-process for testing allows running tests with different node options and with isolation. The former is useful for ESM testing. This change includes adding an ESM test for ioredis. This depends on open-telemetry#1694 to pass. Closes: open-telemetry#1731

…sing

trentm · 2023-10-13T03:06:37Z

However, I'd also be okay to skip them for now to get the fix out and then we come up with a general solution in #1731

Thanks for the review! Let's discuss on #1731 and #1735 -- my stab at a utility to do option 2.

The README example says the Hook callback `exported` arg is "effectively `import * as exported from ${url}`". https://tc39.es/ecma262/#sec-module-namespace-objects specs that a Module Namespace Object has a `@@toStringTag` property with value "Module" and no constructor. Fixes: #57 Obsoletes: #64 * * * This behaviour changed with the changes in #43 when the `register(...)`'d namespace changed from using an actual imported module object to using a plain object with module properties copied over to it: https://github.com/DataDog/import-in-the-middle/pull/43/files#diff-e69a24a4c3746fa1ee96a78e12bb12d2dd4eb6e4cacbced2bf1f4084952681d9L130-R208 I suspect that was an unintentional change. The main question here is **whether you would like import-in-the-middle to promise this**: that the `exported` namespace returned to the `Hook` callback mimics this aspect of `import * as exported from '...'`. As links to #57 show, this would help the OpenTelemetry JS project. I [started](open-telemetry/opentelemetry-js-contrib#1694 (comment)) using `exported[Symbol.toStringTag]` in OpenTelemetry instrumentations a while back as a way to handle differences in instrumentating a module based on whether it was being used from ESM code vs CommonJS code. This is convenient because **OpenTelemetry core instrumentation code uses the same hook function for require-in-the-middle and import-in-the-middle hooks**. It also seemed reasonable given the `Module Namespace Object` spec entry. However, I grant that the `exported` arg need not be a Module Namespace Object. * * * Assume you are willing to accept this, a note on my implementation: I chose to explicitly add the `@@toStringTag` property because: - it is more explicit - the `for (const k of Object.getOwnPropertySymbols(primary)) { ... }` alternative (proposed in #57 and #64) will only ever include the `@@toStringTag`. Assuming my read of the https://tc39.es/ecma262/#sec-module-namespace-objects and https://tc39.es/ecma262/#sec-module-namespace-exotic-objects sections is correct, the module object will only ever have *string* keys (for the "export"s), plus the one `@@toStringTag` property. - the `@@toStringTag` property should not be enumerable (i.e. `Object.getOwnPropertyDescriptor(exported, Symbol.toStringTag).enumerable === false`). The other implementation does not persist that descriptor value.

github-actions bot assigned blumamir and naseemkullah Sep 21, 2023

github-actions bot requested review from blumamir and naseemkullah September 21, 2023 20:22

eslint style fix

7bff58b

trentm marked this pull request as ready for review September 21, 2023 21:34

trentm requested a review from a team September 21, 2023 21:34

trentm mentioned this pull request Sep 21, 2023

REQUEST: New membership for trentm open-telemetry/community#1689

Closed

6 tasks

trentm mentioned this pull request Sep 22, 2023

chore: move dev deps to devDependencies watson/parse-env-string#2

Merged

blumamir approved these changes Sep 24, 2023

View reviewed changes

blumamir reviewed Sep 25, 2023

View reviewed changes

plugins/node/opentelemetry-instrumentation-ioredis/src/instrumentation.ts Show resolved Hide resolved

pichlermarc added the pkg:instrumentation-ioredis label Sep 25, 2023

Flarna mentioned this pull request Sep 25, 2023

pg: cannot instrument an ESM-imported pg #1693

Closed

trentm mentioned this pull request Sep 27, 2023

fix(pg): fix instrumentation of ESM-imported pg #1701

Merged

2 tasks

jacobwood091 mentioned this pull request Sep 27, 2023

Pino instrumentation fails when using ES modules #1587

Closed

trentm added 3 commits September 27, 2023 10:16

Merge branch 'main' into trentm/issue1692-ioredis-esm

faea55c

Merge branch 'main' into trentm/issue1692-ioredis-esm

fa28a93

Merge branch 'main' into trentm/issue1692-ioredis-esm

cc31725

pichlermarc added bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies labels Oct 12, 2023

pichlermarc approved these changes Oct 12, 2023

View reviewed changes

trentm mentioned this pull request Oct 13, 2023

feat(test-utils): runTestFixture utility for running out-of-process tests #1735

Merged

1 task

trentm added a commit to trentm/opentelemetry-js-contrib that referenced this pull request Oct 13, 2023

add the fix from open-telemetry#1694 to show the ioredis ESM test pas…

5eb218b

…sing

chentsulin mentioned this pull request Oct 13, 2023

fix(koa): fix instrumentation of ESM-imported koa #1736

Merged

Merge branch 'main' into trentm/issue1692-ioredis-esm

f6e6414

pichlermarc merged commit 7b457cd into open-telemetry:main Oct 17, 2023
11 checks passed

dyladan mentioned this pull request Oct 17, 2023

chore: release main #1742

Merged

trentm deleted the trentm/issue1692-ioredis-esm branch October 17, 2023 14:08

david-luna mentioned this pull request Nov 6, 2023

Cannot get HTTP Context Propagation to work open-telemetry/opentelemetry-js#4247

Closed

trentm mentioned this pull request Feb 15, 2024

coordinate first pass at ESM support for all instrumentations #1942

Open

37 tasks

trentm mentioned this pull request Mar 28, 2024

fix: ensure the hooked module exports has @@toStringTag property nodejs/import-in-the-middle#66

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ioredis): fix instrumentation of ESM-imported ioredis #1694

fix(ioredis): fix instrumentation of ESM-imported ioredis #1694

trentm commented Sep 21, 2023 •

edited

Loading

trentm commented Sep 21, 2023 •

edited

Loading

trentm commented Sep 21, 2023

Flarna commented Sep 22, 2023

codecov bot commented Sep 22, 2023 •

edited

Loading

trentm commented Sep 22, 2023

trentm commented Sep 22, 2023

blumamir left a comment

trentm commented Sep 25, 2023

pichlermarc commented Oct 12, 2023

pichlermarc commented Oct 12, 2023

1. unit tests

pichlermarc left a comment

trentm commented Oct 13, 2023

fix(ioredis): fix instrumentation of ESM-imported ioredis #1694

fix(ioredis): fix instrumentation of ESM-imported ioredis #1694

Conversation

trentm commented Sep 21, 2023 • edited Loading

Checklist

trentm commented Sep 21, 2023 • edited Loading

1. unit tests

2. lint

trentm commented Sep 21, 2023

Flarna commented Sep 22, 2023

codecov bot commented Sep 22, 2023 • edited Loading

Codecov Report

trentm commented Sep 22, 2023

trentm commented Sep 22, 2023

blumamir left a comment

Choose a reason for hiding this comment

trentm commented Sep 25, 2023

pichlermarc commented Oct 12, 2023

pichlermarc commented Oct 12, 2023

1. unit tests

pichlermarc left a comment

Choose a reason for hiding this comment

trentm commented Oct 13, 2023

trentm commented Sep 21, 2023 •

edited

Loading

trentm commented Sep 21, 2023 •

edited

Loading

codecov bot commented Sep 22, 2023 •

edited

Loading