-
Notifications
You must be signed in to change notification settings - Fork 896
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
Remove alternative attribute sets from HTTP semantic conventions #2469
Conversation
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. |
It would be also great to categorize the changes into 2 buckets:
|
Here's the delta of changes: none of these changes are breaking from a consumer standpoint, schema files - ?: 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
ClientSets of attributes (one of them is required):
Attributes
ServerSets of attributes (one of them is required):
Attributes
|
9b68495
to
7079d1a
Compare
@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. |
@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. |
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").
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. Semantically this change is 2 operations:
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). |
@arminru thanks!
Let's discuss - I'd like to bring it up at the spec meeting tomorrow.
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. Regarding #2180 - it talks about instrumentations, making the status of conventions irrelevant, what does it mean for semantic conventions? |
@lmolkova Just to clarify: in your new proposal you made |
@tigrannajaryan, correct. |
@lmolkova OK, this is good. I no longer have any objection to this change from the perspective of removal of 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. |
@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). |
Reviewed this document and it all makes sense to me, and I think it's ok to conver it to a non-draft PR.
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). |
05abaeb
to
8607598
Compare
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:
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. |
Discussed open questions during today's Spec meeting:
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 @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:
|
8607598
to
36ba540
Compare
f836823
to
c52afc3
Compare
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. |
There was a problem hiding this 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!
Thank you @lmolkova! |
Thank you all!! 🥳 |
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:
Proposal:
Client: the only set of attributes clients should support is:
http.url
,net.peer.name
,net.peer.port
http.url
,net.peer.name
(use IP if only IP is avaialble),net.peer.port
(when not default)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
net.host.name
, ask for it's value to be the best available server name with specific priority (server-name, host header, ip address)http.route
- it's an essential attribute for both server traces and metricshttp.host
- host header should be populated (opt-in) usinghttp.request.header.host
http.server_name
- if it's available it's populated innet.host.name
http.url
optional (opt-in)net.host.ip
attribute optional (opt-in)Related issues #2028