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

Align OTEL http attributes with latest standard #121

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

ev-gor
Copy link
Contributor

@ev-gor ev-gor commented Aug 29, 2024

Closes #120

@ev-gor ev-gor requested a review from a team as a code owner August 29, 2024 09:26
@ev-gor
Copy link
Contributor Author

ev-gor commented Aug 29, 2024

@microsoft-github-policy-service agree

Copy link
Member

@baywet baywet 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 contribution!
I'd encourage you to check in the retry handler which most likely has some of those headers as well.

@ev-gor
Copy link
Contributor Author

ev-gor commented Aug 29, 2024

I've searched the whole project for attributes listed in #121 and replaced them all. Retry handler has retry_count attribute mentioned in #119, but I wanted to create another PR for it, because I'm not sure about implementation of retry_delay attribute. "int duration in seconds before the request was retried" Does it mean number of seconds between every retry or total sum for all retries?

@baywet
Copy link
Member

baywet commented Aug 30, 2024

Thank you for the additional information.

The attributes in the retry handler are both outdated AND different from the fullfill/reject implementations...
We'd need to:

  1. move them to a constant to avoid repetition
  2. update them

The areas I could find

$rejectedSpan->setAttribute('http.retry_count', $retries);

$span->setAttribute('retryCount', $retries);

And to answer your question: retry delay I think it the delay of each retry (not the total delay).

Let us know if you have any additional comments or questions.

@baywet
Copy link
Member

baywet commented Aug 30, 2024

also the status code attribute is missing in this handler.

@ev-gor
Copy link
Contributor Author

ev-gor commented Aug 31, 2024

I added second commit to update resend_count and resend_delay attributes and add status_code attribute in RetryHandler class. If everything is ok, it closes #119

@baywet baywet linked an issue Sep 3, 2024 that may be closed by this pull request
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes!
Can you add a new changelog entry (patch bump, today's date, changed) please?
As well as align the version constant

This will ensure a swift release.

@ev-gor
Copy link
Contributor Author

ev-gor commented Sep 4, 2024

Done.

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes!

@baywet baywet merged commit 14d4411 into microsoft:dev Sep 4, 2024
6 checks passed
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 - align OTEL http attributes with latest standard http - retry handler attributes updates
2 participants