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

(chore) Move backwards compatibility checks to weaver. #1327

Merged
merged 22 commits into from
Aug 20, 2024

Conversation

jsuereth
Copy link
Contributor

@jsuereth jsuereth commented Aug 8, 2024

Fixes #1039 - Migrate backwards compatibility checks to weaver.

  • Update github actions to use ONE policy check
  • Move backwards compat check to weaver policies.

Rules we enforce:

  • Attributes
    • Attributes cannot be removed
    • Attributes cannot "degrade" stability (stable->experimental)
    • Stable attributes cannot change type
  • Enum members
    • Stable members cannot change stability
    • Values cannot change
    • ids cannot be removed
  • Metrics
    • metrics cannot be removed
    • Stable metrics cannot become unstable
    • Stable Metric units cannot change
    • Stable Metric instruments cannot change
    • Set of required/recommended attributes must remain the same

Example errors:

Stable metrics cannot change required/recommended attributes:

Violation: Metric 'http.client.request.duration' cannot change required/recommended attributes (missing '{"http.request.method"}')
  - Category         : backward_compatibility
  - Type             : semconv_attribute
  - SemConv group    : metric.http.client.request.duration
  - SemConv attribute: 
  - Provenance: /source

Stable metrics cannot change unit type:

Violation: Metric 'http.server.request.duration' cannot change unit (was 's', now: 'ms')
  - Category         : backward_compatibility
  - Type             : semconv_attribute
  - SemConv group    : metric.http.server.request.duration
  - SemConv attribute: 
  - Provenance: /source

Stable metrics cannot become unstable

Violation: Metric 'http.server.request.duration' cannot change from stable
  - Category         : backward_compatibility
  - Type             : semconv_attribute
  - SemConv group    : metric.http.server.request.duration
  - SemConv attribute: 
  - Provenance: /source

Metrics cannot be removed

Violation: Metric 'http.server.request.duration' no longer exists in semantic conventions
  - Category         : backward_compatibility
  - Type             : semconv_attribute
  - SemConv group    : metric.http.server.request.duration
  - SemConv attribute: 
  - Provenance: /source
Violation: Attribute 'http.request.header' no longer exists in the attribute registry
  - Category         : backward_compatibility
  - Type             : semconv_attribute
  - SemConv group    : registry.http
  - SemConv attribute: http.request.header
  - Provenance: /source

Enums cannot lose stable members

Violation: Enum 'http.request.method' had member 'connect', but is no longer defined
  - Category         : backward_compatibility
  - Type             : semconv_attribute
  - SemConv group    : registry.http
  - SemConv attribute: http.request.method
  - Provenance: /source

Enum ids cannot change stable status

Violation: Enum 'http.request.method' had stable member 'get', but is no longer stable
  - Category         : backward_compatibility
  - Type             : semconv_attribute
  - SemConv group    : registry.http
  - SemConv attribute: http.request.method
  - Provenance: /source

Enum stable values cannot change (for same id)

Violation: Enum 'http.request.method' had stable value 'HEAD', but is now 'BAD'
  - Category         : backward_compatibility
  - Type             : semconv_attribute
  - SemConv group    : registry.http
  - SemConv attribute: http.request.method
  - Provenance: /source

Attributes cannot be removed form registry

Violation: Attribute 'http.request.header' no longer exists in the attribute registry
  - Category         : backward_compatibility
  - Type             : semconv_attribute
  - SemConv group    : registry.http
  - SemConv attribute: http.request.header
  - Provenance: /source

Stable Attributes cannot change type

Violation: Attribute 'http.request.resend_count' was 'int', but has new type 'string'
  - Category         : backward_compatibility
  - Type             : semconv_attribute
  - SemConv group    : registry.http
  - SemConv attribute: http.request.resend_count
  - Provenance: /source

Stable attributes cannot become experimental/other.

Violation: Attribute 'http.response.header' was stable, but has new stability marker
  - Category         : backward_compatibility
  - Type             : semconv_attribute
  - SemConv group    : registry.http
  - SemConv attribute: http.response.header
  - Provenance: /source

@jsuereth jsuereth requested review from a team August 8, 2024 14:55
Makefile Outdated Show resolved Hide resolved
policies/compatibility.rego Show resolved Hide resolved
policies/compatibility.rego Outdated Show resolved Hide resolved
@jsuereth jsuereth added the Skip Changelog Label to skip the changelog check label Aug 8, 2024
Makefile Outdated Show resolved Hide resolved
@jsuereth jsuereth merged commit bad3685 into open-telemetry:main Aug 20, 2024
14 checks passed
@jsuereth jsuereth deleted the wip-backwards-compat-weaver branch August 20, 2024 15:29
Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

I really like that we're reusing the Rego framework to test our policies, but I think that to properly/fully test them, we need integration in Weaver instead of relying solely on OPA. The reasoning is as follows:

  • The names of the policy packages are meaningful for Weaver, unlike with OPA, so ideally, we need to check that too.
  • The Rego extensions that we will likely introduce in Weaver over time will only be testable within Weaver.
  • This isn't specific to Weaver, but in addition to the tests, we could combine both tests and Rego code coverage to aid the policy review process (code coverage is supported by Regorus).

# Find stable baseline attributes in latest registry.
some attr in baseline_attributes
attr.stability == "stable"
some nattr in registry_attributes
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the instances where rewriting registry_attributes as shown below will accelerate the rules. Instead of having a list of attributes, we rely on a map of attribute name -> attribute.

registry_attributes := [name: attr |
    some g in data.semconv.registry_groups
    some attr in g.attributes
    name := attr.name
]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Label to skip the changelog check
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Migrate semantic-conventions compatibility check to weaver
4 participants