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(tracer): instrument fetch requests #2293

Merged
merged 12 commits into from
Apr 5, 2024
Merged

feat(tracer): instrument fetch requests #2293

merged 12 commits into from
Apr 5, 2024

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Mar 28, 2024

Description of your changes

This PR introduces to Tracer the ability to instrument and trace requests made with the fetch global module.

Given that the fetch implementation present in Node.js is based on undici, the implementation added in PR uses diagnostics_channel, which according to undici's docs is the preferred way to instrument requests made using the modules.

This implementation - based on a pub/sub model - allows us to completely decouple the tracing logic from the module, which removes the need of monkey patching or wrapping the module with our implementation. The result consists in a series of event handlers that are subscribed to certain channels as soon as the Tracer class is instantiated.

The feature follows the same conventions of the HTTP(s) request tracing, meaning that it's enabled by default and can be disabled by setting either the captureHTTPsRequests constructor parameter or the POWERTOOLS_TRACER_CAPTURE_HTTPS_REQUESTS environment variable to false. As a side note, in v3, we could deprecate these values in favor of a more generic traceOutgoingRequests and POWERTOOLS_TRACER_TRACE_OUTGOING_REQUESTS.

To support the implementation the PR also extends some types of the Subsegment class as well as adding a handful of types and helpers. This opens the door to also replace the instrumentation of the http and https modules with a similar pattern in the future since these modules also publish events to their own diagnostics_channel.

Finally, the PR also includes changes to the docs and to the integration tests so that the new feature is covered in both. For the integration tests I had to introduce a conditional behavior to fall back on axios due to the fetch API not being available in Node.js 16.

A special thanks goes to @RaphaelManke, who suggested this idea and created the first proof of concept for it.

Related issues, RFCs

Issue number: #1619

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
  • My changes generate no new warnings
  • I have added tests that prove my change is effective and works
  • The PR title follows the conventional commit semantics

Breaking change checklist

Is it a breaking change?: NO

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

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

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@dreamorosi dreamorosi self-assigned this Mar 28, 2024
@boring-cyborg boring-cyborg bot added tracer This item relates to the Tracer Utility tests PRs that add or change tests labels Mar 28, 2024
@pull-request-size pull-request-size bot added the size/L PRs between 100-499 LOC label Mar 28, 2024
@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Mar 28, 2024
@dreamorosi dreamorosi linked an issue Mar 28, 2024 that may be closed by this pull request
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Apr 2, 2024
@dreamorosi
Copy link
Contributor Author

Integration tests passing on all managed runtimes: https://github.com/aws-powertools/powertools-lambda-typescript/actions/runs/8522716494

@dreamorosi dreamorosi marked this pull request as ready for review April 2, 2024 12:39
@dreamorosi dreamorosi requested review from a team as code owners April 2, 2024 12:39
@RaphaelManke
Copy link

What about the error channel? Do you think the "headers" channel who'll be called when there is an error like request timeout or hang up ? I think in case of an error the segment needs to be closed at least.

@dreamorosi
Copy link
Contributor Author

What about the error channel? Do you think the "headers" channel who'll be called when there is an error like request timeout or hang up ? I think in case of an error the segment needs to be closed at least.

Good catch, I only tested for cases where the server returns a 4xx or 5xx response which is handled by the onResponse handler. I am adding an additional handler for onError which will cover cases where the client is unable to establish a connection.

@pull-request-size pull-request-size bot added size/XL PRs between 500-999 LOC, often PRs that grown with feedback and removed size/L PRs between 100-499 LOC labels Apr 3, 2024
@dreamorosi
Copy link
Contributor Author

@RaphaelManke I have added additional logic to handle the case and close the subsegment + restore the parent.

Thank you again for spotting this!

Copy link
Contributor

@am29d am29d left a comment

Choose a reason for hiding this comment

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

Great work Andrea!

Small ask, otherwise it's good to merge.

We already have fetch in our examples, let's remove the rest of axios in the batch readme.

packages/tracer/tests/unit/random.test.ts Outdated Show resolved Hide resolved
@dreamorosi
Copy link
Contributor Author

We already have fetch in our examples, let's remove the rest of axios in the batch readme.

I found another file that also uses axios in the batch package that is not used at all, plus I found a few line highlight that are wrong in the Batch page - if you're okay I'll open a separate issue and tackle all these there, so we keep this PR focused.

@dreamorosi dreamorosi requested a review from am29d April 5, 2024 10:06
@am29d
Copy link
Contributor

am29d commented Apr 5, 2024

We already have fetch in our examples, let's remove the rest of axios in the batch readme.

I found another file that also uses axios in the batch package that is not used at all, plus I found a few line highlight that are wrong in the Batch page - if you're okay I'll open a separate issue and tackle all these there, so we keep this PR focused.

Sounds good to me, removing random.test.ts and we are good to merge

@dreamorosi
Copy link
Contributor Author

@am29d - I removed the file by pushing an empty comment, not sure what happened.

Should now be fine.

Copy link

sonarcloud bot commented Apr 5, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
23.2% Duplication on New Code

See analysis details on SonarCloud

@am29d am29d merged commit cc34400 into main Apr 5, 2024
10 checks passed
@am29d am29d deleted the feat/tracer_fetch branch April 5, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature PRs that introduce new features or minor changes size/XL PRs between 500-999 LOC, often PRs that grown with feedback tests PRs that add or change tests tracer This item relates to the Tracer Utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: support for tracing NodeJs 18 native fetch calls
3 participants