opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

Experimental grpc plugin async onInit conflicts with testing norms

Open cdaringe opened this issue 2 years ago • 6 comments
trafficstars

What happened?

Steps to Reproduce

  • create a jest test that sets up instrumentation using opentelemetry/otlp-grpc-exporter-base
  • ensure that the test does very little else

e.g.

describe("my server", () => {
  it("should  boot", async () => {
     const server = await startServerThatHasInstrumentation();
     expect(server.state.foobar).toBe('baz');
  });
});
  • run it! observe:
ReferenceError: You are trying to `import` a file after the Jest environment has been torn down. From src/index.spec.ts.

/<path>/node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@opentelemetry/otlp-grpc-exporter-base/build/src/OTLPGRPCExporterNodeBase.js:59
            onInit(this, config);

Expected Result

No error.

Actual Result

Error.

Additional Details

onInit is modeled as a sync function--however, it is certainly async. it's worth considering moving the that dyn require to a dyn import and tracking the control flow versus onInit being purely a side effect.

in other words:

onInit: () => void => onInit: () => Promise<void>, and changing the upstream callee patterns as needed.

Otherwise, in all of our integration tests, we must mock out a transitive dependency (the OT libs) just to not crash the test runner.

OpenTelemetry Setup Code

import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-grpc';
import { diag, DiagConsoleLogger } from '@opentelemetry/api';

export async function setup() {
  const provider = new BasicTracerProvider({ resource });
  // snip
  const exporter = new OTLPTraceExporter({ url: input.collectorUrl });
  diag.setLogger(new DiagConsoleLogger());
  provider.addSpanProcessor(new BatchSpanProcessor(setupTracingSpanExporter(exporter)));
  // snip
}

package.json

{
  "name": "my-tracing-bundle-lib",
  "version": "40.0.2",
  "description": "canned tracing",
  "main": "dist/index.js",
  "types": "dist/index.d.ts",
  "scripts": {
    "test": "jest --ci"
  },
  "files": [
    "dist",
    "!**/__tests__",
    "!**/__snapshots__",
    "!**/.tsbuildinfo",
    "!**/*.spec.**",
    "!**/*.spec.**.**"
  ],
  "dependencies": {
    "@opentelemetry/api": "1.6.0",
    "@opentelemetry/core": "1.17.1",
    "@opentelemetry/exporter-trace-otlp-grpc": "0.44.0",
    "@opentelemetry/resources": "1.17.1",
    "@opentelemetry/sdk-trace-base": "1.17.1",
    "@opentelemetry/semantic-conventions": "1.17.1",
    "graphql": "16.6.0"
  },
  "devDependencies": {
    "@walmart/reme-client": "^5.1.9",
    "fastify": "^4.17.0"
  }
}

Relevant log output

ReferenceError: You are trying to `import` a file after the Jest environment has been torn down. From src/index.spec.ts.

/Users/c0d01a5/src/orchestra-sidecar/node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@opentelemetry/otlp-grpc-exporter-base/build/src/OTLPGRPCExporterNodeBase.js:59
            onInit(this, config);

cdaringe avatar Nov 08 '23 17:11 cdaringe

Also seeing this. Let me know if I can provide more info! Everything I would say appears to be captured in the parent comment.

beggers avatar Nov 29 '23 00:11 beggers

Possibly related: some of my (fully unrelated) unit tests are failing with the following message:

/Users/beggers/chroma/hosted-chroma/chroma-dashboard/backend/node_modules/@opentelemetry/otlp-grpc-exporter-base/build/src/OTLPGRPCExporterNodeBase.js:59
            onInit(this, config);
            ^ 
 
TypeError: onInit is not a function
    at Immediate.<anonymous> (/Users/beggers/chroma/hosted-chroma/chroma-dashboard/backend/node_modules/@opentelemetry/otlp-grpc-exporter-base/src/OTLPGRPCExporterNodeBase.ts:87:7)         
    at processImmediate (node:internal/timers:478:21)
    at process.callbackTrampoline (node:internal/async_hooks:130:17) 
 
Node.js v21.0.0                                                      

beggers avatar Nov 29 '23 18:11 beggers

I stand corrected, the above message was likely caused by a change in how my team ran tests. Disregard.

beggers avatar Nov 29 '23 18:11 beggers

TypeError: onInit is not a function

ya, get that all of the time. same general issue as discussed in the root report

cdaringe avatar Nov 29 '23 23:11 cdaringe

is there any fix? What I found was to mock opentelemetry in all the test cases. But that is not the feasible solution.

charandazn avatar Jan 02 '24 08:01 charandazn

is there any fix? What I found was to mock opentelemetry in all the test cases. But that is not the feasible solution.

@charandazn #4432 should include a fix for this. It loads @grpc/grpc-js on the first export instead of on the next tick.

pichlermarc avatar Feb 13 '24 08:02 pichlermarc