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

[lmolkova]

Fixes #268

Changes

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

Merge requirement checklist

• CONTRIBUTING.md guidelines followed.
• CHANGELOG.md updated for non-trivial changes.
• schema-next.yaml updated with changes to existing conventions.

[trask]

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

[Oberon00]

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.

[trask]

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]

@trask Sorry, then I misunderstood you.

[joaopgrassi]

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]

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]

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

[trask]

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

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

[jsuereth]

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]

@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]

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]

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]

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?

[lmolkova]

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]

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 🧠