-
Notifications
You must be signed in to change notification settings - Fork 534
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
chore: adding instrumentation for connect #588
Conversation
e24b3e5
to
07a5341
Compare
Codecov Report
@@ Coverage Diff @@
## main #588 +/- ##
==========================================
+ Coverage 94.96% 94.98% +0.01%
==========================================
Files 200 199 -1
Lines 11787 11849 +62
Branches 1122 1108 -14
==========================================
+ Hits 11194 11255 +61
- Misses 593 594 +1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just need to fix tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits, otherwise beautifully crafted! 👍
plugins/node/opentelemetry-instrumentation-connect/test/instrumentation.test.ts
Outdated
Show resolved
Hide resolved
@obecny, see #588 (comment) |
What should we do about the component_owners.yml in the case maintainers add the instrumentation? Is it ok to have an unowned instr or should you own it? |
Well i think we should add them for every instrumentation in case there is a change in maintainer ? Specially since its one of us that have wrote it, it will force us to switch ownership when stepping down. |
Stepping down as maintainer wouldn't mean stepping down as component owner I think. |
Not necessarely i agree, however if a maintainer don't have the time anymore to work on otel i suppose he would not have the time to review PRs about components he owns (in the case of a step down because of lack of time of course). |
Which problem is this PR solving?
Short description of the changes