Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental grpc plugin async onInit conflicts with testing norms #4266

Closed
cdaringe opened this issue Nov 8, 2023 · 6 comments · Fixed by #4432
Closed

Experimental grpc plugin async onInit conflicts with testing norms #4266

cdaringe opened this issue Nov 8, 2023 · 6 comments · Fixed by #4432
Assignees
Labels
bug Something isn't working pkg:otlp-grpc-exporter-base priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc

Comments

@cdaringe
Copy link

cdaringe commented Nov 8, 2023

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/@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);
@cdaringe cdaringe added bug Something isn't working triage labels Nov 8, 2023
@pichlermarc pichlermarc added priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc pkg:otlp-grpc-exporter-base and removed triage labels Nov 15, 2023
@pichlermarc pichlermarc self-assigned this Nov 15, 2023
@beggers
Copy link

beggers commented Nov 29, 2023

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
Copy link

beggers commented Nov 29, 2023

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
Copy link

beggers commented Nov 29, 2023

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

@cdaringe
Copy link
Author

cdaringe commented Nov 29, 2023

TypeError: onInit is not a function

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

@charandazn
Copy link

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

@pichlermarc
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:otlp-grpc-exporter-base priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants