Instrumentation "enabled" configuration inconsistent with OpenTelemetry

[blumamir]

Instrumentation "enabled" configuration inconsistent with OpenTelemetry

[JamieDanielson]

Is there a possible third option of implementing number 1, with a deprecated codepath that checks for enabled:false and sets disabled:true if that is found?

[Flarna]

By the way, the handling of enabled flag is not done consistent as of now, see #2257

I think a single enable / disable flag is not enough in general. I would expect 2 flags:

• enable (or disable) - only evaluated at construction time to decide patch yes/no
• capture - runtime changeable, evaluated whenever some monitored/pached API is called

[blumamir]

By the way, the handling of enabled flag is not done consistent as of now, see #2257

I think a single enable / disable flag is not enough in general. I would expect 2 flags:

• enable (or disable) - only evaluated at construction time to decide patch yes/no
• capture - runtime changeable, evaluated whenever some monitored/pached API is called

So the patch will be decided at construction time and then capture will decide if an invoked patch should record telemetry or not?

In this case, when will instrumentation unpatch is expected to be called?

[Flarna]

I would never call unpatch in a production environment. There is no guarantee that it really does what one would expect.
There were quite some discussions in the regard in the past, see #3301 and linked issues for details but got stale without decission.
This didn't improve since that time, with ECMA script modules, loader threads and source code modification on load (see import in the middle for details).

I think unpatch is fine in tests. I would expect it tries to undo what patch did. In tests you usually do not have other stuff potentially patching the the same APIs so there it should work fine.

It might be reasonable to expect that setting enabled to false at runtime also sets capture to false. But that's more a topic for documentation.

[JamieDanielson]

I can think of 2 ways to fix this issue:

1. align with opentelemetry specification and change the enabled property to disabled, and change the default value to false. This will be a potential breaking change for the package which we can introduce at the moment but not after the package is stable.
2. Keep the current property as enabled but fix the places where it is used, as well as add documentation for consumers on how to use it safely.

🤔 If we change this to disabled, that would potentially be very disruptive to people using enabled: false - the first thing that comes to mind is the fs-instrumentation that produces many events and is not always needed for folks, so they add the auto-instrumentations-node metapackage, and explicitly set fs instrumentation to false. With this change, if they don't update their code but they upgrade their instrumentation package, they will need to change enabled: false to disabled: true to avoid getting an unexpected influx of events. I am concerned about how big that impact may be for folks who miss the changelog update, and it may be too much without declaring a major version. Experimental technically means we could do this... but I am still hesitant based on the potential impact to end users.

Because of the potentially large impact on end users, I'd be more in favor of Option 2 to keep things as consistent as possible. As we look to stabilize, we may consider reversing the enabled/disabled flag and evangelizing it (similar to how we will need to do it with the http attributes).