-
Notifications
You must be signed in to change notification settings - Fork 544
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
feat(opentelemetry-instrumentation-fastify): Support Fastify V4 also #1164
Conversation
|
plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.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.
overall lgtm, could you sign the CLA please ?
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1164 +/- ##
==========================================
- Coverage 96.08% 96.04% -0.04%
==========================================
Files 14 19 +5
Lines 893 1113 +220
Branches 191 230 +39
==========================================
+ Hits 858 1069 +211
- Misses 35 44 +9
|
@vmarchaud I am waiting on my Corp CLA manager. |
@vmarchaud All done with the CLA. |
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.
could you update the readme supported version too ? https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-fastify/README.md#supported-versions. Othersiwe LGTM
plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
@vmarchaud Yeah, good call. That's added. |
@markrzen not sure if you forgotten but you didn't push a modification to include the newly supported version (https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-fastify/README.md?rgh-link-date=2022-09-18T07%3A19%3A00Z#supported-versions) |
@vmarchaud Sorry about that. I didn't know it was also documented on the README. Just pushed that change. |
No worries :) I made a comment about it thats why i though you add forgotten it |
@vmarchaud Thanks so much. |
@vmarchaud What's our next step for this PR? Not trying to rush, just making sure I am not missing a step. |
@markrzen Just getting more eyes to look at it to verify its fine, we generally require 2-3 reviews to merge |
!s.attributes[AttributeNames.PLUGIN_NAME] || | ||
s.attributes[AttributeNames.PLUGIN_NAME] === 'fastify' |
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.
Can you explain what changed so that now we need this extra check?
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.
Absolutely. Fastify reworked their namespacing of plugins. All plugins are now under the fastify top level namespace.
What used to be "plugin.name": "my-plugin"
is now "plugin.name": "fastify -> my-plugin"
.
The extra check is there so that if you wanted to switch between fastify versions and run tests, you could and still have consistent results.
Fastify 3.x checking if the plugin name does not exist is enough, for Fastify 4.x the equivalent is to check if the plugin name is just fastify
.
If you would like me to remove it from the tests as you don't think y'all will be switching versions, I am totally happy to.
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.
@blumamir if the explaination is fine for you feel free to go ahead and merge this
Which problem is this PR solving?
Short description of the changes
Checklist
npm run test-all-versions
for the edited package(s) on the latest commit if applicable.