Skip to content
This repository was archived by the owner on Aug 14, 2024. It is now read-only.

feat(perf-issues): Convert Body Size params into snake_case forms #920

Merged
merged 3 commits into from
Apr 27, 2023

Conversation

narsaynorath
Copy link
Member

@narsaynorath narsaynorath commented Apr 25, 2023

We're proposing to change the names of these non-standard keys ("Encoded Body Size", "Decoded Body Size", and "Transfer Size") to be more aligned with OTel's standards. A spec doc here references http.response_content_length, which I am adding for the Giant Payload detector, but I am using resource.encoded_body_size and resource.decoded_body_size for the old values.

@narsaynorath narsaynorath requested review from AbhiPrasad and a team April 25, 2023 18:45
@vercel
Copy link

vercel bot commented Apr 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
develop ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 26, 2023 2:11pm

@narsaynorath
Copy link
Member Author

I'd appreciate some feedback on how to handle differentiating encoded/decoded here. The OTel spec linked in the description mentions:

For requests using transport encoding, this should be the compressed size.

which I'm interpreting as: the field can support both encoded and decoded values, but we need both at the same time for our detectors.

| `url` | string | The URL of the resource that was fetched. | `https://example.com` |
| `method` | string | The HTTP method used. | `GET` |
| `type` | string | The type of the resource that was fetched. | `xhr` |
| `http.encoded_response_content_length` | number | The encoded body size of the request. | `123` |
Copy link
Member

Choose a reason for hiding this comment

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

resource.encoded_body_size etc. yeah @AbhiPrasad ? You'll also want to change that content length header from encoded_body_size to http.response_content_length as well I imagine.

Copy link
Member

@AbhiPrasad AbhiPrasad Apr 26, 2023

Choose a reason for hiding this comment

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

I think we can actually just use http.X for everything here - since it is just http information. We can tell the difference by the operation (resource.X vs. http.X) if we want more granular analysis.

To match otel I think we use http.request_content_length to represent the encoded body size, and introduce http.decoded_request_content_length to represent the decoded body size.

We should still indicate that what these fields used to be called for reference (since older SDKs will keep sending this info).

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I've called out the original names in the descriptions and used http.request_content_length, http.decoded_request_body_length, and http.transfer_size.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Thanks for going the extra mile @narsaynorath!

@narsaynorath narsaynorath marked this pull request as ready for review April 26, 2023 14:45
@AbhiPrasad AbhiPrasad merged commit ed2acb1 into master Apr 27, 2023
@AbhiPrasad AbhiPrasad deleted the nar/feat/update_perf_span_attributes_to_snake_case branch April 27, 2023 14:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants