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

Problem with grpc instrumentation in v0.41.2 #4053

Closed
mydea opened this issue Aug 10, 2023 · 7 comments
Closed

Problem with grpc instrumentation in v0.41.2 #4053

mydea opened this issue Aug 10, 2023 · 7 comments
Labels
bug Something isn't working information-requested Bug is waiting on additional information from the user priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect

Comments

@mydea
Copy link
Contributor

mydea commented Aug 10, 2023

What happened?

Steps to Reproduce

If you comment the line:

// const grpcInstrumentation = new GrpcInstrumentation();

And then run npm run start, you'll see the log @opentelemetry/instrumentation-http http instrumentation incomingRequest

Expected Result

Should work with grpc instrumentation

Actual Result

Does not work with grpc instrumentation

Additional Details

I've done some digging, and it seems to me to work if I move this (from built output, but you get the idea):

class GrpcInstrumentation {
    constructor(config) {
        this.instrumentationName = '@opentelemetry/instrumentation-grpc';
        this.instrumentationVersion = version_1.VERSION;
        // --> THIS LINE BELOW
        this._grpcJsInstrumentation = new grpc_js_1.GrpcJsInstrumentation(this.instrumentationName, this.instrumentationVersion, config);
    }

into init:

 constructor(config) {
        this.instrumentationName = '@opentelemetry/instrumentation-grpc';
        this.instrumentationVersion = version_1.VERSION;
    }

    init() {
        this._grpcJsInstrumentation = new grpc_js_1.GrpcJsInstrumentation(this.instrumentationName, this.instrumentationVersion, config);
    }

Not sure if that has other implications, though...

OpenTelemetry Setup Code

const { GrpcInstrumentation } = require("@opentelemetry/instrumentation-grpc");

const grpcInstrumentation = new GrpcInstrumentation();

package.json

{
  "name": "sentry-repro",
  "main": "app.js",
  "scripts": {
    "start": "node --require ./tracing.js app.js"
  },
  "dependencies": {
    "@opentelemetry/api": "1.4.1",
    "@opentelemetry/core": "1.15.2",
    "@opentelemetry/exporter-trace-otlp-grpc": "0.41.2",
    "@opentelemetry/instrumentation-express": "0.33.0",
    "@opentelemetry/instrumentation-grpc": "0.41.2",
    "@opentelemetry/instrumentation-http": "0.41.2",
    "@opentelemetry/sdk-node": "0.41.2",
    "@opentelemetry/semantic-conventions": "1.15.2",
    "@sentry/node": "7.62.0",
    "@sentry/opentelemetry-node": "7.62.0",
    "@sentry/profiling-node": "1.1.2",
    "express": "^4.18.2"
  }
}

Relevant log output

Expected:
@opentelemetry/instrumentation-http http instrumentation incomingRequest
Triggered route
@opentelemetry/instrumentation-http http instrumentation incomingRequest
Triggered route

Actual:
Triggered route
Triggered route
@pichlermarc
Copy link
Member

Hi @mydea, thanks for opening this issue:

Would you mind trying to instantiate the GrpcInstrumentation after the HttpInstrumentation? It may be that http has already been loaded by @grpc/grpc-js when the instrumentation was instantiated and therefore wasn't wrapped correctly.

Moving it to init() would then work as it defers that to the next tick: that's when the HttpInstrumentation has already wrapped the http module. 🤔

@mydea
Copy link
Contributor Author

mydea commented Aug 10, 2023

OK, I see the problem now I think, it happens if you instantiate the GRPC instrumentation before you setup Otel. This works:

const sdk = new opentelemetry.NodeSDK({
  traceExporter: new OTLPTraceExporter(),
  //traceExporter: new ConsoleSpanExporter(),
  instrumentations: [
    new GrpcInstrumentation(),
    new HttpInstrumentation(),
    new ExpressInstrumentation(),
  ],

This doesn't:

const grpc = new GrpcInstrumentation();

const sdk = new opentelemetry.NodeSDK({
  traceExporter: new OTLPTraceExporter(),
  //traceExporter: new ConsoleSpanExporter(),
  instrumentations: [
    grpc,
    new HttpInstrumentation(),
    new ExpressInstrumentation(),
  ],

I wonder, is there a reason why the initialization happens in the constructor, not in init? Most instrumentations I've seen seem to do stuff rather in init?

@dyladan
Copy link
Member

dyladan commented Aug 16, 2023

Most likely it is just a historical thing. The gRPC instrumentation actually predates the init function and at the time the constructor was the only option.

@dyladan
Copy link
Member

dyladan commented Aug 23, 2023

Is this resolved?

@pichlermarc pichlermarc added the information-requested Bug is waiting on additional information from the user label Aug 30, 2023
@priyanshu-gautam
Copy link

priyanshu-gautam commented Jan 10, 2024

Any update here? Facing the same issue, http instrumentation seems to be working, but not able to see any grpc data.
Code for ref :

import { getNodeAutoInstrumentations } from '@opentelemetry/auto-instrumentations-node';
const instrumentations = getNodeAutoInstrumentations();
import { NodeSDK } from '@opentelemetry/sdk-node';
import { Resource } from '@opentelemetry/resources';
import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-grpc';
import { diag, DiagConsoleLogger, DiagLogLevel } from '@opentelemetry/api';

const ddAgentHost = process.env.DD_AGENT_HOST;
const ddAgentTraceReceiverPort = process.env.DATADOG_AGENT_TRACE_RECEIVER_PORT_GRPC;
const serviceName = process.env.DD_SERVICE;

const opentelemetryTracer = async () => {
  try {
    diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.DEBUG);
    const sdk = new NodeSDK({
      resource: new Resource({
        serviceName: serviceName,
      }),
      traceExporter: new OTLPTraceExporter({
        url: `http://localhost:4317`,
      }),
      instrumentations: instrumentations
    });

    sdk.start();
    console.log('Tracing started successfully');

    process.on('SIGTERM', async () => {
      try {
        await sdk.shutdown();
        console.log('Tracing terminated');
      } catch (error) {
        console.error('Error terminating tracing:', error);
      } finally {
        process.exit(0);
      }
    });
  } catch (error) {
    console.error('Error starting OpenTelemetry SDK:', error);
    process.exit(1);
  }
};

export default opentelemetryTracer;

@pichlermarc pichlermarc added priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect and removed triage labels Jan 17, 2024
@pichlermarc
Copy link
Member

@priyanshu-gautam I think the bug you're describing is actually #4151, where the gRPC exporter requires @grpc/gprc-js before the instrumentation is loaded - but IIUC that's different to what @mydea is describing.

I was planning for the work on the exporter rewrite to be on track sooner which also aims to fix that bug, but it looks like it'll take a while until the rewrite is a viable solution.

I'll pick up work on #4151 ASAP as we've had a lot of people that are in the same boat with this issue and I don't think we should wait. With a manual setup, there was actually a workaround of changing the import order and enabling instrumentations early, but when using NodeSDK there is no way to do that as instrumentations are enabled by NodeSDK itself, which also needs the exporter added in the constructor.

@mydea
Copy link
Contributor Author

mydea commented Jul 29, 2024

I think this was fixed, so closing this! thank you 🙏

@mydea mydea closed this as completed Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working information-requested Bug is waiting on additional information from the user priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
Development

No branches or pull requests

4 participants