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

Define net.sock attributes and clarify logical net.peer|host.name attributes #2614

Merged
merged 15 commits into from
Jul 13, 2022

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Jun 10, 2022

Fixes #2607

Changes

  1. Introduces socket-level attributes:
    • BREAKING: net.peer.ip -> net.sock.peer.addr, net.host.ip -> net.sock.host.addr
    • BREAKING: for pure socket-level instrumentation, net.peer.name is renamed to net.sock.peer.addr. For other instrumentations, net.peer.name semantics didn't change.
  2. Clarifies net.peer|host.name and port attributes as logical ones representing final destination.
  3. Clarifies socket-level proxy scenario

Related issues #2469

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 10, 2022

/cc @Oberon00 I made the first stab at net.sock attributes.

One thing I didn't figure out yet is the relationship between net.transport and net.sock.family.

Transport currently is a closed set, address families are not (and different sets on win and linux).
Transport defines TCP and UDP, address family differentiates IPv4 and IPv6.
Transport has pipe and it's not a socket.

I'd propose the following on 'net.transport:

  • remove or deprecate net.transport ip and unix values
  • add socket value

Then net.sock.family can be used to understand if it's an IP-based protocol, Unix, or perhaps Bluetooth when net.transport = socket. Or if addr family is AF_INET6, net.transport explains if it tcp or udp.

LMK what you think.

@Oberon00
Copy link
Member

Oberon00 commented Jun 10, 2022

I didn't have time to think about the net.transport question yet, but I noticed one thing:

There is no replacement for net.peer.name yet, and also the definition in the "net.*.name" section has not changed yet to address the logical peer change:

For net.peer.name, this should be the name of logical remote destination that was used to look up the IP address that was connected to (i.e., matching net.peer.ip if that one is set)

With the new definition, we don't know if we actually connect to the logical peer, I don't think a more precise definition than "logical peer" can be provided on the generic level, maybe just a note that semantic conventions that want this to be set SHOULD define more precisely what it means for them.

I also think that net.peer.ip does not make sense anymore, there should probably only be net.sock.peer.addr.

@lmolkova
Copy link
Contributor Author

I also think that net.peer.ip does not make sense anymore, there should probably only be net.sock.peer.addr.

I think we wanted to use it instead of http.client_ip, but thinking more about it, perhaps reverse proxy and x-forwarded-for and friends are so HTTP-specific that http.client_ip would make more sense than generic net.peer.ip.

@lmolkova lmolkova force-pushed the net-sock branch 2 times, most recently from 6a20e52 to a5ddee9 Compare June 10, 2022 21:54
@lmolkova
Copy link
Contributor Author

@Oberon00 I think I addressed your feedback. I'm not sure if renaming net.peer.name is necessary - even though we work on experimental spec, we should minimize breaking changes. Currently, the breaking change is minimal (net.peer.ip -> net.sock.peer.addr and net.host.ip -> net.sock.host.addr) and spec is clear on socket vs logical attributes plus proxy scenarios,

@lmolkova lmolkova force-pushed the net-sock branch 5 times, most recently from b2f2024 to 40c9728 Compare June 10, 2022 22:10
@Oberon00
Copy link
Member

Oberon00 commented Jun 12, 2022

I'm not talking about renaming the existing net.peer.name, but about adding an additional attribute, e.g. net.sock.peer.name that would then correspond to net.sock.peer.addr (net.peer.name does not necessarily do that anymore).

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 13, 2022

@Oberon00 what would net.sock.peer.name mean - DNS name of net.sock.peer.addr ? Like proxy host name if one's available?

@Oberon00
Copy link
Member

@Oberon00 what would net.sock.peer.name mean - DNS name of net.sock.peer.addr ? Like proxy host name if one's available?

Exactly. This is how I understood the net.peer.ip/name attributes originally. net.sock.peer.name would then be mostly IP-specific, just like net.peer.ip currently is.

As I understand it, this PR will cause the following "reshuffling":

  • net.peer.name: Logical peer name, but not of the peer service (which is peer.service) but of something that could theoretically be "connected" to, typically a DNS-resolvable hostname (even if just from some private network), but might be something else. Depending on the address family used, a low-level 2non-logical" net.peer.name becomes net.peer.sock.addr or net.peer.sock.name.
  • net.peer.ip -> net.peer.sock.addr, always, and only available for low-level peer, not for logical one if different.

@lmolkova lmolkova force-pushed the net-sock branch 4 times, most recently from aa5f310 to de2643d Compare June 14, 2022 23:56
@lmolkova lmolkova changed the title Define net.sock attributes Define net.sock attributes and clarify logical net.peer|host.name attributes Jun 14, 2022
@lmolkova lmolkova marked this pull request as ready for review June 15, 2022 00:00
@lmolkova lmolkova requested review from a team June 15, 2022 00:00
@lmolkova
Copy link
Contributor Author

@Oberon00 thank you for the feedback! I believe I addressed it and this PR now is ready for review.

CHANGELOG.md Outdated Show resolved Hide resolved
semantic_conventions/trace/general.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/general.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/general.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/general.yaml Outdated Show resolved Hide resolved
@lmolkova
Copy link
Contributor Author

@Oberon00 I believe I resolved your feedback, can you please take another look?

@lmolkova
Copy link
Contributor Author

lmolkova commented Jul 8, 2022

@trask thanks a lot for the feedback and the prototype!

@carlosalberto, can you please take another look? Thanks!

@jmacd jmacd self-assigned this Jul 12, 2022
@jmacd
Copy link
Contributor

jmacd commented Jul 12, 2022

@open-telemetry/specs-approvers Last call for feedback.

@reyang reyang merged commit 0b45213 into open-telemetry:main Jul 13, 2022
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
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:metrics Related to the specification/metrics directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should net.peer.* describe socket-level peer or "logical" peer (for HTTP and in general)?
9 participants