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

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

JamieDanielson
Copy link
Member

@JamieDanielson JamieDanielson commented May 2, 2024

Which problem is this PR solving?

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.

Copy link

codecov bot commented May 2, 2024

Codecov Report

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

Project coverage is 90.40%. Comparing base (dfb2dff) to head (be3e083).
Report is 154 commits behind head on main.

Current head be3e083 differs from pull request most recent head 1923703

Please upload reports for the commit 1923703 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2174      +/-   ##
==========================================
- Coverage   90.97%   90.40%   -0.57%     
==========================================
  Files         146      149       +3     
  Lines        7492     7527      +35     
  Branches     1502     1577      +75     
==========================================
- Hits         6816     6805      -11     
- Misses        676      722      +46     
Files Coverage Δ
...tapackages/auto-instrumentations-node/src/utils.ts 98.18% <94.11%> (-0.60%) ⬇️

... and 52 files with indirect coverage changes

Copy link
Contributor

@david-luna david-luna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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.

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants