update nodejs autoinstrumetation, fixes #2626

[ElfoLiNk]

Description:

Update nodejs autoinstrumentation like register.ts from auto-instrumentations-node metapackage

Link to tracking Issue(s):

• Resolves: Node.js resource detectors can't be disabled with OTEL_NODE_RESOURCE_DETECTORS #2626

Testing:

Not tested yet

Documentation:

[swiatekm]

@open-telemetry/javascript-approvers could you have a look? 🙏

[ElfoLiNk]

Thank you @pavolloffay i fixed the build since getResourceDetectorsFromEnv is exported as getResourceDetectors

[pichlermarc]

Actually, I think since metrics auto-configuration is now implemented, you may not even need the autoconfigure.ts file anymore. It just duplicates what's happening in @opentelemetry/auto-instrumentations-node/register now (the only difference being that the implementation from the upstream package does not throw when a non-existing exporter is selected)

[ElfoLiNk]

Hi @pichlermarc, are you suggesting to remove autoinstrumentation.ts here and update the code in nodejs.go to something like:

nodeRequireArgument = " --require /otel-auto-instrumentation-nodejs/node_modules/@opentelemetry/auto-instrumentations-node/build/src/register.js"

Would this approach work when referenced like this? I haven’t tested it yet.

Thank you

PS @swiatekm, what’s your opinion on this?

[iblancasa]

Hi @pichlermarc, are you suggesting to remove autoinstrumentation.ts here and update the code in nodejs.go to something like:

nodeRequireArgument = " --require /otel-auto-instrumentation-nodejs/node_modules/@opentelemetry/auto-instrumentations-node/build/src/register.js"

Would this approach work when referenced like this? I haven’t tested it yet.

Thank you

PS @swiatekm, what’s your opinion on this?
It makes sense to me

[pichlermarc]

Hi @pichlermarc, are you suggesting to remove autoinstrumentation.ts here and update the code in nodejs.go to something like:

nodeRequireArgument = " --require /otel-auto-instrumentation-nodejs/node_modules/@opentelemetry/auto-instrumentations-node/build/src/register.js"

Something along these lines yes. I don't think you necessarily need to reference the full path - you may be able to just ref the entrypoint:

nodeRequireArgument = " --require /otel-auto-instrumentation-nodejs/node_modules/@opentelemetry/auto-instrumentations-node/register"

[swiatekm]

@pavolloffay @pichlermarc could you re-review this (I assume) final state of the change?

@ElfoLiNk have you tested this with a real Node application? Our tests only check if the instrumented application starts, but not if it's actually emitting the right telemetry, so this should be verified manually with this kind of substantial change.

[pichlermarc]

Would this work? That's the entrypoint we provide for these kinds of things.

Suggested change

	nodeRequireArgument = " --require /otel-auto-instrumentation-nodejs/node_modules/@opentelemetry/auto-instrumentations-node/build/src/register.js"
	nodeRequireArgument = " --require /otel-auto-instrumentation-nodejs/node_modules/@opentelemetry/auto-instrumentations-node/register"

[ElfoLiNk]

I tried but i got

Error: Cannot find module '/otel-auto-instrumentation-nodejs/node_modules/@opentelemetry/auto-instrumentations-node/register'

[ElfoLiNk]

@pavolloffay @pichlermarc could you re-review this (I assume) final state of the change?

@ElfoLiNk have you tested this with a real Node application? Our tests only check if the instrumented application starts, but not if it's actually emitting the right telemetry, so this should be verified manually with this kind of substantial change.

I tested today, and everything seems to be working:

OTEL_LOGS_EXPORTER is empty. Using default otlp exporter.
Unsupported OTLP metrics protocol: "undefined". Using http/protobuf.
OpenTelemetry automatic instrumentation started successfully

I was testing it with this configuration:

- name: OTEL_TRACES_EXPORTER
  value: "otlp"
- name: OTEL_METRICS_EXPORTER
   value: "otlp"
- name: OTEL_NODE_RESOURCE_DETECTORS
   value: "env,os,process,container"

The exporter endpoint was set to port 4317.

The only issue I encountered was with the metrics exporter:

Error: PeriodicExportingMetricReader: metrics export failed (error Error: Parse Error: Expected HTTP/)

To resolve this, I explicitly added OTEL_EXPORTER_OTLP_PROTOCOL to the instrumentation configuration, which resolved the issue.

Additionally, both the operator and instrumentation Docker images need to be updated simultaneously. If you update only the instrumentation, you encounter the following error:

Error: Cannot find module '/otel-auto-instrumentation-nodejs/autoinstrumentation.js'

[pichlermarc]

Looks good to me - @ElfoLiNk keep in mind that I only maintain the upstream OTel JS packages, I'm not a maintainer of this repo and I only have limited knowledge about the operator. The decision whether to merge this or not is fully in the hands of the operator maintainers.

[swiatekm]

The changes look good to me, but the following:

Additionally, both the operator and instrumentation Docker images need to be updated simultaneously. If you update only the instrumentation, you encounter the following error:

is a significant problem. We'd be breaking existing installations in a major way if the user sets their own nodejs instrumentation image. We might need a feature flag if we can't support both versions.

@ElfoLiNk could we not support both older and newer images by keeping the NODE_OPTIONS as they are and symlinking /otel-auto-instrumentation-nodejs/node_modules/@opentelemetry/auto-instrumentations-node/build/src/register.js to /otel-auto-instrumentation-nodejs/autoinstrumentation.js? Then we could add a feature flag to use the new path and enable it on a more relaxed schedule, without needing to change the image and operator code in sync.

[ElfoLiNk]

Hi @swiatekm i added in the Dockerfile the old reference seems working

[swiatekm]

@ElfoLiNk we also need to make the new operator version compatible with older autoinstrumentation images. That would involve reverting your changes to nodejs.go for now, and submitting a separate PR introducing a feature flag that will change the instrumentation require path. Does that make sense?

[ElfoLiNk]

@swiatekm looks good now?

[swiatekm]

Thank you for the contribution, and your patience with the review process, @ElfoLiNk! I think this change is ready to be merged, but I'd like to wait for 0.119.0 to release first.