-
Notifications
You must be signed in to change notification settings - Fork 888
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
Move http to main spec #82
Move http to main spec #82
Conversation
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.
Left a few comments
Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>
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'd suggest to keep it unchanged from the existing names and rename status
to status_code
. Is there some objection with this?
Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>
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.
please change the name to status_code
and remove pieces from OT and OC docs to see a diff. Otherwise looks OK
Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>
For a HTTP client span, `SpanKind` MUST be `Client`. | ||
|
||
Given an [RFC 3986](https://www.ietf.org/rfc/rfc3986.txt) compliant URI of the form | ||
`scheme:[//authority]path[?query][#fragment]`, the span name of the span SHOULD |
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.
-1
in OpenTracing we always explicitly recommended against doing that. In the REST services it is very common to have entity IDs as part of the path, e.g. /api/user/{uuid}. And since the instrumentation in general has no knowledge of whether the application's URL pattern includes entity IDs, recommending path as span name can easily lead to cardinality explosion.
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.
---> #270
* Moved HTTP Client to semantic conventions * Moved HTTP server * Update semantic-conventions.md Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com> * Simplified attribute names. * Added note on using lower cardinality names if a framework can infer them. * Update semantic-conventions.md Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com> * Cahnge status to status_code * Add component * Update semantic-conventions.md Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com> * Added optional status_text attribute * Removed attributes covered in the main spec from OC and OT work in progress files.
Signed-off-by: Bogdan Cristian Drutu <bogdandrutu@gmail.com>
Signed-off-by: Bogdan Cristian Drutu <bogdandrutu@gmail.com>
Resolves #24