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.

Sign up for GitHub

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

Jump to bottom

Add Requirement Level backward compatibility clarifications #2601

Closed
wants to merge 4 commits into from
Closed

Add Requirement Level backward compatibility clarifications #2601

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
162a947
Add Requirement Level backward compatibility suggestions
lmolkova Jun 2, 2022
819ca1c
note that stability applies to consumers, not producers
lmolkova Jun 3, 2022
f72eccc
Apply suggestions from code review
lmolkova Jun 9, 2022
9f14113
clarify backward compat and nits
lmolkova Jun 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions specification/common/attribute-requirement-level.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
- [Recommended](#recommended)
- [Optional](#optional)
- [Performance suggestions](#performance-suggestions)
- [Changing requirement level](#changing-requirement-level)

<!-- tocstop -->

Expand All @@ -34,6 +35,8 @@ Semantic convention that refers to an attribute from another semantic convention

For example, [Database semantic convention](../trace/semantic_conventions/database.md) references `net.transport` attribute defined in [General attributes](../trace/semantic_conventions/span-general.md) with `Conditionally Required` level on it.

Semantic conventions MAY changed requirement level on attribute in a backward-compatible manner, see [Changing requirement level](#changing-requirement-level) for details.

## Required

All instrumentations MUST populate the attribute. Semantic convention defining a Required attribute expects that an absolute majority of instrumentation libraries and applications are able to efficiently retrieve and populate it, can ensure cardinality, security, and other requirements specific to signal defined by the convention. `http.method` is an example of a Required attribute.
Expand Down Expand Up @@ -67,3 +70,33 @@ Here are several examples of expensive operations to be avoided by default:
- DNS lookup to populate `net.peer.name` if only IP address is available to the instrumentation. Caching lookup results does not solve the issue for all possible cases and should be avoided by default too.
- forcing `http.route` calculation before HTTP framework calculates it
- reading response stream to find `http.response_content_length` when `Content-Length` header is not available

## Changing requirement level

_Note: Requirement level compatibility applies to telemetry consumers only. When semantic convention is updated from version `X` to version `Y` in a backward-compatible manner, consumers that only support version `X` would be fully compatible with telemetry version `Y`._
Copy link
Member

Choose a reason for hiding this comment

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

These rules only apply to stable semantic conventions, correct? Might be worth noting that.


Requirement level change is backward compatible if and only if the set of cases when previous version populates it, is a subset of cases when new version would populate it.
Copy link
Member

Choose a reason for hiding this comment

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

This definitions is somewhat difficult to unpack. Traditional backwards compatibility is: if my old code is valid in old requirements and still valid in new requirements, then the requirements are backwards compatible (nothing about whether the code populates X or Y). By this traditional definition some changes proposed below are not backwards compatible. For example: Recommended to Required - if my old code did not populate X when X was recommended, the code was still valid since X was still allowed to be skipped, but once X becomes required the code becomes invalid.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it needs to be made clear whether backwards compatibility refers to the telemetry producer (instrumentation) or to the telemetry consumer (backend). It seems to me the changes in this PR refer to consumers.

Changing from Recommended to Required is a breaking change for producers, but not for consumers. Changing from Required to Recommended is a breaking change for consumers, but not for producers.

Copy link
Contributor Author

@lmolkova lmolkova Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think telemetry stability applies to telemetry producers at all. When a new version of semantic conventions is released, producers don't have to update, at least for a long time. They don't need to support the previous version, they don't depend on the telemetry they produce.
If they decide to update, they do it along with the schema version update. If they don't support the schema version, they can't update at all

Almost any semantic convention change would require some action from instrumentations, but none of it seems breaking as long as it's versioned.

Copy link
Contributor Author

@lmolkova lmolkova Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When producer don't populate schema version, almost any change would break it:

  • adding new required attribute (which is considered non-breaking, maybe accidentally)
  • attribute rename, which is not really classified, but allowed

I understand the concern though, but there is nothing we can do for producers that don't version telemetry if we want to evolve conventions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #2606 to discuss further.

Copy link
Member

Choose a reason for hiding this comment

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

I'd consider removing this sentence. It seems to restate the sentence on line 76 with less clarity.


- Requirement level MAY be increased from relaxed to more strict level if it does not add new conditions that limit when attribute is populated. Examples of such changes are:
- `Optional` to `Recommended`
- `Recommended` to `Required`
- `Conditionally Required` to `Required`
- `Optional` to `Conditionally Required` is allowed if the condition does not restrict attribute from being populated if explicitly enabled.
lmolkova marked this conversation as resolved.
Show resolved Hide resolved
- `Recommended` to `Conditionally Required` is allowed if the condition allows to populate the attribute when it's not satisfied
lmolkova marked this conversation as resolved.
Show resolved Hide resolved

lmolkova marked this conversation as resolved.
Show resolved Hide resolved
- Condition for `Conditionally Required` attribute can be changed as long as previous condition still satisfies the new one.

Semantic conventions SHOULD start with relaxed requirement levels and non-restrictive conditions whenever possible and SHOULD increase levels only when necessary based on telemetry consumers needs.
lmolkova marked this conversation as resolved.
Show resolved Hide resolved

_Note: There could be other requirements on [Telemetry Stability](../telemetry-stability.md) that can limit semantic convention evolution or prevent instrumentations from adopting new versions._

Examples of backward-compatible changes:

- Changing `http.url` requirement level from `Recommended` to `Required`. `http.url` would be populated in all cases it was populated before; Consumers of telemetry don't have to change their behavior.
- Changing condition on `db.operation` from "If `db.statement` is not applicable" to "If `db.statement` is not applicable or not provided".
- Changing `http.request.header.host` level from `Optional` to `Conditionally Required` with following condition: 'if request-target is in absolute-form and authority component is not the same as `Host` header'. There is no need to mention that attribute can be explicitly enabled - when condition is not satisfied, `Conditionally Required` attributes are considered `Optional` by default.

Examples of backward-incompatible changes:

- Changing [`faas.invoked_name`](../trace/semantic_conventions/faas.md) level from `Required` to `Recommended`. Telemetry consumers which used to rely on the presence of this attribute have to change their behavior.
- Changing condition on [`rpc.jsonrpc.version`](../trace/semantic_conventions/rpc.md) from "If different than the default version `1.0`" to "If different than the default version `2.0`"