-
Notifications
You must be signed in to change notification settings - Fork 153
feat(tracer): instrument fetch requests #2293
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
Conversation
Integration tests passing on all managed runtimes: https://github.com/aws-powertools/powertools-lambda-typescript/actions/runs/8522716494 |
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 |
@RaphaelManke I have added additional logic to handle the case and close the subsegment + restore the parent. Thank you again for spotting this! |
There was a problem hiding this 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.
I found another file that also uses |
Sounds good to me, removing |
@am29d - I removed the file by pushing an empty comment, not sure what happened. Should now be fine. |
|
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 onundici
, the implementation added in PR usesdiagnostics_channel
, which according toundici
'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 thePOWERTOOLS_TRACER_CAPTURE_HTTPS_REQUESTS
environment variable tofalse
. As a side note, in v3, we could deprecate these values in favor of a more generictraceOutgoingRequests
andPOWERTOOLS_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 thehttp
andhttps
modules with a similar pattern in the future since these modules also publish events to their owndiagnostics_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 thefetch
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
Breaking change checklist
Is it a breaking change?: NO
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.