feat(auto-instrumentations-node): enabling instrumentations via env

[atsu85]

Which problem is this PR solving?

Allowing to speed up application startup time by allowing to disable instrumentations that are not needed via environment variable for use-cases where it isn't possible to disable instrumentations via inputConfigs object passed to getNodeAutoInstrumentations(inputConfigs) (like when using Kubernetes opentelemetry-operator - its instrumentation code can't be customized by applications running in the Kubernetes cluster). More information in this comment

Short description of the changes

This PR allows enabling instrumentations selected via OTEL_NODE_ENABLED_INSTRUMENTATIONS environment variable while disabling others.

To keep backwards compatibility, by default all instrumentations are still enabled when the environment variable isn't provided.

Usage example:

as documented in the README change:

To enable only
@opentelemetry/instrumentation-http
and @opentelemetry/instrumentation-nestjs-core
instrumentations:

export OTEL_NODE_ENABLED_INSTRUMENTATIONS="http,nestjs-core"

Alternative approaches considered

In this comment I proposed two approaches how to enable/disable specific instrumentations via environment variables.

This PR implements the first approach. The benefits of this approach are:

• it allows disabling all non-required instrumentations with single environment variable (easier to configure, similar to configuring resource detectors via OTEL_NODE_RESOURCE_DETECTORS)
• when new instrumentations are added here (@opentelemetry/auto-instrumentations-node npm package), they won't be enabled by default when only selected instrumentations were enabled, and hence won't degrade application startup time (no need to add additional environment variables to disable each new instrumentation when this package is updated)

This does not prevent implementing later also second approach backwards compatibly.
Could implement it for example so that globally only few instrumentations are enabled for all applications using OTEL_NODE_ENABLED_INSTRUMENTATIONS and each application could enable additional instrumentations with second approach based on additional environment variables for each instrumentation

[atsu85]

I thought I'll share the investigation results that motivated this PR:

Summary

Auto-instrumentation is much slower compared to instrumentation after application has loaded dependencies:

I tested this in our Kubernetes cluster

When performing instrumentation after loading application bootstrap import modules, application startup took just 1.5 seconds. Auto-instrumentation (with --require path/to/instrumentation.js) is a lot slower when all instrumentations are enabled:

• ca 27 times slower in case of quite low CPU limit (that throttles CPU a lot: 300m)

• ca 6.5 times slower in case of quite high CPU limit (that prevents CPU throttling: 1500m)

Disabling resource detectors didn’t have a visible effect on startup time with instrumentation agent

While disabling resource detectors could be uselful to store less information in each span, disabling all detectors (using export OTEL_NODE_RESOURCE_DETECTORS=none) didn't have any visible effect

In which startup phase the time is spent?

I tested this part locally on my personal machine with much more resources than allocated to the application in our Kubernetes cluster

• The slowest startup phases are importing modules for bootstraping the application and starting NestJS:

  • loading auto-instrumentation: 0 vs 374ms

  • importing modules (after loading instrumentation): 244 vs 4033ms

  • starting app (after importing modules): 123 vs 2714ms

• disabling all instrumentation besides http and nest-js will add just 0.4s startup overhead on my machine

Profiling application startup locally revealed that almost all the time is wasted on loading node modules

• because instrumentation is done before loading application bootstrap modules (probably gazillions of NodeJS modules and each module is instrumented by 37 instrumentations enabled by default)
  

  • when instrumentation was done after loading application bootstrap modules (manually in code, not via --require ...), the overhead for the startup was insignificant

• When i disabled all instrumentations, then application started up almost without any overhead to the application startup process (besides mainly loading the agent - ca 0.4 seconds)

[atsu85]

@obecny, could you take a look?

[obecny]

thx for this contribution, few suggestions with regards to naming so they are more descriptive, other than that would lgtm, thx

[obecny]

looking at the comment when this idea comes from, I would also think about adding
OTEL_DISABLED_NODE_INSTRUMENTATIONS explaining that you can use either OTEL_ENABLED_NODE_INSTRUMENTATIONS or OTEL_DISABLED_NODE_INSTRUMENTATIONS, never both. That would give user more flexibility if you for example want to enable only 2 or always disable only certain one. Of course not in this PR, just something to consider for maybe next PR, thx

[atsu85]

@obecny, thanks for super-quick feedback.

Adding another variable OTEL_DISABLED_NODE_INSTRUMENTATIONS sounds good, but i would leave it out of this PR, because

• I'm not sure if after this feature people even would start using it. I'm dogfooding content currently present in this PR, but i wouldn't dogfood the OTEL_DISABLED_NODE_INSTRUMENTATIONS part, and i'm not sure if anyone else would start using it either.
• implementing it would be fairly easy without introducing breaking changes related to this PR

What do you think?

[obecny]

@obecny, thanks for super-quick feedback.

