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

Remove alternative attribute sets from HTTP semantic conventions #2469

Merged
merged 10 commits into from
Jul 27, 2022

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Apr 5, 2022

Fixes #2114

Depends on #2522

Changes

Here's a revision of what current instrumentations do (#2114 (comment)) and discussions on required/optional.

The TLDR:
Require HTTP instrumentation to be consistent:

  • define one set of required attributes for server and client, clarify how hostname should be populated
  • clarify what required and optional attributes mean
    • required: MUST provide if available
    • optional opt-in: MUST populate by default (if available), MAY allow disabling
    • optional opt-out: MUST NOT populate by default, MAY allow enabling
  • categorize all attributes based on it

Proposal:

Client: the only set of attributes clients should support is: http.url, net.peer.name, net.peer.port

  1. require http.url, net.peer.name (use IP if only IP is avaialble), net.peer.port (when not default)
  2. Make net.peer.ip attribute optional (opt-in)

Server: the only set of attributes servers should support is: http.scheme, net.host.name, net.host.port, http.target, http.route

  1. Require net.host.name, ask for it's value to be the best available server name with specific priority (server-name, host header, ip address)
  2. Require (when available) http.route - it's an essential attribute for both server traces and metrics
  3. Remove http.host - host header should be populated (opt-in) using http.request.header.host
  4. Remove http.server_name - if it's available it's populated in net.host.name
  5. Make http.url optional (opt-in)
  6. Make net.host.ip attribute optional (opt-in)

Related issues #2028

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory labels Apr 5, 2022
@tigrannajaryan
Copy link
Member

Discussed in spec SIG meeting. Decided that @lmolkova will make it clearer which of these changes are breaking (if any) so that we can understand the impact on existing recipients.

specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

It would be also great to categorize the changes into 2 buckets:

  • Renaming of an attribute without semantic changes. This is supported by Schema files.
  • Any other changes. This is unsupported by Schema files and I would like to understand if changes like this may be common in the future and needs to be supported.

@lmolkova
Copy link
Contributor Author

lmolkova commented Apr 6, 2022

Here's the delta of changes: none of these changes are breaking from a consumer standpoint, schema files - ?:
Most changes only switch optional attributes to required or remove optional attributes (which might be breaking from a schema file standpoint).

It requires almost all instrumentations to change, but all consumers, who comply with the current spec fully, should not need to change anything.

I've done the analysis of existing instrumentations in Java, .NET, Python, Go and JS (otel or contrib repos) on how they populate attributes to demonstrate inconsistencies and show common patterns #2114 (comment).

Common attributes

  • http.url:
    • currently common and optional
    • in the PR:
      • clients: required
      • servers: removed
    • existing instrumentations:
      • clients: all instrumetnations
      • servers: most, but usually constructed from components
  • http.target:
    • currently common and optional
    • in the PR:
      • clients: remove
      • servers: required
    • existing instrumentations:
      • clients: few
      • servers: all
  • http.host: renamed
    • currently common and optional
    • in the PR: data goes to to net.peer.name, net.host.name or opt-in header http.request.header.host
    • existing instrumentations:
      • clients: most
      • server: most, but inconsistently - as URL components (host:port) or host header
  • http.scheme
    • currently common and optional
    • in the PR:
      • clients: removed
      • servers: required
    • existing instrumentations:
      • clients: few
      • servers: most
  • http.flavor
    • common and optional -> common, optional, opt-out
    • existing instrumentations: most populate it.
  • http.request_content_length, http.request_content_length_uncompressed, http.response_content_length
    • common and optional -> common, optional, opt-in
    • existing instrumentations:
      • very few populate them. Uncompressed attributes are only populated by JS, check out all insights here here
  • http.retry_count moved to client-only. I believe it was added to common attributes by mistake ~ a month ago here Define span structure for HTTP retries and redirects #2078, @denisivan0v, can you please confirm?)).
  • net.peer.ip
  • currently common and optional
    • in the PR:
      • clients: optional (opt-in) - IP is rarely known by HTTP clients
      • servers: optional (opt-out)
    • existing instrumentations:
      • clients: few, among them Go parses it out of host component (if it happens to be IP)
      • servers: most
  • net.peer.port:
    • currently common and optional
    • in the PR:
      • clients: required (if not default)
      • servers: optional (opt-in)
  • existing instrumentations:
    • clients: most
    • servers: some
  • http.request.header.*, http.response.header.*: nothing really changed
    • explicitly categorized as opt-in, current spec already requires explicit configuration to enable

Client

Sets of attributes (one of them is required):

  • currently: 4 sets (url or components)
  • in the PR: there is just one - http.url, net.peer.name, net.peer.port, which complies with the previous spec, since it includes url

Attributes

  • net.peer.name: optional -> required
    • existing instrumentations: most
  • net.peer.port: optional -> required (if not default)
    • existing instrumentations: most

Server

Sets of attributes (one of them is required):

  • currently: 4 sets (url or components)
  • in the PR there is one: http.scheme, net.host.name, net.host.port, http.target, which is one of required sets per current spec

Attributes

  • http.server_name: renamed
    • when value is available it goes to net.host.name
    • existing instrumentations: few
  • http.route: optional -> required
    • existing instrumentations: most who have it, populate it, but sone fallback to path (which is not right)
  • http.client_ip: optional -> optional, opt-out
  • net.host.name: optional -> required
    • it was not clearly specified what goes there, specified it (as best known server name)
    • existing instrumentations: few, it's chosen as the most generic one (among http.host and http.server_name)
  • net.host.port: optional -> required (if not default)
    • existing instrumentations: ~half

@lmolkova
Copy link
Contributor Author

lmolkova commented Apr 6, 2022

Any other changes. This is unsupported by Schema files and I would like to understand if changes like this may be common in the future and needs to be supported.

@tigrannajaryan Can you please share what you think about removals of optional attributes? Backends/exporters should not require those and any instrumentation is free to drop any optional attribute at any moment.

@lmolkova
Copy link
Contributor Author

lmolkova commented Apr 6, 2022

@carlosalberto @arminru I have added the delta for each attribute changed in the #2469 (comment). Please take a look.

The only change that might be breaking is the removal of optional attributes. I'd really like to explore this option even if we consider it breaking.
As a last resort, we can keep such attributes as opt-in, but it'd be not ideal to keep redundant information.

@tigrannajaryan
Copy link
Member

Can you please share what you think about removals of optional attributes? Backends/exporters should not require those and any instrumentation is free to drop any optional attribute at any moment.

I think we need to discuss this. There is currently no precise definition of what "Optional" means. You are suggesting that the backends should be ready that the attribute may or may not be present and that the telemetry may include or exclude optional attributes at any time. Alternatively, we could have a stricter definition of what "optional" means, in particular we could say that the developers are free to decide whether to include an optional attribute or no, but once the decision is made they should stick with it and should not change the particular instrumentation (if the instrumentation is considered "Stable").

http.server_name: renamed
when value is available it goes to net.host.name

I don't think this is a renaming. Renaming is when the attribute name is changed while preserving its semantics exactly. This is not the case here. net.host.name has a different semantic. It can mean the http server name, but it can also be used for other scenarios, where there is no http server at all. http.server_name had narrower application than net.host.name. We cannot express such changes in schema files. If we mark this as a renaming in schema file then any conversion from the new version to old version will unconditionally rename every net.host.name attribute into http.server_name, even if the span has nothing to do with http operation, thus breaking the telemetry.

Semantically this change is 2 operations:

  • deleting of http.server_name attribute.
  • adding a new requirement that the server name must be recorded in net.host.name if the span represents an http operation.

This is a breaking change. How do we handle it? Technically, this should be allowed, since it is a change in an "Experimental" document. How much pain will this cause in practice? I don't know. I would like to hear opinions.

Informally, in the past we said that we are putting a freeze to the semantic conventions and will not be making breaking changes until we have a good backward compatibility story and the proposal #2180 that addresses it is hopefully very close to being accepted. If #2180 gets accepted we will have a formal stance on this and can say that all instrumentations are "Unstable" and thus we are explicitly allowed to make changes introduced by #2469 (this PR).

@lmolkova
Copy link
Contributor Author

@arminru thanks!

I think we need to discuss this. There is currently no precise definition of what "Optional" means. You are suggesting that the backends should be ready that the attribute may or may not be present and that the telemetry may include or exclude optional attributes at any time. Alternatively, we could have a stricter definition of what "optional" means, in particular we could say that the developers are free to decide whether to include an optional attribute or no, but once the decision is made they should stick with it and should not change the particular instrumentation (if the instrumentation is considered "Stable").

Let's discuss - I'd like to bring it up at the spec meeting tomorrow.

don't think this is a renaming. Renaming is when the attribute name is changed while preserving its semantics exactly. This is not the case here. net.host.name has a different semantic

net.host.name has a very vague description and I don't think its semantics was really changed in this PR: "net.host.name should be the host name of the local host, preferably the one that the peer used to connect for the current operation. If that is not known, a public hostname should be preferred over a private one.". If I can generalize, vaguely defined attribute semantics is a bug, clarifying the meaning is a bugfix.

This is a breaking change. How do we handle it? Technically, this should be allowed, since it is a change in an "Experimental" document. How much pain will this cause in practice? I don't know. I would like to hear opinions.

Sure, let's try to collect opinions. My point here is that this proposal is still a valid HTTP instrumentation according to the current specification - all backends should expect this set of attributes already. I've made it based on what current instrumentations populate - it gives some confidence that backends already support it.
Current instrumentations are very inconsistent and backends/exporters have to normalize the data and support all possible variations of the current spec anyway. For future stable specs, I agree, that it's essential to define what optional means.

Regarding #2180 - it talks about instrumentations, making the status of conventions irrelevant, what does it mean for semantic conventions?

@tigrannajaryan
Copy link
Member

Sure, let's try to collect opinions. My point here is that this proposal is still a valid HTTP instrumentation according to the current specification - all backends should expect this set of attributes already.

@lmolkova Just to clarify: in your new proposal you made http.scheme, net.host.name, net.host.port, http.target always required, right? So, the telemetry emitted according to the new proposal is completely valid according to the current (old) specification which says that if these 4 attributes are present then it is a valid span and http.server_name is no longer required. If that is the case then I agree with you, this is not a breaking change.

@lmolkova
Copy link
Contributor Author

@tigrannajaryan, correct.

@tigrannajaryan
Copy link
Member

@tigrannajaryan, correct.

@lmolkova OK, this is good. I no longer have any objection to this change from the perspective of removal of http.server_name.

The only question that remains for me is whether we consider removal of optional attributes a breaking change or no. I can't make the spec SIG meeting today. If you discuss this in the meeting please post a summary here.

@lmolkova
Copy link
Contributor Author

lmolkova commented Apr 13, 2022

@tigrannajaryan thank you! we didn't have a proper discussion on optional attribute removal, but I beleive @carlosalberto wanted to chat with a few folks and share his opinion on it (and this change).

@carlosalberto
Copy link
Contributor

Reviewed this document and it all makes sense to me, and I think it's ok to conver it to a non-draft PR.

The only question that remains for me is whether we consider removal of optional attributes a breaking change or no

This point brought by @tigrannajaryan is the only important thing I see now, and we should gather more information. Let's mention that in the next maintainers/Spec meeting again, to double verify this is fine (I'm trying to gather information on that myself, etc).

specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

Reviewed this document and it all makes sense to me, and I think it's ok to conver it to a non-draft PR.

