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

payment service crashes with OTEL_SDK_DISABLED #2124

Open
rlankfo opened this issue Mar 15, 2025 · 4 comments
Open

payment service crashes with OTEL_SDK_DISABLED #2124

rlankfo opened this issue Mar 15, 2025 · 4 comments
Labels
bug Something isn't working

Comments

@rlankfo
Copy link
Member

rlankfo commented Mar 15, 2025

Bug Report

Which version of the demo you are using?

2.0.1

Symptom

payment service crashes when OTEL_SDK_DISABLED env var is true

What is the expected behavior?

Expected behavior is that the payment service honors this variable as described in the docs:
If “true”, a no-op SDK implementation will be used for all telemetry signals. Any other value or absence of the variable will have no effect and the SDK will remain enabled.

https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#general-sdk-configuration

What do you expect to see?

payment service runs with no-op sdk implementation.

What is the actual behavior?

payment service crashes

> start
> node --require ./opentelemetry.js index.js

{"level":"info","time":1742007177599,"pid":21,"hostname":"payment-7b7b5f6dc7-jcf5n","service.name":"payment","msg":"payment gRPC server started on port 8080"}
{"level":"warn","time":1742007215173,"pid":21,"hostname":"payment-7b7b5f6dc7-jcf5n","service.name":"payment","err":{"type":"TypeError","message":"Cannot read properties of undefined (reading 'setAttributes')","stack":"TypeError: Cannot read properties of undefined (reading 'setAttributes')\n    at Object.chargeServiceHandler [as charge] (/usr/src/app/index.js:16:10)\n    at clientStreamAndUnaryHandler (/usr/src/app/node_modules/@opentelemetry/instrumentation-grpc/build/src/serverUtils.js:97:21)\n    at Object.handleServerFunction (/usr/src/app/node_modules/@opentelemetry/instrumentation-grpc/build/src/serverUtils.js:108:20)\n    at /usr/src/app/node_modules/@opentelemetry/instrumentation-grpc/build/src/instrumentation.js:105:68\n    at NoopContextManager.with (/usr/src/app/node_modules/@opentelemetry/api/build/src/context/NoopContextManager.js:25:19)\n    at ContextAPI.with (/usr/src/app/node_modules/@opentelemetry/api/build/src/api/context.js:60:46)\n    at /usr/src/app/node_modules/@opentelemetry/instrumentation-grpc/build/src/instrumentation.js:104:47\n    at NoopContextManager.with (/usr/src/app/node_modules/@opentelemetry/api/build/src/context/NoopContextManager.js:25:19)\n    at ContextAPI.with (/usr/src/app/node_modules/@opentelemetry/api/build/src/api/context.js:60:46)\n    at Object.func (/usr/src/app/node_modules/@opentelemetry/instrumentation-grpc/build/src/instrumentation.js:87:43)"},"msg":"Cannot read properties of undefined (reading 'setAttributes')"}
/usr/src/app/index.js:27
    span.recordException(err)
         ^

TypeError: Cannot read properties of undefined (reading 'recordException')
    at Object.chargeServiceHandler [as charge] (/usr/src/app/index.js:27:10)
    at clientStreamAndUnaryHandler (/usr/src/app/node_modules/@opentelemetry/instrumentation-grpc/build/src/serverUtils.js:97:21)
    at Object.handleServerFunction (/usr/src/app/node_modules/@opentelemetry/instrumentation-grpc/build/src/serverUtils.js:108:20)
    at /usr/src/app/node_modules/@opentelemetry/instrumentation-grpc/build/src/instrumentation.js:105:68
    at NoopContextManager.with (/usr/src/app/node_modules/@opentelemetry/api/build/src/context/NoopContextManager.js:25:19)
    at ContextAPI.with (/usr/src/app/node_modules/@opentelemetry/api/build/src/api/context.js:60:46)
    at /usr/src/app/node_modules/@opentelemetry/instrumentation-grpc/build/src/instrumentation.js:104:47
    at NoopContextManager.with (/usr/src/app/node_modules/@opentelemetry/api/build/src/context/NoopContextManager.js:25:19)
    at ContextAPI.with (/usr/src/app/node_modules/@opentelemetry/api/build/src/api/context.js:60:46)
    at Object.func (/usr/src/app/node_modules/@opentelemetry/instrumentation-grpc/build/src/instrumentation.js:87:43)

Node.js v22.14.0
npm notice
npm notice New major version of npm available! 10.9.2 -> 11.2.0
npm notice Changelog: https://github.com/npm/cli/releases/tag/v11.2.0
npm notice To update run: npm install -g npm@11.2.0
npm notice
stream closed EOF for otel-demo/payment-7b7b5f6dc7-jcf5n (payment)

Please describe the actual behavior experienced.

Reproduce

Could you provide the minimum required steps to resolve the issue you're seeing?

To reproduce, set OTEL_SDK_DISABLED env var to "true".

I set this for all services in my values.yaml like so:

  values:
    default:
      env:
        - name: OTEL_SERVICE_NAME
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: "metadata.labels['app.kubernetes.io/component']"
        - name: OTEL_COLLECTOR_NAME
          value: otelcol
        - name: OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE
          value: cumulative
        - name: OTEL_SDK_DISABLED
          value: "true"
        - name: OTEL_RESOURCE_ATTRIBUTES
          value: 'service.name=$(OTEL_SERVICE_NAME),service.namespace=opentelemetry-demo,service.version={{ .Chart.AppVersion }}'
@rlankfo rlankfo added the bug Something isn't working label Mar 15, 2025
@julianocosta89
Copy link
Member

I wonder if that is a Demo issue, or an SDK issue.
@open-telemetry/javascript-approvers what do you think?

@pichlermarc
Copy link
Member

@julianocosta89 thanks for reaching out.

This is a Demo issue, I think.
On this line the return value of opentelemetry.trace.getActiveSpan() may be undefined.

See also https://open-telemetry.github.io/opentelemetry-js/classes/_opentelemetry_api.TraceAPI.html#getActiveSpan

@julianocosta89
Copy link
Member

julianocosta89 commented Mar 17, 2025

Thanks for the prompt reply @pichlermarc 🤩 !

That made me wonder, should this env var affect the usage of the API?

I've done a quick test to see how Java was behaving and my expectation was met.
I've added the following to the ad env vars on docker-compose.yml file:

- OTEL_SDK_DISABLED=true

Started the demo with make start.
When checking the logs for ad I could see everything working fine:

2025-03-17 09:35:55 - oteldemo.AdService - Non-targeted ad request received, preparing random response. trace_id=b0ddb683f4734e32a9f8757d6f3094a8 span_id=7b599b9ccc4afb54 trace_flags=01 
2025-03-17 09:36:12 - oteldemo.AdService - Targeted ad request received for [accessories] trace_id=d0fb8524507c88f59d7b07c5efc0e0c7 span_id=51bca5c57cf36ac2 trace_flags=01 
2025-03-17 09:36:25 - oteldemo.AdService - Non-targeted ad request received, preparing random response. trace_id=9c054ffccf0c43355a17fdedbcd16406 span_id=7194ec9b5e414788 trace_flags=01 
2025-03-17 09:36:35 - oteldemo.AdService - Targeted ad request received for [travel] trace_id=b833989b000444c00e41dc89c0c602ff span_id=66292d1dcad35c3d trace_flags=01

But no Spans from ad were leaving the service, because the SDK is no-op.
The service ad was not even listed on Jaeger.

@pichlermarc
Copy link
Member

That made me wonder, should this env var affect the usage of the API?

It shouldn't, you're right. But this is actually an issue that would've been caught by using TypeScript as the return type states that it can be undefined, but since there's no type system in JavaScript to catch it, this goes unnoticed. 🙂 The same can happen in cases where there is no active span (it will also return undefined).

This function is similar to Java's Span#fromContextOrNull() which may also return null when there's nothing active. (Java's function has a much better name to indicate that though). I'd assume that this function would cause something similar in Java if there was no null check on the returned Span.

Side note: these are all utility functions that are named differently across SDKs, coming from this part of the spec. Since it does not fully specify behavior, everyone kind of does something different. There's a good case for having a null/undefined response for such APIs as it allows us to skip resource heavy work for retrieving attributes if there's no active span. I was not around when this API was introduced in OTel JS, but I assume that this was the rationale for making it this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants