feat(tracing): add body size for fetch requests #7935
Conversation
size-limit report 📦
|
| @@ -195,6 +197,7 @@ export function fetchCallback( | |||
| data: { | |||
| ...handlerData.fetchData, | |||
| type: 'fetch', | |||
| ...(contentLength ? { 'Encoded Body Size': contentLength } : {}), | |||
There was a problem hiding this comment.
h: I actually think we should try and deprecate and move away from Encoded Body Size - having a key with capitals + spaces seems like an anti-pattern, can we try to rename this and update the sentry code accordingly?
There was a problem hiding this comment.
@AbhiPrasad Is there a convention for params like this? camel/snake case? I'm taking over the rollout of this perf issue so I'll make the changes
There was a problem hiding this comment.
https://develop.sentry.dev/sdk/performance/span-data-conventions/ is the list.
Keys on the data field should be lower-case and use underscores instead of camel-case. There are some exceptions to this, but these exist because of backwards compatibility.
. separators are also used.
There was a problem hiding this comment.
Hmm, I made a change to switch to encoded_body_size here, but I noticed that other perf issues also depend on values in this data object such as "Transfer Size", "Encoded Body Size", and "Decoded Body Size". Do you know if these keys would count as exceptions to the rule? It feels like renaming here would be inconsistent
There was a problem hiding this comment.
those keys would be exceptions - and we would update the develop docs accordingly.
Ideally all sentry code would update to reference and use the new keys cc @k-fish.
There was a problem hiding this comment.
No perf detector should depend on it atm
I was mistaken here - I thought the perf detectors would restrict to resource spans since that only sends data atm, but from reading source code I guess http works as well.
There was a problem hiding this comment.
also @AbhiPrasad I don't think we ever changed to snake_case we just talked about it right? it's bothered me as well but I'm not sure we made plans to change encoded/decoded to snake yet.. or did we and I forget?
There was a problem hiding this comment.
otel now uses snake case so I think we try and match that (for ex: https://github.com/open-telemetry/opentelemetry-specification/pull/3287/files).
There was a problem hiding this comment.
Also, should it be http.response_content_length? That seems to be what OTel is suggesting
There was a problem hiding this comment.
yeah @gggritso that makes sense to me. @narsaynorath could we first open a PR on develop to https://github.com/getsentry/develop to establish the naming, and then update both sentry/sdks?
…47830) Replaces usages of "Encoded Body Size", "Decoded Body Size", "Transfer Size" with their snake case counterparts. This is for standardizing with OTel formatting in SDKs. Motivating discussion: getsentry/sentry-javascript#7935 (comment)
|
@narsaynorath reminder we squash merge all of our commits! |
|
Hey @AbhiPrasad, were there any other changes you needed from me here? |
|
Looks like we need to fix some conflicts, then we should be good to go to merge. |
AbhiPrasad
changed the title
|
Our CI is broken - please bear with us while we fix. I can take care of merging this in after we do that! |
|
Took the liberty to update to develop and change a test - will merge in! |
Co-authored-by: Nar Saynorath <nar.saynorath@sentry.io> Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
Drafting this for now, after discussing this, we couldn't find a way to get body size easily from the performance timing api, so instead we're retrieving this from the content-length header, this represents the body size of the encoded response.
We already document the
encoded body size, so I decided to reuse that name as to not cause confusion.