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

feat: new fastify instrumentation #611

Merged
merged 10 commits into from
Oct 22, 2021
Merged

Conversation

obecny
Copy link
Member

@obecny obecny commented Aug 11, 2021

Which problem is this PR solving?

Short description of the changes

  • new instrumentation for fastify

@obecny obecny added the enhancement New feature or request label Aug 11, 2021
@obecny obecny requested a review from a team August 11, 2021 11:40
@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #611 (ff5924c) into main (25b93bb) will decrease coverage by 0.16%.
The diff coverage is 100.00%.

❗ Current head ff5924c differs from pull request most recent head 1986524. Consider uploading reports for the commit 1986524 to get more accurate results

@@            Coverage Diff             @@
##             main     #611      +/-   ##
==========================================
- Coverage   96.86%   96.69%   -0.17%     
==========================================
  Files           9       16       +7     
  Lines         638     1029     +391     
  Branches      126      159      +33     
==========================================
+ Hits          618      995     +377     
- Misses         20       34      +14     
Impacted Files Coverage Δ
...entelemetry-instrumentation-fastify/src/version.ts 100.00% <100.00%> (ø)
...strumentation-fastify/test/instrumentation.test.ts 96.88% <0.00%> (ø)
...try-instrumentation-fastify/src/instrumentation.ts 97.29% <0.00%> (ø)
...entelemetry-instrumentation-fastify/src/version.ts 100.00% <0.00%> (ø)
...opentelemetry-instrumentation-fastify/src/utils.ts 90.00% <0.00%> (ø)
...telemetry-instrumentation-fastify/src/constants.ts 100.00% <0.00%> (ø)
...nstrumentation-fastify/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)

@naseemkullah
Copy link
Member

cc @mcollina in case you'd like to take a look (your feedback for pino instrumentation was helpful!), file of most interest: plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts 🙏

@mcollina
Copy link

Feel free to ping me whenever you need for all instrumentations of Fastify.

@YanivD
Copy link
Member

YanivD commented Aug 25, 2021

Just a small note -
Can you add @opentelemetry/instrumentation-fastify to Node Instrumentations list?
All merged packages are updated in #638

@carlosalberto
Copy link

Ping @mcollina ;)

examples/connect/server.js Show resolved Hide resolved
examples/mongodb/server.js Show resolved Hide resolved
examples/fastify/tracing.js Show resolved Hide resolved
examples/fastify/tracing.js Show resolved Hide resolved
@obecny
Copy link
Member Author

obecny commented Sep 9, 2021

@blumamir appreciate the detailed review, pls have a look I either fixed or commented. Just still not sure about the export problems you mentioned

@blumamir
Copy link
Member

@blumamir appreciate the detailed review, pls have a look I either fixed or commented. Just still not sure about the export problems you mentioned

Thanks for addressing the review. Added comments and examples on the open issues.
I understand if some of these issues are things you choose to not fix in the scope of this PR. I can also offer my help (here or in slack) in discussing possible solutions to the above cases if you like.

@obecny
Copy link
Member Author

obecny commented Oct 19, 2021

@blumamir addressed the last things, the one you added and the fixes we discussed, please check how it looks now. Thx for all suggestions and samples

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

As discussed with @obecny , we think this PR is ready to be merged as is, and later fixes and enhancements can be added later as follow-up PRs.
Since it's currently not in release-please config, it will not be published to npm, allowing me to add a few fixes before it goes public

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Instrumentation - fastify
7 participants