test(ioredis): test ioredis ESM usage

[trentm]

This PR is not intended for merging. It is to support discussion of #1731. Maintainers can unassign themselves, and perhaps assign me.

This is an attempt to test ESM usage of ioredis based on Marc's comment at #1731 (comment)

This passes, but the thing that scares me is enabling the ESM hook for .test.ts tests as well. The IORedisInstrumentation patch method is being called both for ESM and CommonJS for ioredis.test.ts -- I don't understand that.

Refs: #1731
Refs: #1735

---

This would be an alternative to #1735.

@pichlermarc Is this close to what you tried for your ESM testing experiements? This runs and passes, however I don't grok the:

XXX IORedisInstrumentation: esm? true
XXX IORedisInstrumentation: esm? false

output for the execution of "test/ioredis.test.ts" -- for what should be just CommonJS. It would scare me a little bit to turn on NODE_OPTIONS='--loader @opentelemetry/instrumentation/hook.mjs' for all *.test.ts files. :)

A full test run with this patch:

[.../opentelemetry-js-contrib4/plugins/node/opentelemetry-instrumentation-ioredis]
% npm run test:local

> @opentelemetry/instrumentation-ioredis@0.35.2 test:local
> cross-env RUN_REDIS_TESTS_LOCAL=true npm run test


> @opentelemetry/instrumentation-ioredis@0.35.2 test
> cross-env NODE_NO_WARNINGS=1 NODE_OPTIONS='--loader @opentelemetry/instrumentation/hook.mjs' nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts' 'test/**/*.test.mjs'



  ioredis
XXX IORedisInstrumentation: esm? true
XXX IORedisInstrumentation: esm? false
    ✓ should have correct module name
    #createClient()
      ✓ should propagate the current span to event handlers
    #send_internal_message()
      Instrumenting query operations
XXX IORedisInstrumentation: esm? true
XXX IORedisInstrumentation: esm? false
        ✓ should create a child span for cb style insert
        ✓ should create a child span for cb style get
        ✓ should create a child span for hset promise
        ✓ should set span with error when redis return reject
        ✓ should create a child span for streamify scanning
        - should create a child span for pubsub
        ✓ should create a child span for multi/transaction
        ✓ should create a child span for pipeline
        ✓ should create a child span for get promise
        ✓ should create a child span for del
        ✓ should create a child span for lua
      Instrumenting without parent span
        ✓ should not create child span for query
        ✓ should not create child span for connect
      Instrumentation with requireParentSpan
        ✓ should instrument queries with requireParentSpan equal false
        - should instrument connect with requireParentSpan equal false
        ✓ should not instrument queries with requireParentSpan equal true
        ✓ should not instrument connect with requireParentSpan equal true
      Instrumenting with a custom db.statement serializer
        ✓ should tag the span with a custom db.statement for cb style insert
        ✓ should tag the span with a custom db.statement for cb style get
      Removing instrumentation
        ✓ should not create a child span for cb style insert
        ✓ should not create a child span for cb style get
        ✓ should not create a child span for hset promise upon error
      Instrumenting with a custom hooks
XXX IORedisInstrumentation: esm? true
XXX IORedisInstrumentation: esm? false
        ✓ should call requestHook when set in config
        ✓ should ignore requestHook which throws exception
        ✓ should call responseHook when set in config
        ✓ should ignore responseHook which throws exception
      setConfig - custom dbStatementSerializer config
        ✓ should properly execute the db statement serializer for operation insert
        ✓ should properly execute the db statement serializer for operation get

  ioredis ESM
XXX IORedisInstrumentation: esm? true
    ✓ should create spans


  29 passing (1s)
  2 pending

[codecov]

Codecov Report

Merging #1752 (fb9140a) into main (1dc2e81) will increase coverage by 0.47%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1752      +/-   ##
==========================================
+ Coverage   90.98%   91.46%   +0.47%     
==========================================
  Files         129      139      +10     
  Lines        6448     7172     +724     
  Branches     1296     1446     +150     
==========================================
+ Hits         5867     6560     +693     
- Misses        581      612      +31     

Files	Coverage Δ
...try-instrumentation-ioredis/src/instrumentation.ts	93.54% <100.00%> (+3.33%)	⬆️

... and 10 files with indirect coverage changes

[pichlermarc]

@pichlermarc Is this close to what you tried for your ESM testing experiements?

I actually had two different scripts

• test:cjs that does everything as usual (not registering the loader)
• test:esm which does register the loader through .mohcarc.json, a seperate tsconfig.json and "type": "module" and uses a
  • .mocharc.json

    	{
    	    "extension": ["ts"],
    	    "node-option": ["experimental-specifier-resolution=node", "loader=ts-node/esm", "experimental-loader=@opentelemetry/instrumentation/hook.mjs"],
    	    "spec": ["test/**/*.test.ts"]
    	}

  • running the tests with npx mocha

That had a few problems though:

• I had to modify the package.json by adding"type": "module" to get this to work
• I had not yet figured out how to configure ts-node to use the correct tsconfig with this approach, so I needed to modify that one too by coping the esm tsconfig first (there's probably some way, but I did not have the time to research it)
• it would not start mocha, as it would use the mohca cli, which wouldn't work with import-in-the-middle, I needed to make some manual changes to the mocha code to get it to run

TL;DR: it was not ideal to say the least, and needed a lot of modifications to the current setup to get it to work. Even then it may not have been accurate as we don't publish ESM files with our instrumentations.

Thank you for trying this out too, though. I think once tooling catches up we may be able to re-visit this approach, but right not it does not seem like a feasible solution as there are a lot of moving parts. I'll put on my list to polish what I did and then push a branch for future reference.

[pichlermarc]

I've opened a draft to show what I did in my testing: #1794
Edit: it is a bit different from what I described above as I've realized that it can be simplified a bit - I added some comments to explain what I did in the PR

[trentm]

This is no longer needed now that the alternative ESM testing option (#1735) has been merged.