-
Notifications
You must be signed in to change notification settings - Fork 552
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(connect): fix wrong rpcMetada.route value not handle nested route #1555
fix(connect): fix wrong rpcMetada.route value not handle nested route #1555
Conversation
plugins/node/opentelemetry-instrumentation-connect/test/instrumentation.test.ts
Show resolved
Hide resolved
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1555 +/- ##
==========================================
+ Coverage 91.75% 91.80% +0.04%
==========================================
Files 137 139 +2
Lines 7084 7124 +40
Branches 1426 1432 +6
==========================================
+ Hits 6500 6540 +40
Misses 584 584
|
Broken CI will be fixed once #1556 is merged, sorry for the inconvenice. 😞 |
plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-connect/src/internal-types.ts
Outdated
Show resolved
Hide resolved
I think this PR also have some GH action's cache issue like my PR for express. Not sure how we can resolve those issue. |
can we merge this or add pkg:instrumentation-connect label so we can test tav for this? |
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. Apologies for the long time to review, and thank you for all the great contributions.
I added a few nit comments which are all optional and do not block. Feel free to resolve them if you don't want to spend more time on this PR (or address them if you are in the mood).
plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-connect/src/internal-types.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-connect/test/instrumentation.test.ts
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
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.
Thanks for addressing the comments 🙏 and for these contributions.
Please fix the lint so I can merge
@blumamir fixed |
Which problem is this PR solving?
Short description of the changes
handle
method. Wheneverhandle
is call, we add another layer in the stack. Ifhandle
also call with anout
method, we must pop the stack before out is execute. (this is opposite with current patched next where we call next first before finish span)