Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add semantic conventions for HTTP metrics #739
Add semantic conventions for HTTP metrics #739
Changes from all commits
34eb0ba
18a3e67
028d345
6f0e55d
d5d15cc
09116fc
14c0a98
b3e8bf2
7e0c306
4d2ec7f
b87fcb8
e2ce1b3
952d1ae
c27d66d
7b59ac0
1ba7a15
bdcbdd5
7de9d45
d420059
655a490
d607daf
da2a2de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a fan of status_text / reason phrase myself. It is Optional, so I let it lie.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://tools.ietf.org/html/rfc7231#section-6.1 says that
Not that I have seen this actually happening but it is a possibility. If source start sending different reason phrase for the same status code that would arguably make the status text less useful since aggregations would be difficult/impossible to do.
Since we already have
http.status_code
perhaps just drop this label?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh. I highly dislike dealing with reason phrases.
I can see an argument for leaving it in. I have interacted with a REST service that encoded important stuff in the reason phrase (like the difference between a 404 caused by a route that doesn't exist and a 404 because the requested object doesn't exist).
I vote we leave it in. If we do, it will likely result in all HTTP client instrumentation including it. I guess this is good because in the case of meaningful reason codes, it will be possible to include them on the final metric events, and the user can facet or filter on them in the UI.
And in the general case, this label doesn't add any more cardinality (though it does add a few more bytes per instrument).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "flavor" the right term or "version"?
Also, I am not sure "SPDY" or "QUIC" belong to the same set.