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

Fastify sdk contrib #523

Merged
merged 36 commits into from
Aug 29, 2022
Merged

Fastify sdk contrib #523

merged 36 commits into from
Aug 29, 2022

Conversation

jorgevrgs
Copy link
Contributor

Issue #, if available:

Description of changes:

Add X-Ray plugin for Fastify

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jorgevrgs jorgevrgs requested a review from a team as a code owner August 2, 2022 02:13
@willarmiros
Copy link
Contributor

Hi @jorgevrgs,

Thanks for contributing this instrumentation! Do you have any vision or plan for longer-term support for the instrumentation? Generally speaking we do not maintain the instrumentations in the sdk_contrib, so it would be nice to know someone with more expertise on the framework is able to help out with customer issues if they come in.

Also, we are advising newcomers to X-Ray to try out using OpenTelemetry JS, which already has an instrumentation for fastify. So we also don't want to be unnecessarily duplicating instrumentation options if the OpenTelemetry one would be suitable. Let me know what you think.

@jorgevrgs
Copy link
Contributor Author

Hi @jorgevrgs,

Thanks for contributing this instrumentation! Do you have any vision or plan for longer-term support for the instrumentation? Generally speaking we do not maintain the instrumentations in the sdk_contrib, so it would be nice to know someone with more expertise on the framework is able to help out with customer issues if they come in.

Also, we are advising newcomers to X-Ray to try out using OpenTelemetry JS, which already has an instrumentation for fastify. So we also don't want to be unnecessarily duplicating instrumentation options if the OpenTelemetry one would be suitable. Let me know what you think.

I checked the contribution guide about the support requirement. Yes, I can help you to bring support, but if you have someone in mind please invite them. It would be great to have more people contributing here.

@willarmiros
Copy link
Contributor

Hi @jorgevrgs can you fix the failing CI tests? Then I can take a look

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Looks like there are still some errors, possibly related to the lerna config. I would recommend bootstrapping & running tests locally to be sure they pass

sdk_contrib/fastify/NOTICE.txt Outdated Show resolved Hide resolved
sdk_contrib/fastify/.prettierrc Outdated Show resolved Hide resolved
sdk_contrib/fastify/package.json Outdated Show resolved Hide resolved
sdk_contrib/fastify/package.json Outdated Show resolved Hide resolved
sdk_contrib/fastify/package.json Show resolved Hide resolved
sdk_contrib/fastify/test/unit/xray.test.js Outdated Show resolved Hide resolved
@jorgevrgs
Copy link
Contributor Author

Looks like there are still some errors, possibly related to the lerna config. I would recommend bootstrapping & running tests locally to be sure they pass

Checking it, thanks for the info. Fixing ASAP.

@willarmiros
Copy link
Contributor

Hmm seems to still be failing - try running the same commands that the CI runs (found here:

npx lerna bootstrap --hoist
npx lerna run compile
npx lerna run test
) locally from the root of the repo. I guess that's the best way to avoid these snags

@jorgevrgs
Copy link
Contributor Author

jorgevrgs commented Aug 23, 2022

Getting this error:

lerna ERR! npm run test exited 1 in 'aws-xray-sdk-mysql' lerna WARN complete Waiting for 4 child processes to exit. CTRL-C to exit immediately.

I will sync branches to check what's wrong.

@jorgevrgs
Copy link
Contributor Author

Hmm seems to still be failing - try running the same commands that the CI runs (found here:

npx lerna bootstrap --hoist
npx lerna run compile
npx lerna run test

) locally from the root of the repo. I guess that's the best way to avoid these snags

Done:

image

@jorgevrgs jorgevrgs closed this Aug 28, 2022
@jorgevrgs jorgevrgs reopened this Aug 28, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #523 (8697f35) into master (f432b2d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #523   +/-   ##
=======================================
  Coverage   82.36%   82.36%           
=======================================
  Files          36       36           
  Lines        1758     1758           
=======================================
  Hits         1448     1448           
  Misses        310      310           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for getting the CI to green and addressing other comments

@willarmiros willarmiros merged commit 9e199b9 into aws:master Aug 29, 2022
@carolabadeer carolabadeer mentioned this pull request May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants