-
Notifications
You must be signed in to change notification settings - Fork 226
feat: HTTP Client Semconv v1.17 Span Naming #1788
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
base: main
Are you sure you want to change the base?
feat: HTTP Client Semconv v1.17 Span Naming #1788
Conversation
14b1c09 to
49c6dc0
Compare
49c6dc0 to
9c2d5b8
Compare
|
@arielvalentin overall looks great! The one thing that caught my eye are the edits for For this PR, we are changing old, but old spec compliant, spans: Should we be updating these names? |
|
@kaylareopelle OK so you are saying we want to stick with the spec v1.17 or earlier for old and then v1.18+ for dup and new? |
|
@arielvalentin yes! |
|
@kaylareopelle Any reason why we would not want to support v1.18+? |
|
@kaylareopelle @hannahramadan I have rolled back the changes to v1.17 span names. I can keep renaming spans myself in the collector but I would be keen on getting old span names to v1.18 because it would reduce the bytes we transfer from the SDK and reduce the processing and span renaming in our collectors. |
It corrects the span name for the old semantic conventions where the name should be set to the HTTP Method or the literalHTTPas well as adds theurl.templateto the name when using stable conventions.More explicitly stated, when usingoldconventions the span name will be one of the known HTTP method (GET,PUT,POST,PATCH,DELETE...) orHTTPif the method is non-standard.When using
duporstableconventions the name may contain aurl.templatevalue if it is known at the time or the request. Users may provide aurl.templatevia theClientContextattributes.Some concessions:
See #1779
Notice to reviewers
This is a follow up to #1781