Adding another variable OTEL_DISABLED_NODE_INSTRUMENTATIONS sounds good, but i would leave it out of this PR, because

• I'm not sure if after this feature people even would start using it. I'm dogfooding content currently present in this PR, but i wouldn't dogfood the OTEL_DISABLED_NODE_INSTRUMENTATIONS part, and i'm not sure if anyone else would start using it either.
• implementing it would be fairly easy without introducing breaking changes related to this PR

What do you think?

definitely in next PR, that was just suggestions and some more info why having "enabled" in name would make possible to have 2 features and then user can set it much easier.

[obecny]

thx for changes, lgtm

[atsu85]

@obecny, thanks for the feedback and approving the PR!

It seems that I can't merge this PR myself and I need help from you regarding 2 topics:

1. based on Pull Request Merge Requirements there shouldn't be any unresolved conversations before merging a PR:

Pull requests MAY be merged by an approver OR a maintainer provided they meet all the following requirements:
...

• No “changes requested” reviews or unresolved conversations by
  ...
  • component owners

Perhaps You'll mark CR comment threads also as resolved Yourself? (I could do it myself, but IMHO the reviewer should decide if it is resolved or not ;) )

2. GH is still showing that "Merging is blocked" and "Code owner review required":
   
   most likely because of this line in CODEOWNERS file,
   but that team either doesn't exist (this link doesn't work for me) or it exists and I just can't see it because I'm not a member of that team?
   However based on Pull Request Merge Requirements you could merge the PR (because @obecny is component owner for auto-instrumentations-node):

Pull requests MAY be merged by an approver OR a maintainer provided they meet all the following requirements:

• Approved by
  • at least one component owner if one is defined in .github/component_owners.yml

@obecny, can you merge the PR yourself or should i do smth else myself to get it merged?

[atsu85]

@obecny, can you merge the PR yourself or should i do smth else myself to get it merged?
After reading again Pull Request Merge Requirements from contribution guide, it seems that you should be able to merge it (I also updated my previous comment regarding 2 topics I raised yesterday)

[obecny]

@obecny, can you merge the PR yourself or should i do smth else myself to get it merged? After reading again Pull Request Merge Requirements from contribution guide, it seems that you should be able to merge it (I also updated my previous comment regarding 2 topics I raised yesterday)

unfortunately not, you will have to wait for someone with at least js approver privileges
https://github.com/orgs/open-telemetry/teams/javascript-approvers

[atsu85]

@open-telemetry/javascript-approvers, please approve this PR - @obecny, who is the component owner for auto-instrumentations-node, has already aproved it

[atsu85]

@pichlermarc , if i understood the note

Maintainer @open-telemetry JavaScript SIG.,

under Your GH profile correctly, then you should be member of javascript-approvers whom i pined (after component owner approved this pr) without getting a response.

Can you merge it and release it yourself or is there smth else I need to do?

I'd really like to start using it in lots of our projects.

[atsu85]

@blumamir, can you merge this PR? Looks like you have the merge permissions that the component-owner (who approved this PR doesn't have)

[codecov]

Codecov Report

Merging #1953 (156b6cf) into main (dfb2dff) will decrease coverage by 0.12%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1953      +/-   ##
==========================================
- Coverage   90.97%   90.86%   -0.12%     
==========================================
  Files         146      143       -3     
  Lines        7492     7379     -113     
  Branches     1502     1471      -31     
==========================================
- Hits         6816     6705     -111     
+ Misses        676      674       -2     

Files	Coverage Δ
...tapackages/auto-instrumentations-node/src/utils.ts	98.92% <100.00%> (+0.14%)	⬆️

... and 4 files with indirect coverage changes

[pichlermarc]

Sorry, was busy with other PRs and was not able to pick up a new feature to review yet.
Also it looks like this package was not owned by the maintainers so we did not get auto-assigned (opened #1996 to fix that)

Generally looks good % comments (env var naming and lint fixes) 👍

[pichlermarc]

I'm not sure if after this feature people even would start using it. I'm dogfooding content currently present in this PR, but i wouldn't dogfood the OTEL_DISABLED_NODE_INSTRUMENTATIONS part, and i'm not sure if anyone else would start using it either.

I agree with this. I think it's good to hold off on implementing this and would argue that a follow up to implement this is not necessary. Let's see if there's people who have a real need for it first.

I think the here implemented option of explicitly opting-in to instrumentations will be preferred by most.

[atsu85]

@pichlermarc, thanks for approving!
Can you merge and release it as well, or is there anything I could do myself, without having merge permissions?

[pichlermarc]

@pichlermarc, thanks for approving! Can you merge and release it as well, or is there anything I could do myself, without having merge permissions?

I'm going to merge it now, but the release will only happen next week as I'd like to avoid releasing on a Friday.

[hamin]

Hurray! Was waiting for this for so long!