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

[auto-instrumentations-node] disabling/removing fs instrumentation #2453

Open
pichlermarc opened this issue Sep 26, 2024 · 1 comment · May be fixed by #2467
Open

[auto-instrumentations-node] disabling/removing fs instrumentation #2453

pichlermarc opened this issue Sep 26, 2024 · 1 comment · May be fixed by #2467

Comments

@pichlermarc
Copy link
Member

pichlermarc commented Sep 26, 2024

Description

The @opentelemetry/autoinstrumentations-node package is usually used to get a quick-start on instrumenting with OpenTelemetry JS, however, there are a few instrumentations that behave in very verbose ways:

  • @opentelemetry/instrumentation-fs
  • @opentelemetry/instrumentation-net
  • @opentelemetry/instrumentation-dns

This proposal is limited to @opentelemetry/instrumentation-fs, as it is the most verbose of them, creating error spans that may be picked up by error reporting even though the error is expected by the user. An example of this includes trying to access a file that does not exist, then catching the error. The @opentelemetry/instrumentation-fs package will then create an error span, even though this behavior is completely intended by the user. Acting in such a way is even recommened by the Node.js documentation, as checking with fs.access() before accessing the file can introduce a race condition.

In addition to the error spans, @opentelemetry/instrumentation-fs also generates a lot of spans during startup, when files are required. This causes increased memory pressure for fs calls that are most likely not relevant for the user.

To see what I'm talking about, see this demo.

Once we have decided on a way forward, we can give a similar treatment to @opentelemetry/instrumentation-net and @opentelemetry/instrumentation-dns

Proposal

(1) I propose we remove @opentelemetry/instrumentation-fs from the automatically enabled instrumentations that are returned by getNodeAutoinstrumentations() unless:

  • OTEL_JS_ENABLED_INSTRUMENTATIONS constains fs
  • or @opentelemetry/instrumentation-fs is explicitly enabled when calling getNodeAutoinstrumentations()

This has the drawback of breaking users that rely on the telemetry generated by this instrumentation. If they want to get it back, they have to explicitly enable it via one of the above options. OTEL_JS_ENABLED_INSTRUMENTATIONS is an allow-list so they will have to also explicitly list every other instrumentation they want to have enabled, which may be tedious.

If there are no objections, I will go forward with this approach 2 weeks after this issue was opened.

Other options:

  • (2) removing @opentelemetry/instrumentation-fs from @opentelemetry/autoinstrumentations-node (this will break getNodeAutoinstrumentations() users, but they will not be able to get the instrumentation back by listing the instrumentation via configuration only. They will have to add a dependency to the package and instantiate and register the instrumentation themselves)
  • (3) removing @opentelemetry/instrumentation-fs from defaults but introducing "profiles" (or "groups" of instrumentations) that can be enabled
    • OTEL_JS_ENABLED_INSTRUMENTATIONS=group:legacy (would allow users to go back to the "previous" state)
    • this has the drawback of adding yet another way of configuring the @opentelemetry/autoinstrumentations-node, which means additional maintenance overhead, possible user-confusion.
  • (n) other options I did not think of yet (I will add them here as they come up in discussion)

Additional context:

@pichlermarc
Copy link
Member Author

Since two weeks have passed, I opened #2467 to disable it by default using Option 1 from the issue text above.

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