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

Clarify the HTTP client http.retry_count spec #2743

Merged
merged 11 commits into from
Oct 18, 2022

Conversation

mateuszrzeszutek
Copy link
Member

Fixes #2737

Changes

Clarifies when the http.retry_count attribute should be added to the HTTP client span (every time the HTTP request gets re-sent, no matter what the cause is).

I also added a grouping "Examples" header for all examples, similar to other (messaging, DB) convention docs.

semantic_conventions/trace/http.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/http.yaml Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
Copy link
Member

@joaopgrassi joaopgrassi 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 the extra examples and for better formatting them :)

I just have a question on naming. The attribute is retry but in the text we use the word re-send/sent. Not sure if it's a problem but I wonder if we shouldn't be consistent?

semantic_conventions/trace/http.yaml Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
@mateuszrzeszutek
Copy link
Member Author

I just have a question on naming. The attribute is retry but in the text we use the word re-send/sent. Not sure if it's a problem but I wonder if we shouldn't be consistent?

I'm in favor of this idea; initially I didn't want to change too much of the spec, just clarify things, but if more people than just me think it's a valid point I think we could rename it to resend_count.
(I also mentioned this in #2743 (comment))

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 30, 2022
@mateuszrzeszutek
Copy link
Member Author

@open-telemetry/specs-approvers @open-telemetry/technical-committee please take a look at this PR

cc @lmolkova (sorry, I completely forgot to tag you before 🙈)

@github-actions github-actions bot removed the Stale label Aug 31, 2022
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.

LGTM, thank you @mateuszrzeszutek!

@joaopgrassi
Copy link
Member

joaopgrassi commented Sep 7, 2022

Sorry, forgot to hit send in one comment: You probably need to add an entry to the changelog for this as well, otherwise looks good to me.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Sep 17, 2022
@trask
Copy link
Member

trask commented Sep 17, 2022

/unstale

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

schemas/1.13.0 Outdated Show resolved Hide resolved
@mateuszrzeszutek mateuszrzeszutek force-pushed the clarify-http-retries branch 2 times, most recently from 560a1d3 to 4a448db Compare October 5, 2022 07:01
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 13, 2022
@mateuszrzeszutek
Copy link
Member Author

Ping @open-telemetry/technical-committee : is this PR ready to be merged? Is there anything else that I can help with?

@github-actions github-actions bot removed the Stale label Oct 14, 2022
@reyang reyang merged commit 8029345 into open-telemetry:main Oct 18, 2022
@mateuszrzeszutek mateuszrzeszutek deleted the clarify-http-retries branch October 19, 2022 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP retry spec clarification needed
9 participants