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: patch fetch with tracing #954

Merged
merged 1 commit into from
May 10, 2023
Merged

feat: patch fetch with tracing #954

merged 1 commit into from
May 10, 2023

Conversation

xplosunn
Copy link
Contributor

@xplosunn xplosunn commented May 5, 2023

Patches 'fetch' with tracing.

When running tests I found out that sometimes the patching was applied multiple times. Not sure if it's specific to testing or something that can also happen in production code, so I added a "patched" variable to track it.

@xplosunn xplosunn requested a review from a team May 5, 2023 10:02
@jonbretman
Copy link
Member

I don't think this will be hugely useful as it won't track the full lifecycle of the request. There is some otel instrumentation for fetch which is intended for web-use but maybe is worth trying? https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/opentelemetry-instrumentation-fetch I can't see anything particularly web-specific.

If it doesn't "just work" then maybe we can port that file to be node compatible

@jonbretman
Copy link
Member

Key things we want to capture:

  • URL being requested
  • HTTP method
  • Response status
  • Request time

@xplosunn
Copy link
Contributor Author

xplosunn commented May 5, 2023

@jonbretman that package unfortunately only works on the browser. I'll try to port it over.

@xplosunn
Copy link
Contributor Author

xplosunn commented May 8, 2023

@jonbretman here's what I tried:

  1. Adding the global browser fields that library depends on. Somehow the spans were silently not finishing.
  2. Copy that file (and related files) but drop the dependency on opentelemetry-sdk-trace-web. Turns out everything is built on top of that package, so there's no point.

So I ended up digging in the source code an copy-pasting the parts that seemed like things we want, including what you listed, apart from "Request time". The logic for that is a few hundred lines of code. It has a bunch of stuff for observing some events and filtering to get to the relevant times.

Is it worth bringing that over or is this an acceptable version for now?

Copy link
Contributor

@adaam2 adaam2 left a comment

Choose a reason for hiding this comment

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

LGTM 🌈

@jonbretman jonbretman merged commit d96ca0a into main May 10, 2023
@jonbretman jonbretman deleted the patch-fetch branch May 10, 2023 11:07
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.

3 participants