[instrumentation] consider dropping Instrumentation#disable()

[pichlermarc]

Description

Important

Do not start working on implementation yet. This is a feature-request that's pending maintainer feedback for open questions.
Researching feasibility of this request and feedback is encouraged. If you do, please post your findings as a comment on this issue.

Every OpenTelemetry instrumentation has a disable() functionality that allows users to call it to unwrap the library it's instrumenting on runtime.

import-in-the-middle@1.11.1 introduced a change via nodejs/import-in-the-middle#153 which broke that functionality for ESM users. Since that version was released, we've not received a single report that this functionality is broken - which leads me to believe that this feature is of negligible value to our end-users, all while introducing additional complexity, points-of-failure and maintainance overhead to our @opentelemetry/instrumentation-* packages and third-party instrumentations.

Internally this feature seems to be mainly intended for testing - we make extensive use of it though our projects in integration tests.

I propose we deprecate the functionality, make Instrumentation#disable() optional and then gradually phase out use of it in our instrumentations and tests before eventually removing it altogether.

Possible ways to move forward

Before moving forward, we (@open-telemetry/javascript-maintainers) will have to find out if this is the path we want to go. If the answer is yes we need to:

• create a follow-up type:research issue to determine if removing disable from tests is even feasible. I suggest we use @opentelemetry/instrumentation-http for this kind of experiment as it is the most feature-rich instrumentation.
  • we may have to split tests that rely on wrapping with different options to different test files, which mocha will execute in different processes, which could make unwrap functionality unnecessary while reducing the amount of code-changes needed to accomplish the task.

If the outcome of this research is that it is feasible, we should:

• create a follow-up type:feature issue to deprecate Instrumentation#disable() and make it optional in the interface.
  • Doing so will break @opentelemetry/instrumentation registerInstrumentations() (old instrumentation may be used with the latest @opentelemetry/instrumentation version, but instrumentations that depend on the latest version will not work with older versions of registerInstrumentations().
• create create a type:feature-tracking follow-up for this repository (https://github.com/open-telemetry/opentelemetry-js) to remove the usages of disable() from tests
• create create a type:feature-tracking follow-up for this repository (https://github.com/open-telemetry/opentelemetry-js) to remove the usages of disable() from tests

Additional Resources

• import-in-the-middle PR that introduced the behavior that broke disable on ESM: fix: Support Hooking multiple times nodejs/import-in-the-middle#153
• PR to skip ESM disable tests in @opentelemetry/instrumentation: test(instrumentation): skip unwrap tests for esm #5153
• <please comment with links to any additional context and I'll add it here - I know that we talked about this somewhere else before but could not find the specific thread>

[trentm]

• we may have to split tests that rely on wrapping with different options to different test files, which mocha will execute in different processes

Nope. Mocha runs the separate test files in the same process.

example

% pwd
/Users/trentm/tm/opentelemetry-js9/experimental/packages/opentelemetry-instrumentation-http

% ls test/*.test.ts
test/a.test.ts test/b.test.ts

% cat test/a.test.ts
describe('a test', () => {
  it('should show its PID', async () => {
    console.log('pid is', process.pid);
  });
});

% cat test/b.test.ts
describe('b test', () => {
  it('should show its PID', async () => {
    console.log('pid is', process.pid);
  });
});

% npx mocha test/*.test.ts

  a test
pid is 95618
    ✔ should show its PID

  b test
pid is 95618
    ✔ should show its PID


  2 passing (2ms)

I (now vaguely) recall this being a source of surprises in some of the tests: a test suite that is enabling/disabling repeatedly or a test suite that is enabling instrumentation in separate test files is sometimes getting multiple wrapping/monkey-patching of the module.

[pichlermarc]

• we may have to split tests that rely on wrapping with different options to different test files, which mocha will execute in different processes

Nope. Mocha runs the separate test files in the same process.
[..]
I (now vaguely) recall this being a source of surprises in some of the tests: a test suite that is enabling/disabling repeatedly or a test suite that is enabling instrumentation in separate test files is sometimes getting multiple wrapping/monkey-patching of the module.

Mhm, I see - that's a bummer :/
For some reason I thought that was the case - thanks for the correction - I crossed that comment out in the issue above.

[djMax]

Funnily enough I think I need this feature now. My service hosts two express/http instances, and I only want to instrument one of them. Seems like disable() would be one way to do that?

[pichlermarc]

Funnily enough I think I need this feature now. My service hosts two express/http instances, and I only want to instrument one of them. Seems like disable() would be one way to do that?

Hmm, I don't think that'd work with disable(), unfortuantely. It would just unwrap both instances, I think what you'd want is to not wrap them in the first place or some kind of filter or to suppress telemetry for the other instance. 🤔 Suppressing tracing for certain calls can be done via suppressTracing() from @opentelemetry/core.