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

Add a variety of key definitions surrounding stability #142

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add a variety of key definitions surrounding stability #142

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ad096e
Add stability definition
jack-berg Nov 26, 2024
851a389
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
jack-berg Dec 3, 2024
1aac97d
Fix typo, make removal of required property allowed
jack-berg Dec 4, 2024
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
82 changes: 76 additions & 6 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,85 @@ go-jsonschema \

## Stability definition

Before reaching 1.0, each minor version change is equivalent to major version change. That is, there are no guarantees about compatibility and all changes are permitted. As of 1.0, we provide the following stability guarantees:
**NOTICE**: The stability definitions are applicable after this repository publishes a stable release (1.0.0). Currently, this repository is experimental and makes no stability guarantees.

- For major version: No guarantees.
- For minor versions: TBD
Stability definition consists of the following sections:

Allowable changes:
* [Objectives](#objectives): Overview of the motivation behind stability.
* [Guarantees and allowed changes](#guarantees-and-allowed-changes): Specific details on allowed and disallowed changed.
* [Applicability](#applicability): Limits of stability definitions, including experimental features and extension points.
* [File format](#file-format): The `file_format` property and implementation behavior when schema versions are not aligned.

- For major versions: All changes are permitted.
- For minor versions: TBD
### Objectives

The primary objective of stability is to protect users from breaking changes. That is, users providing configuration conforming to a particular stable version of the schema should be able to reliably upgrade minor and patch versions without risk that their configuration becomes invalid, or that the interpretation changes.

A secondary objective is to allow for stable code generation from the JSON schema for language implementations of the [in-memory configuration model](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk.md#in-memory-configuration-model). With stability guarantees in mind, maintainers should be able to write code generation logic (or handcraft an in-memory configuration model) without risk that allowed changes to the schema will break stable implementations. **NOTE**: There is _no_ guarantee that the output of off-the-shelf [code generation](#code-generation) tools will be stable when allowed changes are made. Maintainers are responsible for understanding how code generation tools work (and evolve) and intersect with the guarantees made here.

All schema changes are considered through the lens of maintaining these objectives.

### Guarantees and allowed changes
Copy link
Member

Choose a reason for hiding this comment

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

This whole section is written as "absolute" terms.

In reality, every statement here can be invalid, when interpreted in a different, "relative" context.

By absolute/relative, I mean this:

The format is not just a contract between two (2) parties.

The format is a contract between three (3) parties, which are:

  • [SCHEMA] maintainers of the opentelemetry-configuration repository, when defining a yaml schema
  • [SIG] maintainers of a SIG, when parsing configuration from a yaml schema
  • [USER] maintainers of a given user configuration files, when writing a yaml schema configuration

I think it will be beneficial to details separately:

  • the contract and expectations between [SCHEMA] and [SIG]
  • the contract and expectations between [SCHEMA] and [USER]
  • the contract and expectations between [USER] and [SIG]

A claim as "Type property names will not change" is not absolute, some type names will change.

For a given schema version number, the type property name will not change even on minor file_format changes, sure, so this is true in the context of [USER] - [SIG], within some constraints on the file_format value.

This does not forbid name changes when evolving the schema definitions:

  • new schema can rename properties, affecting the [SCHEMA] - [SIG] part when parsing, for example on a major version change in file_format
  • new schema can rename properties, affecting the [SCHEMA] - [USER] part when writing a yaml config, also on major version change in file_format: users changing the file_format tag in their file are expected to adjust to the new format spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

The format is a contract between three (3) parties, which are:

I'm not sure I completely understand what you mean by this. I think part of what you're saying is reflected in the objectives section, which explains that stability guarantees impact both a user writing YAML config (i.e. [SCHEMA] and [USER] in your terms), and implementations interpretting this config and publishing in-memory representations of the data model (i.e. [SCHEMA] and [SIG] in your terms).

A claim as "Type property names will not change" is not absolute, some type names will change.

What do you mean by this? What I mean to claim here is that a given property name (let's say the .tracer_provider property for example) which is part of a stable type (i.e. no */experimental suffix) will not change in minor or path versions.

new schema can rename properties, affecting the [SCHEMA] - [SIG] part when parsing, for example on a major version change in file_format

Yes, all changes are allowed in major version updates. Do we need to explicitly state this? I'd like to follow the lead of opentelemetry-proto's stability definitions and try to keep language as terse as is required by the language.

Do you think that it would help to explain that these stability requirements apply to minor version updates, and explain that major version updates can include carte blanche breaking changes.

Copy link
Member

@marcalff marcalff Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The problem I see is the following:

A major update in the yaml format will have a massive impact:

  • all SDK implementations will have to be updated with the new schema
  • all the SIG will have to make a new release, including the SIG that are not concerned with the change in the first place (for example, because they do not implement all features, and the change that caused the major update is located in a part of the yaml schema which is not supported)
  • all the user applications will need to deploy the new SDK, for all languages
  • all user configurations will need to be updated with the new yaml version

When the root cause of the change is to say that:

exporter:
  jaeger:

is no longer valid, this has huge consequences for everybody, including SDK that did not supported jaeger in the first place, just because MAJOR changed from 1 to 2.

The text as written does a very good job of describing what a stable type is, it really does.

The problem is that when something affects the stability of a type, there should be a way to mitigate changes and provide a transition path to change the schema in a MINOR version instead of forcing a major version change.

Another example:

something:
  # constraints in the schema enforce a valid range of [0, 100]
  foo_value: 50

The day the value ranges needs to change to [10, 90] instead, this makes it a breaking change, forcing a major version update to the schema.

Instead, there should be a way to provide a reasonable migration path, like:

  • (a) schema 1.0.0 allows range [0, 100]
  • (b) schema 1.1.0 allows range [0, 100] in yaml, and document that the SDK implementation must:
    • increase values below 10 to 10,
    • decrease values above 90 to 90,
    • raise a warning when this happens
  • (c) schema 1.2.0 allows range [10, 90]

Item (b) is about stability garantees between [SCHEMA] and [SIG] as I called it, where the SIG implementation is to expect extra processing beyond yaml parsing, mandated by the schema.

Copy link
Member Author

Choose a reason for hiding this comment

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

A major update in the yaml format will have a massive impact:

I agree the impact is huge.

The impact is so huge that it creates a strong incentive to avoid a major version update. And if the rules are fairly (and necessarily) restrictive in terms of what is allowed in a minor version update, then it creates a strong incentive to model types correctly the first time.

Consider other areas where similar strict restrictions exist like opentelemetry-proto, or the specification, or language implementations like opentelemetry-java.

We have definitions of what is allowable in minor versions to protect users, and to promote careful / thoughtful design. In all these cases, a major version bump is something to strongly avoid. We write down when a major version is required with the hope that we don't have to do it.

For your examples:

  1. The first example is the removal of a known property / type, the jaeger exporter. This is considered a breaking change under the rules laid out in this PR. Luckily for us, jaeger is already gone, but the same could happen to other exporters, processors, samplers, etc. The fact that removing a type is a breaking change is an incentive to keep it some form (potentially deprecated) until the next major version, which could be indefinitely if there is never a strong enough reason. Its not an incentive to make invasive changes and bump the major version.
  2. The second example is the adjustment of constraints from allowing [0, 100] to [10, 90]. This is considered a breaking change under the rules laid out in this PR. I think this is correct. In the migration path you lay out, the change in behavior is broken into multiple minor versions, but this doesn't change the fact that a value of 5 was valid in 1.0.0, and became invalid in 1.2.0. No major version change occurred yet the user's input became invalid. I think this is wrong, regardless of whether or not it is broken into steps. For authors of the schema, the fact that making validation more strict requires a major version bump is an incentive to get the validation criteria correct up front. The PR lists all the different types of validation conditions present in JSON schema for completeness, but I really don't see us using those in most cases. minLength, maxLength, format, multipleOf and others seem like really niche keywords we really ought to avoid. I do anticipate (from experience so far) we'll use keywords like pattern, propertyNames, minProperties, maxProperties, required, and enum frequently. It seems reasonable to model these correctly up front. And if we're not sure, we can start off stricter and loosen the criteria as needed, since reducing strictness is allowed.

Item (b) is about stability garantees between [SCHEMA] and [SIG] as I called it, where the SIG implementation is to expect extra processing beyond yaml parsing, mandated by the schema.

On a somewhat unrelated note: Right now the extra processing responsibilities of the SIG implementation are sometimes implied and other times made explicit in comments. This seems flimsy, and error prone as SIG maintainers may miss some subtle change in the expected semantics. To me, this implies we should minimize these extra responsibilities.


A type is the JSON schema analog to a [protobuf message](https://protobuf.dev/programming-guides/proto3/#simple). Types have a [`type`](https://json-schema.org/understanding-json-schema/reference/type) of `object`, and use various keywords to describe their properties and conditions which constitute valid data.

Stable types provide the following guarantees. All types except those excluded in [applicability](#applicability) are considered stable after a 1.0.0 release.

* Type property names will not change.
* The `type` of properties will not change, except the allowed addition of `null`.
* Type [title](https://json-schema.org/understanding-json-schema/reference/annotations) will not change.
* Types will be not change to make validation more strict. Changes may occur if they make validation less strict. This applies to the following keywords. Examples are given, but they are not exhaustive.
* [minLength, maxLength](https://json-schema.org/understanding-json-schema/reference/string): `minLength` will not increase and `maxLength` will not decrease.
* [pattern](https://json-schema.org/understanding-json-schema/reference/string#regexp): pattern will not become stricter.
* [format](https://json-schema.org/understanding-json-schema/reference/string#built-in-formats): will not change.
* [multipleOf](https://json-schema.org/understanding-json-schema/reference/numeric#multiples): will not change.
* [minimum, exclusiveMinimum, maximum, exclusiveMaximum](https://json-schema.org/understanding-json-schema/reference/numeric#range): `minimum`, `exclusiveMinimum` will not increase; `maximum`, `exclusiveMaximum` will not decrease.
* [patternProperties](https://json-schema.org/understanding-json-schema/reference/object#patternProperties): will not expand scope to restrict additional properties.
* [additionalProperties](https://json-schema.org/understanding-json-schema/reference/object#additionalproperties): will not go from `true` to `false`.
* [propertyNames](https://json-schema.org/understanding-json-schema/reference/object#propertyNames): will not become stricter.
* [minProperties, maxProperties](https://json-schema.org/understanding-json-schema/reference/object#size): `minProperties` will not increase, `maxProperties` will not decrease.
* [required](https://json-schema.org/understanding-json-schema/reference/object#required): will not add additional entries.
* [contains, minContains, maxContains](https://json-schema.org/understanding-json-schema/reference/array#contains): will not become stricter.
* [minItems, maxItems](https://json-schema.org/understanding-json-schema/reference/array#length): `minItems` will not increase, `maxItems` will not decrease.
* [uniqueItems](https://json-schema.org/understanding-json-schema/reference/array#uniqueItems): will not go from `true` to `false`.
Copy link

Choose a reason for hiding this comment

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

false to true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good catch

* [enum](https://json-schema.org/understanding-json-schema/reference/enum): will not remove entries.
* [const](https://json-schema.org/understanding-json-schema/reference/const): will not change.
* No existing type will be deleted.
Copy link
Member

Choose a reason for hiding this comment

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

Assume a format 1.0.0 (stable) that contains the Jaeger exporter, which we all know has been deprecated and removed from the spec.

What this says is that the jeager exporter will never be removed from the yaml format in version 1.x.y, as this requires a major format change.

There must be a reasonable way to deprecate and remove features, and I don't think the following is shocking:

  • format 1.0.0 does support a jeager exporter
  • format 1.1.0 removes support for jaeger, and documents this as an incompatible breaking change

Copy link
Member Author

Choose a reason for hiding this comment

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

There must be a reasonable way to deprecate and remove features, and I don't think the following is shocking:

I think there must be a way to deprecate features, but not removal. Deprecated features must linger on (potentially indefinitely) until a major version bump. This shifts the burden of understanding / dealing with schema evolution from users to maintainers, which is where it belongs.

Semantic conventions reached this same conclusion: https://opentelemetry.io/docs/specs/otel/versioning-and-stability/#deprecated

I believe (and hope) language SIGs reach this same conclusion. In java, we maintain code which we marked as stable but have deprecated with the same gaurantees as other stable code - no removal until next major version, which we have no plans of. Here's an example. We accidentally exposed a protected constructor, which is not aligned with our code style in the repository. Even though its unlikely anyone was / is using this, we've maintained it for 3 years, and will continue maintaining it.

* No type property will be deleted.

The following additive changes are allowed:

* Adding of new properties to existing types.
* Adding new types.
* Changes that make property validation less strict. See above for examples.

### Applicability

Stability guarantees do not apply to [experimental features](#experimental-features) and [extension points](#extension-points).

#### Experimental features

Sometimes we need to experiment with new types and properties. For example, to evaluate the configuration experience for experimental features in [opentelemetry-specification](https://github.com/open-telemetry/opentelemetry-specification).

Experimental properties are denoted by a `*/(development|alpha|beta)` suffix (e.g.`foo/development`). The suffix indicates the property value and all types nested within it are exempt from stability guarantees, and are subject to breaking changes in minor versions. Experimental types have a [title](https://json-schema.org/understanding-json-schema/reference/annotations) prefixed with `Experimental*` (e.g. `ExperimentalFoo`).

Maintainers are not obligated to implement support for experimental properties and types. When they do, they are under no obligation to maintain any stability guarantees.

End users should be cautious of adopting experimental properties and types, since in doing so they are subject to breaking changes in minor versions.

#### Extension points

The schema contains types which are designed for extension, as indicated by the presence of `"additionalProperties": true`. For example, [component provider](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk.md#register-componentprovider) provides mechanisms for referencing custom SDK extension components like exporters, processors, samplers, etc. The stability guarantees surrounding properties not explicitly defined in this repository is out of scope. Users should consult documentation for the components interpreting these additional properties and decide if their stability guarantees are sufficient for adoption.

### File format

The top level `.file_format` property is special in that it conveys the version of the schema a user's configuration conforms to. Implementations also target a particular version of the schema, which may or may not align with the version specified by the user.

Given the [guarantees and allowed changes](#guarantees-and-allowed-changes), implementations may encounter the following scenarios:

* The `file_format` major version aligns with the implementation major version, AND:
* The `file_format` minor version is less than or equal to the implementation minor version: This is idea, with versions maximally aligned. Despite this, an implementation might not support every property and type of its target version.
jack-berg marked this conversation as resolved.
Show resolved Hide resolved
* The `file_format` minor version is greater than the implementation minor version: The implementation should detect and emit a warning since there may be configuration features the user specifies which the implementation does not understand. However, this is acceptable in many cases, and not terribly different from the ideal path where an implementation also might not support every configuration feature.
Copy link

Choose a reason for hiding this comment

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

So, as an example: minLength is 10 in 1.1.0 and minLength is 5 for 1.2.0 (hence relaxing the value). User of 1.2.0 expects 7 to be a valid value, but for 1.1.0 it is not. What do you do here? Generate a warning and accept the value? Or ignore it?

Copy link
Member Author

Choose a reason for hiding this comment

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

User of 1.2.0 expects 7 to be a valid value, but for 1.1.0 it is not.

So the user has specified that they are using 1.2.0 by setting file_format: 1.2. The implementation is conforming to 1.1.0. The implementation detects that the user's version is great than the implementation and emits a warning, alerting the user to the fact that a 1.1.0 implementation won't be able to understand some of the changes from a 1.2.0 file format.

In this case, the implementation rejects the value as its outside of the acceptable range according to the schema its aware of. If implementations stay up to date with opentelemetry-configuration releases, it will be much more common for the implementation's version to be greater than the users (which is ideal) than for the user's version to be greater than the implementations.

* The `file_format` major version does not align with the implementation major version. The implementation should produce an error, since there may be breaking changes in the properties and semantics on how they are interpreted. Implementations may choose to temporarily support multiple major version to accommodate transitioning users.

## Schema modeling rules

Expand Down
Loading