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

fix: remove unuseful patch message from instrumentations #2161

Merged
merged 30 commits into from
May 2, 2024

Conversation

blumamir
Copy link
Member

#2107 removed the diag prints on InstrumentationNodeModuleDefinition and InstrumentationNodeModuleFile, patch and unpatch callbacks, as this message is now hoisted to be emitted consistently from the base class. The scope of this PR was narrow to handle specific types of messages that became redundant, but it was noted that there are other diag prints for patching and unpatching specific function from moduleExports.

This PR is a followup to #2107

  • Cleanup diag patch messages for trivial function which created noise and added no real value for troubleshooting to users.
  • Document guidelines on using diag channel to emit patch messages, based on the examples I observed in the repo.

@blumamir
Copy link
Member Author

@JamieDanielson @trentm @pichlermarc thanks again for the great feedback and discussion on the previous PR.

Since those changes were intentionally left out of the previous one, I opened a follow-up to apply the issues we talked about in a dedicated PR. I also added a section to the GUIDELINES.md file for future reference to capture the decisions.

Please let me know if this work correctly reflects the discussion in #2107 🙏

@blumamir blumamir changed the title chore: remove unuseful patch message from instrumentations fix: remove unuseful patch message from instrumentations Apr 27, 2024
Copy link

codecov bot commented Apr 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.45%. Comparing base (dfb2dff) to head (8185fbd).
Report is 99 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2161      +/-   ##
==========================================
- Coverage   90.97%   90.45%   -0.53%     
==========================================
  Files         146      147       +1     
  Lines        7492     7571      +79     
  Branches     1502     1589      +87     
==========================================
+ Hits         6816     6848      +32     
- Misses        676      723      +47     
Files Coverage Δ
...ode/instrumentation-tedious/src/instrumentation.ts 92.30% <ø> (-0.17%) ⬇️
...lemetry-instrumentation-dns/src/instrumentation.ts 94.59% <ø> (+11.05%) ⬆️
...try-instrumentation-fastify/src/instrumentation.ts 95.41% <ø> (-0.07%) ⬇️
...emetry-instrumentation-hapi/src/instrumentation.ts 99.30% <ø> (-0.01%) ⬇️
...emetry-instrumentation-knex/src/instrumentation.ts 98.70% <100.00%> (-0.08%) ⬇️
...metry-instrumentation-mysql/src/instrumentation.ts 92.72% <ø> (-0.46%) ⬇️
...etry-instrumentation-mysql2/src/instrumentation.ts 94.52% <ø> (-0.22%) ⬇️
...metry-instrumentation-redis/src/instrumentation.ts 92.68% <ø> (-0.80%) ⬇️

... and 24 files with indirect coverage changes

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great - I especially love the addition to the guidelines explaining where it might be useful to have logging about patches. I left a few nits but nothing blocking from me.

GUIDELINES.md Outdated Show resolved Hide resolved
GUIDELINES.md Outdated Show resolved Hide resolved
GUIDELINES.md Outdated Show resolved Hide resolved
GUIDELINES.md Outdated Show resolved Hide resolved
GUIDELINES.md Outdated Show resolved Hide resolved
GUIDELINES.md Outdated
@@ -164,3 +164,36 @@ To support this use case, you can choose one of the following options:
};
...
```

## Diag Channel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩 Love having this guideline written out here


Instrumentation should not add additional debug messages for triggering the patching and unpatching callbacks, as the base class will handle this.

Instrumentation may add additional patch/unpatch messages for specific functions if it is expected to help in troubleshooting issues with the instrumentation. Few examples:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, another thought I had that I think will probably be updated in the follow-up PR that replaces inconsistent usages of api.diag.debug and similar that aren't using the instrumentation this to allow for this._diag.debug. When we make that change we should update guidelines for that as well, though it will already be easier to follow suit once these are all more consistent 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

certainly, I aim to first remove or hoist unneeded diag messages, and then will transform all remaining diag message to use this._diag and update the GUIDELINES.md 👍

open-telemetry/opentelemetry-js#4663 will also help in hosting many diag prints and remove the repeated code from dozens of places in contrib.

blumamir and others added 5 commits April 30, 2024 05:09
Co-authored-by: Jamie Danielson <[email protected]>
Co-authored-by: Jamie Danielson <[email protected]>
Co-authored-by: Jamie Danielson <[email protected]>
Co-authored-by: Jamie Danielson <[email protected]>
Co-authored-by: Jamie Danielson <[email protected]>
@blumamir
Copy link
Member Author

Thank you @JamieDanielson

Your suggestions are always valuable. addressed all the comments 🙏

GUIDELINES.md Outdated Show resolved Hide resolved
GUIDELINES.md Outdated Show resolved Hide resolved
@blumamir blumamir merged commit 34f56e0 into open-telemetry:main May 2, 2024
17 checks passed
@dyladan dyladan mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants