-
Notifications
You must be signed in to change notification settings - Fork 100
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
Update HTTP span names to include method and route #143
Update HTTP span names to include method and route #143
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.
Looks good. Suggested a small change to changelog entry 👍🏻
Co-authored-by: Mike Goldsmith <[email protected]>
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.
LGTM
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.
This made me realize our span names contain high-cardinality paths which is not specification compliant:
HTTP spans MUST follow the overall guidelines for span names. HTTP server span names SHOULD be
{http.request.method} {http.route}
if there is a (low-cardinality)http.route
available. HTTP server span names SHOULD be{http.method}
if there is no (low-cardinality)http.route
available. HTTP client spans have nohttp.route
attribute since client-side instrumentation is not generally aware of the "route", and therefore HTTP client spans SHOULD use{http.method}
. Instrumentation MUST NOT default to using URI path as span name, but MAY provide hooks to allow custom logic to override the default span name.
I think we will need a follow on issue/PR for this.
Co-authored-by: Tyler Yahn <[email protected]>
|
PR open-telemetry#143 updated these span names to include HTTP method per the spec.
PR open-telemetry#143 updated these span names to include HTTP method per the spec.
PR open-telemetry#143 updated these span names to include HTTP method per the spec.
PR open-telemetry#143 updated these span names to include HTTP method per the spec.
* wip: experiments with BATS * add a bats suite for gin instrumentation * ensure CI runner has bats installed * workflow YAML is not shell 🤦 * test: add assert_not_empty helper * more assertions for nethttp * more assertions for gorillamux spans currently show net/http, will need review for lib name * more assertions for gin * update http expectations after span name update PR #143 updated these span names to include HTTP method per the spec. * keep traces.json around + remove before verifying with e2e assertions + bring back redaction-jq as a function within the BATS utilities + assert no git difference with a BATS test * need to lookup gorillamux as net/http for now * update test for gorillamux spans now present --------- Co-authored-by: JamieDanielson <[email protected]>
Current semantic conventions state "HTTP server span names SHOULD be
{http.method} {http.route}
".This PR adjusts http span names from
{http.route}
to{http.method} {http.route}
. Also updated are thetraces.json
files to show the new name.