feat(auto-instrumentations-node): disabling instrumentations via env var

[JamieDanielson]

Which problem is this PR solving?

• Similar to feat(auto-instrumentations-node): enabling instrumentations via env #1953 this provides the alternative option discussed in some comments for disabling some instrumentations via environment variable.
• Fixes Support to disable specific library or middleware instrumentation via environment variable #1331
• Finishes fix for NodeJS auto-instrumentation: Allow disabling specific instrumentations with environment variables #1672
• Helps to fix Allow disabling nodejs auto-instrumentations via environment variables opentelemetry-operator#2622

Short description of the changes

• Disable specific instrumentations using OTEL_NODE_DISABLED_INSTRUMENTATIONS environment variable. This is similar to the use case when instrumenting in code using getNodeAutoInstrumentations() where someone may want to start with every instrumentation and pare down from there.

[codecov]

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.30%. Comparing base (dfb2dff) to head (78be697).
Report is 167 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2174      +/-   ##
==========================================
- Coverage   90.97%   90.30%   -0.68%     
==========================================
  Files         146      147       +1     
  Lines        7492     7260     -232     
  Branches     1502     1509       +7     
==========================================
- Hits         6816     6556     -260     
- Misses        676      704      +28     

Files	Coverage Δ
...tapackages/auto-instrumentations-node/src/utils.ts	98.18% <94.11%> (-0.60%)	⬇️

... and 53 files with indirect coverage changes

[david-luna]

I see that adding this env var was already commented in the linked PR here. I guess you already have the use case for this :)

[JamieDanielson]

I see that adding this env var was already commented in the linked PR here. I guess you already have the use case for this :)

I do keep getting the request for the simplest possible setup, and iterating from there. So this is similar to how many folks have gotten started with the auto-instrumentations-node package as it is. Start with everything enabled by default, and then disable one by one as you decide what you don't want, instead of trying to figure out everything you do want. Later as you understand your telemetry more it may be possible to switch from one to the other (start with disabling specific ones, then later only need to enable specific ones). But yes, that is the reason for this. Also other languages have options for both disable and enable specific instrumentations, including dotnet and java, and Python has Disabled instead of Enabled.

[blumamir]

I strongly support this feature, which I find more usable than the enabled list option.

The code changes looks good to me.

Thank you for working on this and adding this option

[JamieDanielson]

@open-telemetry/javascript-approvers Just wanted to ping to see if anyone else wanted a review before merging, since this is the auto-instr package so it is a bit more far-reaching than a single instrumentation. Otherwise will merge on Monday!

[pichlermarc]

@open-telemetry/javascript-approvers Just wanted to ping to see if anyone else wanted a review before merging, since this is the auto-instr package so it is a bit more far-reaching than a single instrumentation. Otherwise will merge on Monday!

I'm neutral on that feature. I'm usually a big proponent of people knowing which instrumentations are enabled, that's why I prefer explicitly enabling them - if we have enough that asked for this after we added the enabled env var then adding this should be okay. :)

[gnz00]

@open-telemetry/javascript-approvers Just wanted to ping to see if anyone else wanted a review before merging, since this is the auto-instr package so it is a bit more far-reaching than a single instrumentation. Otherwise will merge on Monday!

I'm neutral on that feature. I'm usually a big proponent of people knowing which instrumentations are enabled, that's why I prefer explicitly enabling them - if we have enough that asked for this after we added the enabled env var then adding this should be okay. :)

This is essential for an out of the box setup using the Kubernetes instrumentation CRD to auto-instrument existing services. We switched from using the datadog-agent to the opentelemetry-collector and the fs instrumentation overran our budget. I think it also adds parity to the other instrumentation libraries, e.g.

OTEL_DOTNET_AUTO_METRICS_{0}_INSTRUMENTATION_ENABLED

Most of the guides you'll find for the NodeSdk look like this:

const sdk: NodeSDK = new NodeSdk({
  instrumentations: [    
    getNodeAutoInstrumentations({
      // We recommend disabling fs automatic instrumentation because 
      // it can be noisy and expensive during startup
      '@opentelemetry/instrumentation-fs': {
        enabled: false,
      },
    }),
  ],
});

By adding this environment variable, you can replicate this configuration using the opentelemetry-collector's injector without modifying existing services.

[nuclearpidgeon]

Thanks for pushing through this change Jamie - where I work we were doing a lot of OpenTelemetry research earlier in the year and the whole "starting up an otel-instrumented hello world nodejs express app spits out 600 filesystem traces from module loading" thing was a big early problem I came across. I luckily also came across your example app repo that had the magic requireParentSpan: true line which stopped this, but having an environment variable now for the zero-code instrumentation case will make adopting OTel more attractive to any of our JS teams :-). (I happened to be checking back in on issue #1344 this week for one of said teams and found my way to this PR and felt compelled to say thanks!)