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: added captureHTTPsRequest feature #677

Merged
merged 23 commits into from
Apr 5, 2022

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Mar 22, 2022

Description of your changes

This PR introduces a new feature for Tracer that allows to trace, by default, all outgoing HTTP(s) requests:

  • Customers can opt-out from this feature by setting the POWERTOOLS_TRACER_CAPTURE_HTTPS_REQUESTS environment variable or captureHTTPsRequests constructor parameter to false when instantiating the Tracer
  • The feature relies on aws-xray-sdk ability to instrument the standard library's http and https modules, as Tracer should be able to trace any 3rd party request client library that is built upon these two modules. Support to 3rd party HTTP clients is provided on a best effort basis (this disclaimer is specified in the documentation). axios has been tested during development and is included in integration tests.
  • Documentation and JSDocs updated
  • The feature is implemented without exposing any new API on Tracer and directly on the Tracer instance, as such no changes to decorator and middleware implementation has been made
  • Unit tests implemented
  • Integration tests implemented
  • Minor housekeeping changes (mostly formatting)

How to verify this change

See checks passing on this PR, see integration test results (link)

Screenshots of the new section in the documentation:
image

Documentation changes also include the addition of the new config to the Utility Settings section:
image

Screenshots of traces & service map generated by running this build & using axios:
Screenshot 2022-03-22 at 14 37 02
Screenshot 2022-03-22 at 14 36 45

Related issues, RFCs

#675
#275

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published in downstream module
  • The PR title follows the conventional commit semantics

Breaking change checklist

N/A


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@dreamorosi dreamorosi added enhancement tracer This item relates to the Tracer Utility labels Mar 22, 2022
@dreamorosi dreamorosi added this to the production-ready-release milestone Mar 22, 2022
@dreamorosi dreamorosi requested a review from saragerion March 22, 2022 14:28
@dreamorosi dreamorosi self-assigned this Mar 22, 2022
@dreamorosi dreamorosi removed the request for review from saragerion March 22, 2022 14:28
@dreamorosi dreamorosi requested review from flochaz and saragerion and removed request for flochaz March 22, 2022 17:00
@dreamorosi dreamorosi marked this pull request as ready for review March 22, 2022 17:00
@dreamorosi dreamorosi requested a review from flochaz March 22, 2022 17:01
@dreamorosi dreamorosi force-pushed the aamorosi/feat/tracer/add_capture_https branch from 6790faa to c7ab458 Compare March 23, 2022 09:07
@dreamorosi dreamorosi requested a review from ijemmy March 23, 2022 09:15
@flochaz
Copy link
Contributor

flochaz commented Mar 23, 2022

last force push might have removed all your changes ... I can't see diff anymore

@dreamorosi dreamorosi force-pushed the aamorosi/feat/tracer/add_capture_https branch from eb16898 to f5736db Compare March 23, 2022 10:05
@dreamorosi
Copy link
Contributor Author

last force push might have removed all your changes ... I can't see diff anymore

Thanks @flochaz, fixed it and included the changes from #676

@dreamorosi
Copy link
Contributor Author

docs/core/tracer.md Outdated Show resolved Hide resolved
@dreamorosi dreamorosi requested a review from flochaz March 24, 2022 08:24
packages/tracing/tests/e2e/tracer.test.ts Outdated Show resolved Hide resolved
packages/tracing/tests/e2e/tracer.test.ts Outdated Show resolved Hide resolved
packages/tracing/tests/e2e/tracer.test.ts Outdated Show resolved Hide resolved
packages/tracing/tests/e2e/tracer.test.ts Outdated Show resolved Hide resolved
@dreamorosi
Copy link
Contributor Author

Thanks a ton @ijemmy for the review, agree with all your comments and I will address them right away.

@dreamorosi dreamorosi force-pushed the aamorosi/feat/tracer/add_capture_https branch from 54bad38 to 05c20b9 Compare March 31, 2022 15:29
@dreamorosi dreamorosi requested a review from ijemmy March 31, 2022 16:16
flochaz
flochaz previously approved these changes Apr 1, 2022
ijemmy
ijemmy previously approved these changes Apr 1, 2022
@dreamorosi dreamorosi dismissed stale reviews from ijemmy and flochaz via 95cd754 April 1, 2022 11:29
@dreamorosi dreamorosi force-pushed the aamorosi/feat/tracer/add_capture_https branch from 646e5ff to 95cd754 Compare April 1, 2022 11:29
@dreamorosi dreamorosi requested review from ijemmy and flochaz April 1, 2022 11:33
@dreamorosi
Copy link
Contributor Author

Sorry, had to rebase due to merge conflicts :/ Need another round of reviews

flochaz
flochaz previously approved these changes Apr 1, 2022
saragerion
saragerion previously approved these changes Apr 5, 2022
Copy link
Contributor

@saragerion saragerion left a comment

Choose a reason for hiding this comment

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

It looks good to me already, great job!

docs/core/tracer.md Show resolved Hide resolved
docs/core/tracer.md Outdated Show resolved Hide resolved
packages/tracing/src/Tracer.ts Outdated Show resolved Hide resolved
packages/tracing/tests/unit/ProviderService.test.ts Outdated Show resolved Hide resolved
Co-authored-by: Sara Gerion <[email protected]>
@dreamorosi dreamorosi dismissed stale reviews from saragerion and flochaz via b1cac8d April 5, 2022 10:30
@dreamorosi dreamorosi requested review from flochaz and saragerion April 5, 2022 10:31
@dreamorosi dreamorosi merged commit 5a36723 into main Apr 5, 2022
@dreamorosi dreamorosi deleted the aamorosi/feat/tracer/add_capture_https branch April 5, 2022 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracer This item relates to the Tracer Utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants