Skip to content
This repository has been archived by the owner on Jan 29, 2025. It is now read-only.

Add versioning adr #135

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

Conversation

k88hudson
Copy link
Contributor

Still working on this, but I wanted to document some of the potential options (and a recommendation) for how to managing versioning across Nimbus.

As per ADR-0002, we decided to use
[JSON Schema Draft-07](https://tools.ietf.org/html/draft-handrews-json-schema-01) for the Nimbus
Experiment Schema. Besides having some way of validating data across these applications, we also
have to manage the additional complexity that any given time experiments may be live that are only
Copy link

Choose a reason for hiding this comment

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

nit "that at any"

compatible with a subset of clients.

In order to prevent clients from receiving experiment defitions that could cause breakage while
preserving the flexibility to update the Nimbus Experiment Schema when necessary, we need:
Copy link

Choose a reason for hiding this comment

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

I don't have a lot of historical context here, so I'm curious: what sort of breakage have bad experiment definitions caused in the past? Clients not enrolling in experiments that should, or enrolling in ones they shouldn't? Crashers or other user-visible bugs?

Copy link

Choose a reason for hiding this comment

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

This feeds into a broader point that I raised with @k88hudson in slack, but would welcome further opinions on: I think it's hard to define a meaningful versioning policy without understanding what you're trying to communicate or prevent; IOW you need to define what "breaking" means before you can determine whether something is a "breaking change".

For SemVer, "breaking" means "if a consumer updates to this version, there is some chance that their code will no longer compile/run correctly". What does "breaking" mean for the consumers of these experiment definitions?

For the Client SDK I can think of several:

  1. A client that we want to have enrolled in the experiment, fails to do so, potentially skewing the data.
  2. A client that we didn't want to enroll in the experiment, does so, potentially skewing the data.
  3. A client trying to interpret this experiment info will crash or otherwise behave badly.

I don't think versioning is a good mechanism to prevent (3), which seems better served by having clients aggressively ignore experiment records that don't validate against their expected schema. But there does seem to be value in using a version number to manage (1) and (2).

I'm skeptical of needing three levels of versioning, at least from the client SDK perspective. A MAJOR.MINOR schema (or perhaps MODEL.REVISION in the style of schemaver) seems sufficient. Something like:

  • Is the MAJOR number on this record larger than the one we support? Ignore it, we might do the wrong thing even if we manage to parse it correctly.
  • Is the MINOR number on this record larger than the one we support? Attempt to validate/parse/execute it, but log and ignore if that fails.
  • Otherwise, attempt to validate/parse/execute it and treat failure to do so as a noteworthy error for telemetry etc.

The third level of versioning may be useful for other consumers though, in which case I've no objection to it.

versions of the SDK, using per-record versioning would allow us to update to a new schema without
breaking older clients or disrupting existing experiments. We would need to:

- associate each version of the schema with a version
Copy link

Choose a reason for hiding this comment

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

FWIW I'm not sure I properly parsed the distinction between the two uses of "version" in this sentence. Do you mean that every time we change the schema we need to give it a new version number?

## Context and Problem Statement

As per ADR-0002, we decided to use
[JSON Schema Draft-07](https://tools.ietf.org/html/draft-handrews-json-schema-01) for the Nimbus
Copy link

Choose a reason for hiding this comment

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

The JSON schema contains several stringly-typed JEXL fields which AFAICT are not validated, such as the targeting expression. Would the versioning discussed here cover changes to the semantics of those fields as well? I assume so, but it may be worth calling out explicitly that a change in the version number might not map directly to an observable change in the JSON Schema definitions.

#### Pros

- No additional client-side versioning logic
- Simpler to implement for Experimenter
Copy link

@rfk rfk Oct 7, 2020

Choose a reason for hiding this comment

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

One pro of this option is that there are no choices we can make right now that would prevent us from exercising this option in the future :-)

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Just a couple of nits I noticed while reading through this.

Another thing this document might want to consider is how to communicate/document schema changes - eg, as the maintainer of a consumer that currently understands one version, how do I learn what changes were made to the current schema?

#### Pros

- Allows us to introduce breaking changes without overhead of a new RS bucket
- Semver is realtively familiar, SemVer comparison implementations widely available
Copy link
Member

Choose a reason for hiding this comment

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

nit - relatively


### Option B - Use major version on API + application version ranges on records

This is what we currently do for other services in Fx Desktop like Mesasging System, including
Copy link
Member

Choose a reason for hiding this comment

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

nit: Messaging

@tdsmith
Copy link
Contributor

tdsmith commented Jan 12, 2021

I stole your serial number, sorry 😬 Nevermind, I'll renumber

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants