Skip to content
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

Is extra complexity of the net.app.protocol.name and net.app.protocol.version attributes worth it? #3215

Closed
trask opened this issue Feb 15, 2023 · 7 comments · Fixed by #3272
Assignees
Labels
area:semantic-conventions Related to semantic conventions semconv:HTTP spec:trace Related to the specification/trace directory

Comments

@trask
Copy link
Member

trask commented Feb 15, 2023

I find the recent-ish net.app.protocol.name and net.app.protocol.version attributes confusing from the perspective of someone not deeply ingrained in 7-layer osi networking model.

I just want to make sure the extra complexity of these attributes is worth it, over the simpler net.protocol.name and net.protocol.version.

FWIW, ECS has network.protocol, which is defined as "Application Layer protocol":

In the OSI Model this would be the Application Layer protocol. For example, http, dns, or ssh.

The field value must be normalized to lowercase for querying.

type: keyword

example: http

@trask trask added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory semconv:HTTP labels Feb 15, 2023
@pyohannes
Copy link
Contributor

There has been some related discussion in this issue: #2548, especially this comment.

Compatibility with ECS was not considered back then. I think the important part is to be able to distinguish between application and transport layer protocols. It seems ECS does this via network.protocol and network.transport.

@trask
Copy link
Member Author

trask commented Feb 16, 2023

we have net.transport already, do u recall if the idea was to change that into

  • net.transport.protocol.(name|version)?

In that case, I'm curious what others think of:

  • net.protocol.(name|version)
  • net.transport.(name|version)

@pyohannes
Copy link
Contributor

we have net.transport already, do u recall if the idea was to change that into

  • net.transport.protocol.(name|version)?

The intent was not to break existing conventions, so this wasn't considered.

In that case, I'm curious what others think of:

  • net.protocol.(name|version)
  • net.transport.(name|version)

If it is decided to align with ECS and many other net.* attributes are changed anyway, I think it makes sense to change this too.

@Oberon00
Copy link
Member

Oberon00 commented Feb 16, 2023

ECS has e.g. http.version, which this net.protocol.* was meant to replace I think. https://www.elastic.co/guide/en/ecs/current/ecs-http.html#field-http-version

Currently proposed #3178 would move us a bit further away from ECS

@trask trask assigned trask and unassigned carlosalberto Feb 17, 2023
@trask
Copy link
Member Author

trask commented Feb 21, 2023

@Oberon00 do you have any objection to renaming net.app.protocol.(name|version) to the simpler net.protocol.(name|version) (removing the app component)?

net.transport.(name|version) could be transport-level protocol name and version

@Oberon00
Copy link
Member

I don't remember why app was added, probably to emphasize it's not the lower-level protocol? Maybe you have a better qualified opinion on this proposal @lmolkova?

@lmolkova
Copy link
Contributor

lmolkova commented Feb 21, 2023

net.transport:

TL;DR: let's not use it for HTTP semconv v1.

I could not find any place in java or python instrumentation where it's set to anything but TCP_IP except a few places in python where it's incorrectly(?) set to UNIX domain socket.

even with current net.transport, we're not strictly following protocol layers as we combine transport and internet layers:
image

At the same time, we do capture internet-level information in net.sock.family attribute (inet6).

I assume HTTP/1.1 and /2 are good enough heuristics to say the transport protocol is TCP and HTTP/3 is good enough to assume it's UDP. I believe it's a common case that app-level protocol implies a certain transport protocol (e.g. AMQP and MQTT work on top of TCP/IP) and populating both makes sense in rare cases.

So in scope of HTTP spec stabilization, I suggest removing a single mention of net.transport from HTTP traces spec (and two mentions from metrics). It can stay experimental for the time being and be added later on.

net.transport.version also seems even more difficult since none of the common transport protocols have versions.

net.app.protocol
I'm in favor of net.protocol.name|version if we don't align with ECS and network.protocol.name|version if we align with it, hoping ECS can adopt such changes too.

Application protocol seems to be the most interesting one for any application-level instrumentation and having a convenient, easy-to-understand and a shorter name would be great.

jsuereth pushed a commit that referenced this issue Feb 28, 2023
Based on @lmolkova's
#3215 (comment)

## Changes

Removes mention of `net.transport` from http semantic conventions.

---------

Co-authored-by: Reiley Yang <reyang@microsoft.com>
jsuereth pushed a commit to jsuereth/otel-semconv-test that referenced this issue Apr 19, 2023
Based on @lmolkova's
open-telemetry/opentelemetry-specification#3215 (comment)

## Changes

Removes mention of `net.transport` from http semantic conventions.

---------

Co-authored-by: Reiley Yang <reyang@microsoft.com>
jsuereth pushed a commit to open-telemetry/semantic-conventions that referenced this issue May 11, 2023
Based on @lmolkova's
open-telemetry/opentelemetry-specification#3215 (comment)

## Changes

Removes mention of `net.transport` from http semantic conventions.

---------

Co-authored-by: Reiley Yang <reyang@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions semconv:HTTP spec:trace Related to the specification/trace directory
5 participants