open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.74k stars 805 forks source link

Experimental grpc plugin async onInit conflicts with testing norms #4266

Closed cdaringe closed 8 months ago

cdaringe commented 1 year ago

What happened?

Steps to Reproduce

e.g.

describe("my server", () => {
  it("should  boot", async () => {
     const server = await startServerThatHasInstrumentation();
     expect(server.state.foobar).toBe('baz');
  });
});
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/@opentelemetry+otlp-grpc-exporter-base@0.44.0_@opentelemetry+api@1.6.0/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/@opentelemetry+otlp-grpc-exporter-base@0.44.0_@opentelemetry+api@1.6.0/node_modules/@opentelemetry/otlp-grpc-exporter-base/build/src/OTLPGRPCExporterNodeBase.js:59
            onInit(this, config);
beggers commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

cdaringe commented 11 months ago

TypeError: onInit is not a function

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

charandazn commented 10 months ago

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

pichlermarc commented 9 months ago

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.