-
Notifications
You must be signed in to change notification settings - Fork 792
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
e349fa4
656cbc4
538e712
eaf9786
d6149ca
d97b02b
860557e
9dfe663
a14c1f9
499c8ca
19890b5
bee76c8
854cc53
24e3b46
12c42e3
91637aa
08ab734
8d3c533
bf92869
f21eb54
102d128
a0a26ea
8b94dbc
3569096
d011829
178e73b
74cb0ea
7c2bfe2
51cc125
f0dcca9
192f440
0007def
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -244,9 +244,7 @@ | |
} | ||
|
||
if (body instanceof FormData) { | ||
// typescript doesn't like it when we pass FormData into URLSearchParams | ||
// even though this is actually totally valid | ||
return getByteLength(new URLSearchParams(body as any).toString()); | ||
return getFormDataSize(body); | ||
} | ||
|
||
if (body instanceof URLSearchParams) { | ||
|
@@ -266,6 +264,19 @@ | |
return TEXT_ENCODER.encode(s).byteLength; | ||
} | ||
|
||
function getFormDataSize(formData: FormData): number { | ||
let size = 0; | ||
for (const [key, value] of formData.entries()) { | ||
Check failure on line 269 in packages/opentelemetry-sdk-trace-web/src/utils.ts GitHub Actions / browser-tests
Check failure on line 269 in packages/opentelemetry-sdk-trace-web/src/utils.ts GitHub Actions / build
Check failure on line 269 in packages/opentelemetry-sdk-trace-web/src/utils.ts GitHub Actions / node-tests (14)
Check failure on line 269 in packages/opentelemetry-sdk-trace-web/src/utils.ts GitHub Actions / node-windows-tests
Check failure on line 269 in packages/opentelemetry-sdk-trace-web/src/utils.ts GitHub Actions / node-tests (16)
Check failure on line 269 in packages/opentelemetry-sdk-trace-web/src/utils.ts GitHub Actions / webworker-tests
Check failure on line 269 in packages/opentelemetry-sdk-trace-web/src/utils.ts GitHub Actions / node-tests (18)
Check failure on line 269 in packages/opentelemetry-sdk-trace-web/src/utils.ts GitHub Actions / node-tests (20)
|
||
size += key.length; | ||
if (value instanceof Blob) { | ||
size += value.size; | ||
} else { | ||
size += value.length; | ||
} | ||
} | ||
return size; | ||
} | ||
Comment on lines
+268
to
+279
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an improvement over the old implementation (which was stringifying the FormData, and that wouldn't actually stringify file objects correctly), but it still doesn't calculate the exact FormData size. Browsers will serialize FormData in their own way (ex including a separator for each of the fields, etc.) I will keep investigating for more accurate approaches. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this seems good enough for now, and maybe if anyone is sufficiently motivated they can contribute the changes to calculate it more precisely for their platform 😬 |
||
|
||
MustafaHaddara marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* sort resources by startTime | ||
* @param filteredResources | ||
|
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.
Sorry for the 👻 here, just been a bit busy with life and quitting #dayjob!
Just noticed, should these also use
getByteLength
?Feels silly to suggest it given that
FormData
size varies (as you mentioned, browser/platform-specific implementation differences from things like boundaries and such, I just checked out Firefox for example), so I understand if it seems unnecessary at this point since it'd just be shaving a tiny bit of hypothetical inaccuracy off an estimate that will inherently be incorrect under the circumstances that it applies to (and feel free to ignore).