The only question that remains for me is whether we consider removal of optional attributes a breaking change or no

This point brought by @tigrannajaryan is the only important thing I see now, and we should gather more information. Let's mention that in the next maintainers/Spec meeting again, to double verify this is fine (I'm trying to gather information on that myself, etc).

I posted some comments above, which upon re-reading don't seem to be very clear. Let me try to clarify my position a bit better.

First, let me start by stating that making a change to a producer of telemetry should be considered separately from making a change to semantic conventions. The spec distinguishes these two and discusses these topics separately (see semantic convention stability vs stability of telemetry producers). So, with that in mind, here is what I think:

  1. Removal of attributes from semantic conventions in the specification is a breaking change, even if the attribute is optional. The existing wording here is unambiguous about this. Removals of attributes are a breaking changes from semantic convention perspective.
  2. This particular document (Semantic conventions for HTTP spans) is marked as "Experimental", so I do not see any formal reasons to prohibit this change. Even though it is a breaking change of semantic conventions, we are allowed to make it.
  3. Changing existing telemetry producers to produce http.scheme, net.host.name, net.host.port, http.target and stop producing http.server_name is not a breaking change because it was already allowed in the past. Recipients of telemetry should be ready for this.

Given the above, I think the decision we need to make is to decide on how big we think the pain caused by this change to existing recipients can be and whether we are OK with causing it. One argument in favour of allowing it is that after the change the telemetry recipients will receive telemetry that is still valid even according to old semantic conventions, so perhaps the pains are not very big.

@lmolkova
Copy link
Contributor Author

lmolkova commented Apr 20, 2022

Discussed open questions during today's Spec meeting:

  • performance of required attributes: need clarification in the general spec, agreed that expensive calculations are not fine, string formatting is reasonable
  • need to dig into the history of attributes being removed or turned into opt-in. The investigation is below:

Most of the attributes in question were added in #263 by @Oberon00 (thanks @arminru for the pointer!). These attributes haven't changed much since #263. There was no discussion on multiple attribute sets at that time. My understanding based on reading through the PR is that they were added as an alternative to http.url when it's not readily available (on servers).

@Oberon00 I would love to hear your opinion or any additional context on what can break if we follow the current proposal (url for clients, one of the set with fragments for servers) and remove extra sets of attributes.

More details:

@lmolkova lmolkova force-pushed the http-required-vs-optional branch 3 times, most recently from f836823 to c52afc3 Compare April 29, 2022 00:11
@lmolkova
Copy link
Contributor Author

@Oberon00 thank you for the feedback! Can you please share your opinion on limiting supported attribute sets? Do you remember if there are any requirements that made us introduce multiple sets in #263 and if anything (you're aware of) depend on it?

@Oberon00
Copy link
Member

For consumers of telemetry, I think this is not breaking, but for producers (instrumentations), it may require adaption.

@lmolkova
Copy link
Contributor Author

For consumers of telemetry, I think this is not breaking, but for producers (instrumentations), it may require adaption.

producers are not forced to update and if they populate schema url with version, consumers that support old versions (all that exist today), would still work.
So what'll break are instrumentations that won't update and when used with a consumer that does not support old spec version. It's breaking, but should be expected from an experimental spec.

@arminru arminru requested a review from a team July 26, 2022 15:08
Copy link
Contributor

@denisivan0v denisivan0v left a comment

Choose a reason for hiding this comment

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

@lmolkova this is a huge step forward and it helps a lot to clarify and simplify HTTP spec (and make it stable eventually). Many thanks for your hard work!

@jmacd
Copy link
Contributor

jmacd commented Jul 27, 2022

Thank you @lmolkova!

@jmacd jmacd merged commit d1c2493 into open-telemetry:main Jul 27, 2022
@lmolkova
Copy link
Contributor Author

Thank you all!! 🥳

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
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Optional vs required attributes in HTTP conventions and beyond