Skip to content

feat: added captureHTTPsRequest feature - #677

Merged
dreamorosi merged 23 commits into
mainfrom
aamorosi/feat/tracer/add_capture_https
Apr 5, 2022
Merged

feat: added captureHTTPsRequest feature#677
dreamorosi merged 23 commits into
mainfrom
aamorosi/feat/tracer/add_capture_https

Conversation

@dreamorosi

@dreamorosi dreamorosi commented Mar 22, 2022

Copy link
Copy Markdown
Contributor

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

flochaz commented Mar 23, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor Author

Comment thread docs/core/tracer.md Outdated
@dreamorosi
dreamorosi requested a review from flochaz March 24, 2022 08:24
Comment thread packages/tracing/tests/e2e/tracer.test.ts Outdated
Comment thread packages/tracing/tests/e2e/tracer.test.ts Outdated
Comment thread packages/tracing/tests/e2e/tracer.test.ts Outdated
Comment thread packages/tracing/tests/e2e/tracer.test.ts Outdated
@dreamorosi

Copy link
Copy Markdown
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 flochaz and ijemmy April 1, 2022 11:33
@dreamorosi

Copy link
Copy Markdown
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

@saragerion saragerion left a comment

Copy link
Copy Markdown
Contributor

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!

Comment thread docs/core/tracer.md
Comment thread docs/core/tracer.md Outdated
Comment thread packages/tracing/src/Tracer.ts Outdated
Comment thread packages/tracing/tests/unit/ProviderService.test.ts Outdated
Co-authored-by: Sara Gerion <47529391+saragerion@users.noreply.github.com>
@dreamorosi
dreamorosi dismissed stale reviews from saragerion and flochaz via b1cac8d April 5, 2022 10:30
dreamorosi and others added 2 commits April 5, 2022 12:30
Co-authored-by: Sara Gerion <47529391+saragerion@users.noreply.github.com>
Co-authored-by: Sara Gerion <47529391+saragerion@users.noreply.github.com>
@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