Cannot read properties of undefined. Reading OTEL_SDK_DISABLED

[ravindra-dyte]

What happened?

Users integrating our SDK (that internally uses Opentelemetry SDK) fails to initialize with the following error

Cannot read properties of undefined. Reading OTEL_SDK_DISABLED

Steps to Reproduce

We are an SDK company, and we have integrated Opentelemetry in our SDK. Clients of ours who are having bundler configs, such as webpack config, where node integration's globals are set to false (process in this case), not undefined but false, face this issue.

This is happening because

opentelemetry-js/packages/opentelemetry-core/src/utils/environment.ts

Line 336 in 47444f2

return typeof process !== 'undefined'

checks for undefined only whereas the value is false for them therefore it passes the check but fails afterward when it tries to pick environment variables from process.env. Because the process is false, process.env would be undefined and any key retrieval on process.env would throw this above error.

Expected Result

The default configuration should be picked.
A falsy check should be there to ensure that the process.env is also not falsy.

For example, we could replace,

return typeof process !== 'undefined'
        ? parseEnvironment(process.env)
        : parseEnvironment(globalThis_1._globalThis);

with

return typeof process !== 'undefined' && process && process.env
        ? parseEnvironment(process.env)
        : parseEnvironment(globalThis_1._globalThis);

to not allow null, undefine, or any other falsy value.

Or we can remove node globals from browser-based SDKs.

Actual Result

It fails, treats false as an object, and fails with an error.

Additional Details

We could ask our clients to make the change in their bundler configs, but it is less than ideal. Also, it gives a poor first impression.

OpenTelemetry Setup Code

import { WebTracerProvider, StackContextManager } from '@opentelemetry/sdk-trace-web';
import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-http';
import { BatchSpanProcessor, Tracer } from '@opentelemetry/sdk-trace-base';
import { ZoneContextManager } from '@opentelemetry/context-zone-peer-dep';
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
import opentelemetry, {
    Span, ROOT_CONTEXT, PropagationAPI, propagation,
} from '@opentelemetry/api';
import { Resource } from '@opentelemetry/resources';        

const provider = new WebTracerProvider({ // fails here itself
    resource: new Resource({
      'service.name': 'our-service-name'
    }),
});
// Not adding the remaining code as might not be relevant

package.json

{
  "dependencies": {
    "@opentelemetry/api": "^1.4.0",
    "@opentelemetry/context-zone-peer-dep": "^1.9.0",
    "@opentelemetry/exporter-trace-otlp-http": "^0.35.0",
    "@opentelemetry/resources": "^1.9.0",
    "@opentelemetry/sdk-trace-base": "^1.9.0",
    "@opentelemetry/sdk-trace-web": "^1.9.0",
    "@opentelemetry/semantic-conventions": "^1.9.0",
  }
}

Relevant log output

No response

[Flarna]

Why do they set process to false. I think including null to the check makes sense but any falsy value? I assume this just hides a problem which likely pops up at other places then.

What if someone sets process to a string not available?

[ravindra-dyte]

Devs, when they face issues, flock to stack overflow, and end up on threads like https://stackoverflow.com/questions/64557638/how-to-polyfill-node-core-modules-in-webpack-5 where the most voted answer tells them to set it as false.

What if someone sets process to a string not available?

My suggested extra cautious null check would help in sorting this out. I would recommend using _.get method from lodash for the same instead of the below snippet.

return typeof process !== 'undefined' && process && process.env
        ? parseEnvironment(process.env)
        : parseEnvironment(globalThis_1._globalThis);

typeof process would be string. process would be this string and process.env would be undefined therefore not breaking the usual flow. If someone now puts process.env to true/random-stuff, then, anyway, not much can be done for such folks, apart from educating them about it.

What is your opinion on this?

[ravindra-dyte]

For production-grade solution, I would recommend using _.get from lodash.

[Flarna]

We tend to avoid dependencies to 3rd parties even to popular ones like lodash if possible.

If a falsy check solves problems I would vote for adding it. It's Javascript world here so types are not that strict.

Could you create a PR for this?

[ravindra-dyte]

Sure. Would raise it, this week.

[pichlermarc]

@ravindra-dyte would it be okay if I assign the issue to you? 🙂

[ravindra-dyte]

Sure @pichlermarc