Skip to content

Use HTTP instead of _OTHER in HTTP span names (if method is unknown)#270

Merged
arminru merged 12 commits into
open-telemetry:mainfrom
lmolkova:http-span-name
Sep 11, 2023
Merged

Use HTTP instead of _OTHER in HTTP span names (if method is unknown)#270
arminru merged 12 commits into
open-telemetry:mainfrom
lmolkova:http-span-name

Conversation

@lmolkova

@lmolkova lmolkova commented Aug 17, 2023

Copy link
Copy Markdown
Member

Fixes #268

Changes

When http.request.method is set to _OTHER, use HTTP instead of method in span names.

Merge requirement checklist

@trask

trask commented Aug 17, 2023

Copy link
Copy Markdown
Member

does this fit with https://github.com/open-telemetry/opentelemetry-specification/blob/v1.22.0/specification/trace/api.md#span?

The span name SHOULD be the most general string that identifies a (statistically) interesting class of Spans, rather than individual Span instances while still being human-readable.

EDIT sorry I see you discuss this in the issue

@lmolkova lmolkova closed this Aug 21, 2023
@lmolkova lmolkova reopened this Aug 22, 2023
@lmolkova lmolkova marked this pull request as ready for review August 22, 2023 20:26
@lmolkova lmolkova requested review from a team August 22, 2023 20:26
@lmolkova lmolkova changed the title Use original method as HTTP span name Use HTTP instead of _OTHER in HTTP span names (if method is unknown) Aug 22, 2023
Comment thread docs/http/http-spans.md Outdated

@Oberon00 Oberon00 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like d the suggestion of @trask on the issue to always prepend "HTTP ", even for the route. This will also ensure that HTTP span names don't conflict with other span names from unrelated conventions.

I added that as suggestions to this PR review.

Comment thread docs/http/http-spans.md Outdated
Comment thread docs/http/http-spans.md Outdated
Comment thread docs/http/http-spans.md Outdated
@trask

trask commented Aug 23, 2023

Copy link
Copy Markdown
Member

I liked the suggestion of @trask on the issue to always prepend "HTTP ", even for the route. This will also ensure that HTTP span names don't conflict with other span names from unrelated conventions.

I added that as suggestions to this PR review.

my intention was only to use HTTP in place of _OTHER, as I think known http verbs are generally descriptive enough for http span names without the addition of HTTP

@Oberon00

Copy link
Copy Markdown
Member

@trask Sorry, then I misunderstood you.

Comment thread docs/http/http-spans.md
Comment thread docs/http/http-spans.md Outdated
Comment thread docs/http/http-spans.md Outdated

@joaopgrassi joaopgrassi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm thinking since this is breaking, if we have a way/process to communicate the community/maintainers/sigs about this, so it's not forgotten? It's probably a general challenge we have here since PRs gets merged but I don't think there's a process where changes such as this are communicated to all SIGs.

Also, I'm not even sure if any instrumentation already implemented the _OTHER behavior anyway so might not be that critical.

@joaopgrassi

Copy link
Copy Markdown
Member

Additionally: I think the changes are good, but I wonder if we can get things more "direct/clear" if we split and define proper sections for server and client HTTP span names. Something like:

## Name

HTTP spans MUST follow the overall [guidelines for span names](https://github.com/open-telemetry/opentelemetry-specification/tree/v1.22.0/specification/trace/api.md#span).

### Server

HTTP server span names SHOULD be `{method} {http.route}` if there is a
(low-cardinality) `http.route` available,

If there is no (low-cardinality) `http.route` available, HTTP server span names
SHOULD be `{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.

> Refer to [`method` value requirements]() for its definition

### Client

HTTP client spans have no `http.route` attribute since client-side instrumentation
is not generally aware of the "route". Therefore, HTTP client spans SHOULD be `{method}`.

> Refer to [`method` value requirements]() for its definition

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.

### {`method`} value requirements

The `{method}` MUST be `{http.request.method}` if the method represents the original method known to the instrumentation.
In other cases (when `{http.request.method}` is set to `_OTHER`), `{method}` MUST be `HTTP`.

I can submit a separate PR if people think that's an improvement.

@jsuereth jsuereth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the suggestion of always having HTTP as a prefix.

@trask

trask commented Aug 29, 2023

Copy link
Copy Markdown
Member

I like the suggestion of always having HTTP as a prefix.

just confirming, you would like to revisit open-telemetry/opentelemetry-specification#3165?

@jsuereth

Copy link
Copy Markdown
Contributor

just confirming, you would like to revisit open-telemetry/opentelemetry-specification#3165?

Given history there, no. Only if the idea was new and hand't been considered before.

This is good to merge as-is I think.

@Oberon00

Oberon00 commented Aug 31, 2023

Copy link
Copy Markdown
Member

@jsuereth

Given history there, no. Only if the idea was new and hand't been considered before.

The idea of also prepending HTTP to routes for consistency is new IIUC

@trask

trask commented Sep 1, 2023

Copy link
Copy Markdown
Member

The idea of also prepending HTTP to routes for consistency is new IIUC

This is true.

I'm ok revisiting this, but it seems to me to come back to "what is the purpose of span names?", which to my understanding is two things:

  • primarily: a logical grouping key for spans
  • secondarily: a default display for the span in UIs which don't have deeper knowledge of otel semconv

and I'm not sure that prepending HTTP in front of well-known http methods helps either of these things.

@trask

trask commented Sep 1, 2023

Copy link
Copy Markdown
Member

If we did go with prepending HTTP to span names, would we also prepend RPC, Database, Messaging to those span names? or would the same reasoning for prepending HTTP not apply in those cases?

@lmolkova

lmolkova commented Sep 5, 2023

Copy link
Copy Markdown
Member Author

Agree with @trask that HTTP prefix does not bring substantial benefits and if we want to bring it back, we should follow the same patterns in other semconvs.

If we need a semconv-id and be able to group/visualize based on it, this would arguably go beyond default UX and would be best reported as an instrumentation scope attribute. Adding some sort of semconv-id was discussed around the open-telemetry/oteps#172 and was highly controversial. E.g. AFAIK elasticsearch reports spans that are both HTTP and DB - how they should name spans/report semconv-id for those?

Liudmila Molkova and others added 5 commits September 5, 2023 14:10
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Co-authored-by: Joao Grassi <joao@joaograssi.com>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@lmolkova lmolkova force-pushed the http-span-name branch 3 times, most recently from 30a1bd9 to 9ac8f70 Compare September 5, 2023 21:45
@lmolkova

lmolkova commented Sep 5, 2023

Copy link
Copy Markdown
Member Author

Additionally: I think the changes are good, but I wonder if we can get things more "direct/clear" if we split and define proper sections for server and client HTTP span names. Something like:

I like this idea and I tried it, but I'm not quite happy with the outcome:

The sentence

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.

applies to client and server spans (http.route is not really available in many HTTP server instrumentations).
So it leaves us with 1 sentence specific to client and server and much more shared text.

I added an invisible anchor to {method} and references to it, hope it looks better now.

@joaopgrassi

joaopgrassi commented Sep 6, 2023

Copy link
Copy Markdown
Member

I added an invisible anchor to {method} and references to it, hope it looks better now.

Yep looks much nicer! Thanks for addressing it and good idea with the invisible anchor 🧠

Comment thread package.json
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 span name for unknown methods

7 participants