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

trace/http: Add http.{request,response}.size attributes #84

Merged
merged 4 commits into from
Mar 18, 2024

Conversation

kamalmarhubi
Copy link
Contributor

These attributes are the number of bytes in the request or response, including headers. The names were chosen by taking the ECS HTTP fields and replacing bytes with size, as for http.request.content.size.

closes #38

@kamalmarhubi kamalmarhubi requested review from a team June 6, 2023 00:21
@trask
Copy link
Member

trask commented Jun 6, 2023

hi @kamalmarhubi! just a heads up, the HTTP semantic convention WG isn't considering any additions until the HTTP semantic conventions have at least been marked Frozen, so it may be a few weeks until we get to reviewing this proposal

@kamalmarhubi
Copy link
Contributor Author

@trask no worries. Should I hold off opening other PRs I might have if they affect one of the changed namespaces?

@trask
Copy link
Member

trask commented Jun 6, 2023

I think it's fine to continue opening PRs, it's possible that our stale bot might try to close it, but that's ok, just ping to make it not stale, or if it gets closed we can re-open it

@jsuereth
Copy link
Contributor

@kamalmarhubi Now that early HTTP semconv stability is acheived, I think we can start to entertain this again. Would you be willing to rebase this PR?

If not, we may close as stale and you can feel free to re-open a new PR against the current main branch.

@gregkalapos
Copy link
Member

I'd be happy to try to move this forward. @kamalmarhubi do you plan to rebase this, or should I open a new one?

@kamalmarhubi
Copy link
Contributor Author

kamalmarhubi commented Feb 8, 2024

@gregkalapos I'll rebase and push this tomorrow, sorry for the radio silence

@kamalmarhubi kamalmarhubi requested a review from a team February 9, 2024 15:56
@kamalmarhubi
Copy link
Contributor Author

sorry didn't get this pushed yesterday. I thought it would be a quick rebase but all files had moved around and the tooling had changed. just pushed, including a clarification on over-the-wire vs uncompressed.

model/registry/http.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@trisch-me trisch-me left a comment

Choose a reason for hiding this comment

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

The text is different for md and yaml files. This needs to be updated

@kamalmarhubi kamalmarhubi force-pushed the http-request-response-size branch 2 times, most recently from 926ff07 to 93d3a60 Compare February 12, 2024 17:32
@kamalmarhubi
Copy link
Contributor Author

The text is different for md and yaml files. This needs to be updated

🤦
@trisch-me fixed

model/registry/http.yaml Show resolved Hide resolved
model/registry/http.yaml Show resolved Hide resolved
model/registry/http.yaml Outdated Show resolved Hide resolved
model/registry/http.yaml Outdated Show resolved Hide resolved
These attributes are the number of bytes in the request or response,
including control data, headers, body, and trailers. The names were
chosen by taking the [ECS HTTP fields] and replacing `bytes` with
`size`, as for `http.request.content.size`.

closes open-telemetry#38

[ECS HTTP fields]: https://www.elastic.co/guide/en/ecs/current/ecs-http.html
@kamalmarhubi
Copy link
Contributor Author

just pushed this again with some rewording.

Copy link
Contributor

@trisch-me trisch-me left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

Look great, thanks a lot for pushing through and keeping it up to date @kamalmarhubi

I left a couple of naming suggestions, let me know what you think.

model/registry/http.yaml Show resolved Hide resolved
model/registry/http.yaml Show resolved Hide resolved
@lmolkova lmolkova merged commit 0220d78 into open-telemetry:main Mar 18, 2024
10 checks passed
ChrsMark pushed a commit to ChrsMark/semantic-conventions that referenced this pull request Mar 21, 2024
…ry#84)

Co-authored-by: Liudmila Molkova <[email protected]>
Co-authored-by: Alexandra Konrad <[email protected]>
Co-authored-by: Joao Grassi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

trace/http: Add semantic convention for total request and response length
9 participants