chore: remove patch and unpatch diag from instrumentations

[blumamir]

This PR implements open-telemetry/opentelemetry-js#4641 to the contrib instrumentations, so once the core repo PR is merged, we can quickly rebase this PR and apply the changes in contrib so we can release them together.

[codecov]

Codecov Report

Attention: Patch coverage is 78.57143% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 90.45%. Comparing base (dfb2dff) to head (9d8f67b).
Report is 72 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2107      +/-   ##
==========================================
- Coverage   90.97%   90.45%   -0.53%     
==========================================
  Files         146      147       +1     
  Lines        7492     7576      +84     
  Branches     1502     1574      +72     
==========================================
+ Hits         6816     6853      +37     
- Misses        676      723      +47     

Files	Coverage Δ
...de/instrumentation-cucumber/src/instrumentation.ts	92.10% <100.00%> (-0.21%)	⬇️
.../instrumentation-dataloader/src/instrumentation.ts	99.06% <100.00%> (-0.02%)	⬇️
...ode/instrumentation-tedious/src/instrumentation.ts	92.39% <100.00%> (-0.09%)	⬇️
...-instrumentation-aws-lambda/src/instrumentation.ts	93.60% <ø> (-0.08%)	⬇️
...entelemetry-instrumentation-aws-sdk/src/aws-sdk.ts	95.02% <ø> (-0.11%)	⬇️
...etry-instrumentation-bunyan/src/instrumentation.ts	97.89% <100.00%> (-0.81%)	⬇️
...try-instrumentation-connect/src/instrumentation.ts	98.98% <100.00%> (-0.03%)	⬇️
...lemetry-instrumentation-dns/src/instrumentation.ts	94.66% <ø> (+11.12%)	⬆️
...try-instrumentation-express/src/instrumentation.ts	100.00% <100.00%> (ø)
...try-instrumentation-fastify/src/instrumentation.ts	95.45% <100.00%> (-0.04%)	⬇️
... and 20 more

... and 10 files with indirect coverage changes

[JamieDanielson]

I notice there are a few instrumentations not included here that I think are intentional - can you confirm I have this right? For example, mongoose has a patchAggregateExec that includes this._diag.debug('patched mongoose Aggregate exec function');. I think this will at the top level use the shared diag from the main instrumentation package for patching the module itself, and there was just nothing to remove from there because it didn't have a general patch log... and then it will still have additional diag logging for those specific types of patches when it comes up, which seems to be by design already within that instrumentation. Does that sound accurate? A similar situation seems to exist for fastify and a couple others, though there were main patch logs to remove here that you took care of.

I think mongoose will still be able to use the shared unpatch diag log, and koa is missing an update because its syntax is a bit different api.diag.debug('Patching Koa');

[JamieDanielson]

Also just noticed Hapi also uses api.diag.debug('Patching Hapi.server');.

As a follow-up after this, I think we could get these instrumentations consistently using this._diag.debug for their remaining debug statements 🤔 Those could be done separately in small individual PRs though to limit the scope of this.

[blumamir]

Thanks for the thorough review.

can you confirm I have this right? For example, mongoose has a patchAggregateExec that includes this._diag.debug('patched mongoose Aggregate exec function');. I think this will at the top level use the shared diag from the main instrumentation package for patching the module itself, and there was just nothing to remove from there because it didn't have a general patch log... and then it will still have additional diag logging for those specific types of patches when it comes up, which seems to be by design already within that instrumentation. Does that sound accurate? A similar situation seems to exist for fastify and a couple others, though there were main patch logs to remove here that you took care of.

That is correct, the intention was to address only the patch and unpatch of the module (or module file) itself, and not the specific function patchs from the module exports which I intend to cleanup in a different PR.

Not sure how much value we get from reporting specific function patches like mongoose and hapi does. I think we should either:

1. clean this up from all existing instrumentations.
2. hoist it to the base instrumentation and apply to all instrumentations from the base class _wrap and _unwrap functions.
3. Understand when it is needed and when not, and align the existing prints to a documented practice.

[blumamir]

and koa is missing an update because its syntax is a bit different api.diag.debug('Patching Koa');

Thanks! I updated the koa and also few other places. I think now it is removed from all relevant places. please share if you spot one that I missed

[blumamir]

As a follow-up after this, I think we could get these instrumentations consistently using this._diag.debug for their remaining debug statements 🤔 Those could be done separately in small individual PRs though to limit the scope of this.

Absolutely, I intend to align everything to this._diag in a followup PR, which isn't always trivial as some diag useages are coming from utils.ts and are lacking the instrumentation this, but this will be handled in a different PR after some diag usages are removed

[JamieDanielson]

Not sure how much value we get from reporting specific function patches like mongoose and hapi does. I think we should either:

1. clean this up from all existing instrumentations.
2. hoist it to the base instrumentation and apply to all instrumentations from the base class _wrap and _unwrap functions.
3. Understand when it is needed and when not, and align the existing prints to a documented practice.

With the function patches I suppose it may be helpful when debugging to see if something specific is not getting patched as expected, similar to the purpose here for the module itself, but it's maybe more helpful for some instrumentations vs others.. It is something to consider whether we can get a more consistent practice there too, but I don't see it being quite as impactful as this change for the whole module log, or for general usage of this._diag.debug.

[JamieDanielson]

Thanks for working on this! It's looking great. I'm eager to see these tests pass with the updated instrumentation package once it's available 🚀

[trentm]

Compile error in instr-mongodb:

 > @opentelemetry/[email protected] compile
> tsc -p .

src/instrumentation.ts:18:3 - error TS6133: 'diag' is declared but its value is never read.

18   diag,
     ~~~~

[trentm]

Not sure how much value we get from reporting specific function patches like mongoose and hapi does. I think we should either:

1. clean this up from all existing instrumentations.

Speaking from a little experience with instr-hapi, I think those extra diag.debug messages are not useful for debugging at all. +1 for doing option 1 here.

[blumamir]

Thanks @JamieDanielson and @trentm I support addressing the individual diag prints as well, mainly I think we should remove them unless there is some non-standard patching logic which can aid debugging?

As this PR already introduces quite some changes, I want to address the remaining patch diag prints in a followup PR.

I rebased this branch from main after merging #2091 and fixed all the conflicts. Thanks again for helping iterate this and the great suggestions.

[pichlermarc]

Looks good. 👍

I agree that we should address the function-specific logs in a follow-up and not increase the scope of this PR for now. Merging this will unblock the release 🙂