Track request body size in XHR and Fetch instrumentations

[MustafaHaddara]

trafficstars

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

Ideally, there would be some browser API would could just read for this (similar to how we get the response content length via the PerformanceObserver API). However, no such API exists.

Second best would be if we could read the content-length request header. Unfortunately, the XMLHTTPRequest API does not offer any way to read request headers. Even if we could (ie. with the fetch API), this header seems to be set automatically by the browser before it actually sends the request, outside of user-space.

So, we have to compute the body length on our own. This PR implements that.

Detailed Description

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

The primary changes are contained in these 3 commits:

• d6149caa80218021e98c4408b11ba2721536cc6c 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.
• d97b02b5536c509efc7b01a164ebac9e657b276d calls getXHRBodyLength from the XHR instrumentation package and adds unit tests for the XHR instrumentation
• 860557ef88232e4448ab3b8530119078da1731b8 calls getFetchBodyLength from the Fetch instrumentation package and adds unit tests for the Fetch instrumentation
• bee76c802df479a299404ed2f0299b8911ce2290 makes this functionality opt-in

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

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

How Has This Been Tested?

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

Checklist:

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

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.17%. Comparing base (eb3ca4f) to head (1c26261).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4706      +/-   ##
==========================================
- Coverage   93.18%   93.17%   -0.02%     
==========================================
  Files         315      315              
  Lines        8086     8086              
  Branches     1617     1617              
==========================================
- Hits         7535     7534       -1     
- Misses        551      552       +1     

see 1 file with indirect coverage changes

[MustafaHaddara]

@scheler @MSNev I've made the changes we discussed and resolved merge conflicts. Please let me know if you have any other questions.

[JamieDanielson]

FYI @open-telemetry/javascript-approvers I've labeled this as a spec feature since the attribute is part of http semantic conventions. There will be semantic convention changes with the upcoming updates to SemConv but since this attribute is not yet stable it doesn't fall under the same category as those included in the dual emit http/dup.

[MustafaHaddara]

@JamieDanielson

It could change to http.request.body.size in an upcoming SemConv update, but may not since that attribute is not yet stable. TBD I think. I'm good with the attribute as-is in this PR.

I went ahead and used the new (unstable) attribute; let me know if you want me to revert that.

@tbrockman @MSNev @JamieDanielson I've applied all of the requested changes and this PR should be ready for another review. Let me know if I missed anything.

[JamieDanielson]

from comment thread above, moving here for easier finding:

In the semantic conventions tooling meeting this morning a couple of things came up which affect what this PR should be doing

• The http.request_content_length header is actually deprecated in favor of http.request.body.size in v1.21.0
• The http.request.body.size is listed as experimental with a requirement level of Opt-In which implies that all usages SHOULD be behind an experimental flag and not added by default.

Follow-up notes from JS SIG meeting discussion:

• This PR just uses http.request.body.size since the previous version http.request_content_length didn't exist previously anyway, which should be fine.
• This PR does have an opt-in mechanism, and it is disabled by default.
• Because this opt-in mechanism for certain http attributes includes more than just this attribute and this instrumentation, it may be preferred to move this configuration flag elsewhere so it can be shared by multiple instrumentations. For example, Java added an experimental-opt-in flag that includes these.

@dyladan @MSNev did I capture that correctly? And if so, what are the next steps here? I suspect we'll keep much of this logic already written in this PR and just change the enable/disable mechanism.

[MustafaHaddara]

@dyladan @MSNev I just wanted to follow up on @JamieDanielson's message above-- what would you like me to do with this PR?

[MustafaHaddara]

We discussed this PR at the client-side SIG yesterday. The two remaining discussion threads:

from @JamieDanielson:

Because this opt-in mechanism for certain http attributes includes more than just this attribute and this instrumentation, it may be preferred to move this configuration flag elsewhere so it can be shared by multiple instrumentations. For example, Java added an experimental-opt-in flag that includes these.

My position is that we can update the opt-in mechanism in the future, if we decide to go that route and build one flag to control multiple instrumentations.

As for @tbrockman 's memory concerns:

But I also wouldn't underestimate the potential for people to send large JSON payloads (either intentionally or unintentionally).

[...]

Given some of the other inherent issues for calculating content length size with FormData, would you be open to allowing users to specify their own optional getXHRBodyLength (or maybe exposed as calculateBodyLength) function?

I'm not too worried about memory concerns since this entire instrumentation is opt-in, and I think making the suggested change is something that we can discuss in a follow up issue or PR.