fix(instrumentation-dataloader): Patch batchLoadFn without creating an instance

[onurtemizkan]

Related: getsentry/sentry-javascript#13869

Which problem is this PR solving?

• The current version of the instrumentation creates an instance (and returns it) in order to patch the DataLoader constructor for batchLoadFn. This breaks the usage where the DataLoader class is extended.

Short description of the changes

• This implementation uses the first argument of the DataLoader constructor (which is batchLoadFn) to patch to avoid creating an instance.

[codecov]

Codecov Report

Attention: Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.74%. Comparing base (5eb61d8) to head (1554621).

Files with missing lines	Patch %	Lines
.../instrumentation-dataloader/src/instrumentation.ts	93.10%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2498      +/-   ##
==========================================
- Coverage   90.75%   90.74%   -0.02%     
==========================================
  Files         169      169              
  Lines        8018     8020       +2     
  Branches     1632     1633       +1     
==========================================
+ Hits         7277     7278       +1     
- Misses        741      742       +1     

Files with missing lines	Coverage Δ
.../instrumentation-dataloader/src/instrumentation.ts	98.09% <93.10%> (-0.94%)	⬇️

[trentm]

Mostly looks good, just a couple edge cases.

[trentm]

nit: I think this class and the getMd5HashFromIdx function could move inside the test case to be closer to the only place they are used:

--- a/plugins/node/instrumentation-dataloader/test/dataloader.test.ts
+++ b/plugins/node/instrumentation-dataloader/test/dataloader.test.ts
@@ -33,23 +33,10 @@ import * as assert from 'assert';
 import * as Dataloader from 'dataloader';
 import * as crypto from 'crypto';

-const getMd5HashFromIdx = (idx: number) =>
-  crypto.createHash('md5').update(String(idx)).digest('hex');
-
 describe('DataloaderInstrumentation', () => {
   let dataloader: Dataloader<string, number>;
   let contextManager: AsyncHooksContextManager;

-  class CustomDataLoader extends Dataloader<string, string> {
-    constructor() {
-      super(async keys => keys.map((_, idx) => getMd5HashFromIdx(idx)));
-    }
-
-    public async customLoad() {
-      return this.load('test');
-    }
-  }
-
   const memoryExporter = new InMemorySpanExporter();
   const provider = new NodeTracerProvider();
   const tracer = provider.getTracer('default');
@@ -350,6 +337,19 @@ describe('DataloaderInstrumentation', () => {
   });

   it('should not prune custom methods', async () => {
+    const getMd5HashFromIdx = (idx: number) =>
+      crypto.createHash('md5').update(String(idx)).digest('hex');
+
+    class CustomDataLoader extends Dataloader<string, string> {
+      constructor() {
+        super(async keys => keys.map((_, idx) => getMd5HashFromIdx(idx)));
+      }
+
+      public async customLoad() {
+        return this.load('test');
+      }
+    }
+
     const customDataloader = new CustomDataLoader();
     await customDataloader.customLoad();

[trentm]

Suggested change

	if (!instrumentation.isEnabled() || !instrumentation.shouldCreateSpans()) {
	if (!instrumentation.isEnabled()) {

This if-block is at init-time before any spans are being created. This !instrumentation.shouldCreateSpans() is only true by fluke because config.requireParentSpan is undefined in the tests.

[trentm]

There should be a guard that args[0] is actually a function before wrapping it. Something like (untested):

if (typeof args[0] !== 'function') {
  return constructor.apply(this, args);
}

or perhaps only do the unwrap and wrap if it is a function.

---

The current behaviour breaks the guard in Dataloader itself:

foo.js

if (process.env.INSTRUMENT_IT) {
  const { DataloaderInstrumentation } = require('./');
  const { NodeSDK, tracing } = require('@opentelemetry/sdk-node');
  const instr = new DataloaderInstrumentation();
  const sdk = new NodeSDK({
    spanProcessor: new tracing.SimpleSpanProcessor(new tracing.ConsoleSpanExporter()),
    serviceName: 'foo',
    instrumentations: [instr]
  });
  sdk.start();
}

const Dataloader = require('dataloader');
const dl = new Dataloader(); // oops, no batchLoadFn passed in

console.log('the end');

Run that without and with instrumentation shows a changed behaviour:

% node foo.js
/Users/trentm/tm/opentelemetry-js-contrib10/node_modules/dataloader/index.js:32
      throw new TypeError('DataLoader must be constructed with a function which accepts ' + ("Array<key> and returns Promise<Array<value>>, but got: " + batchLoadFn + "."));
      ^

TypeError: DataLoader must be constructed with a function which accepts Array<key> and returns Promise<Array<value>>, but got: undefined.
    at new DataLoader (/Users/trentm/tm/opentelemetry-js-contrib10/node_modules/dataloader/index.js:32:13)
    at Object.<anonymous> (/Users/trentm/tm/opentelemetry-js-contrib10/plugins/node/instrumentation-dataloader/foo.js:14:12)
    at Module._compile (node:internal/modules/cjs/loader:1364:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1422:10)
    at Module.load (node:internal/modules/cjs/loader:1203:32)
    at Module._load (node:internal/modules/cjs/loader:1019:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:128:12)
    at node:internal/main/run_main_module:28:49

Node.js v18.20.4

% INSTRUMENT_IT=true node foo.js
the end

Please add a test case for this.

[onurtemizkan]

I added a guard to check if batchLoadFn is a function. However, I could not reproduce the problem where Dataloader instance is created without a batchLoadFn (with or without the instrumentation).

It fails with:

TypeError: DataLoader must be constructed with a function which accepts Array<key> and returns Promise<Array<value>>, but got: undefined.
for both cases.