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

Track request body size in XHR and Fetch instrumentations #4706

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

MustafaHaddara
Copy link
Contributor

@MustafaHaddara MustafaHaddara commented May 14, 2024

Which problem is this PR solving?

The Fetch and XHR instrumentations expose http.response_content_length attributes but do not expose http.request_content_length attributes. This PR adds the http.request_content_length attributes to outgoing requests that have a body (ex. POST, PATCH, DELETE, etc.)

Short description of the changes

The first few commits (a7b8c04..d7e7368) are refactorings/updates, mainly to unit tests, to enable changes and tests that follow.

The primary changes are contained in these 3 commits:

  • 6001e90 adds getXHRBodyLength and getFetchBodyLength utils to the opentelemetry-sdk-trace-web package.
    • getFetchBodyLength needs to call getXHRBodyLength, otherwise I would have defined these in their respective packages.
    • 21bd673 calls getXHRBodyLength from the XHR instrumentation package and adds unit tests for the XHR instrumentation
    • 373b50b calls getFetchBodyLength from the Fetch instrumentation package and adds unit tests for the Fetch instrumentation

Detailed Description

The getXHRBodyLength function is mostly straightforward; the XHR API is not too complicated and is fairly self-explanatory.

On the other hand, the getFetchBodyLength function is more complex. The root of the problem is that the fetch API doesn't expose clean ways for us to get the body content. In places where it is possible, it is often consumable only once, and often as aPromise that resolves to the body content. I had to take care to not consume the actual body content; we do not want this instrumentation to interfere with actual requests. It is possible that a bug in this implementation would result in the bodies on fetch requests getting consuming by this instrumentation and then not actually included in the network request.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Added unit tests to opentelemetry-sdk-trace-web, opentelemetry-instrumentation-xml-http-request, and opentelemetry-instrumentation-fetch

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@MustafaHaddara MustafaHaddara requested a review from a team as a code owner May 14, 2024 16:00
@MustafaHaddara MustafaHaddara force-pushed the request-body-size branch 2 times, most recently from d32ebea to e262ad9 Compare May 14, 2024 19:41
// can't read a ReadableStream without destroying it
// we CAN "tee" the body stream, which lets us split it, but that still locks the original stream
// so we end up needing to return one of the forks.
const [bodyToReturn, bodyToConsume] = body.tee();
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: tee (may) not exist in all supported runtimes (added in 2022), so there should be some code which checks for the presence of this capability to avoid failures when running in an older runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would clone help, similar to what's done here?

So the Request object has a clone() method on it, which is what I use in the else block of getFetchBodyLength-- https://github.com/open-telemetry/opentelemetry-js/pull/4706/files#diff-52544e5ffe6a8d0257ea6d658d0aae0f70f99b650cf123392e7f6a8e6eb236bbR204-R212

Unfortunately, the ReadableStream object does not have a clone method on it, so if we receive that instead of a Request object, we have to take this approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: tee (may) not exist in all supported runtimes (added in 2022), so there should be some code which checks for the presence of this capability to avoid failures when running in an older runtime.

Thanks for calling this out @MSNev! Did you see any other APIs that might be like this? What's the best way for me to check this?

// can't read a ReadableStream without destroying it
// we CAN "tee" the body stream, which lets us split it, but that still locks the original stream
// so we end up needing to return one of the forks.
const [bodyToReturn, bodyToConsume] = body.tee();
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 91.05%. Comparing base (2610122) to head (0988e15).
Report is 50 commits behind head on main.

Current head 0988e15 differs from pull request most recent head a463c02

Please upload reports for the commit a463c02 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4706      +/-   ##
==========================================
+ Coverage   90.77%   91.05%   +0.27%     
==========================================
  Files          90       89       -1     
  Lines        1930     2000      +70     
  Branches      417      431      +14     
==========================================
+ Hits         1752     1821      +69     
- Misses        178      179       +1     
Files Coverage Δ
packages/opentelemetry-sdk-trace-web/src/utils.ts 72.41% <90.00%> (+5.01%) ⬆️

... and 3 files with indirect coverage changes

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.

None yet

3 participants