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

BREAKING CHANGE: Support stability definitions from OTEP 232 #504

Merged
merged 10 commits into from
Dec 6, 2024

Conversation

jerbly
Copy link
Contributor

@jerbly jerbly commented Dec 5, 2024

BREAKING CHANGE:

  • Issue #502 - Support stability definitions from OTEP 232
    • Stability enum now has these variants: stable, development, deprecated, alpha, beta, release_candidate
    • unmaintained is not supported yet.
    • experimental is still accepted when parsing but aliased to development.
    • The minijinja test, experimental, now returns true for any variant other than stable and deprecated.
    • EBNF and JSON schema updated to define the new enum without the experimental variant.

@jerbly jerbly marked this pull request as ready for review December 5, 2024 03:57
@jerbly jerbly requested review from a team as code owners December 5, 2024 03:57
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Overall this looks great.

My only concern is unexpected breakages.

  • We should likely call this out as a breaking change.
  • We should update the default JQ functions (e.g. exlcude_stability) to keep working
  • We still will have breakages, possibly in REGO policies, depending on how they're written. I think this is manageable, but we should call out "BREAKING" in the PR description. I think the work you've done means likely this ONLY breaks the semantic conventions repo, but not codegen.

cc @lmolkova / @lquerel for thoughts on this.

@lmolkova
Copy link
Contributor

lmolkova commented Dec 5, 2024

I do agree with @jsuereth that we should call out it is breaking - I will join maintainers call on Monday and socialize it. When SIGs run codegen they hopefully should be able to easily detect it and the fix should be straightforward.

There are also just a few repos affected - https://github.com/search?q=org%3Aopen-telemetry+exclude_stability+language%3AYAML&type=code&l=YAML

@jsuereth
Copy link
Contributor

jsuereth commented Dec 5, 2024

I do agree with @jsuereth that we should call out it is breaking - I will join maintainers call on Monday and socialize it. When SIGs run codegen they hopefully should be able to easily detect it and the fix should be straightforward.

There are also just a few repos affected - https://github.com/search?q=org%3Aopen-telemetry+exclude_stability+language%3AYAML&type=code&l=YAML

I think we can fix that usage (either in this PR or a follow on). I was more worried about this: https://github.com/search?q=org%3Aopen-telemetry+%22.stability%22+language%3A%22Open+Policy+Agent%22&type=code

However, looking there, it's only used by semconv so far AND we only rely on the string "stable", so I don't think this will break anything there.

@jerbly I'll send you a snippet for default JQ helpers if you don't get to this before tomorrow.

@jsuereth
Copy link
Contributor

jsuereth commented Dec 5, 2024

@jerbly So, in defaults/jq/semconv.jq, you want to add this helper function:

# Expands stability array that previously used "experimental" for equivalent new strings.
def expand_stability($stability):
  if $stability | index("experimental") then
    $stability + [ "development", "alpha", "beta", "release_candidate" ] 
  else
    . 
  end;

Then modify the methods semconv_signal and semconv_attributes so anywhere you see $options.exclude_stability replace it with expand_stability($options.exclude_stability).

This should ensure that codegen which splits stable from experimental continues to work.

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.9%. Comparing base (4cb5868) to head (70e7170).

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #504     +/-   ##
=======================================
- Coverage   73.9%   73.9%   -0.1%     
=======================================
  Files         50      50             
  Lines       3911    3914      +3     
=======================================
+ Hits        2894    2895      +1     
- Misses      1017    1019      +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jerbly
Copy link
Contributor Author

jerbly commented Dec 6, 2024

@jsuereth - I made your change to the default semconv.jq (couldn't use . in the else clause for some reason??). I also reverted one of the filters in weaver.yaml to exercise this:

- template: semconv_grouped_attributes_without_experimental.json
    filter: >
      semconv_grouped_attributes({
        "exclude_root_namespace": ["url", "network"], 
        "exclude_stability": ["experimental"]
      })
    application_mode: single

@lmolkova - committed your change so the experimental test means non-stable.

@jerbly jerbly changed the title Support stability definitions from OTEP 232 BREAKING CHANGE: Support stability definitions from OTEP 232 Dec 6, 2024
@jsuereth
Copy link
Contributor

jsuereth commented Dec 6, 2024

I made your change to the default semconv.jq (couldn't use . in the else clause for some reason??).

Ah right, the . is super contextual in jq. The fix you made is right, sorry about that!

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Thanks so much for this @jerbly !

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.

LGTM. Thank you Jeremy!

Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

Thank you!!!

@jsuereth jsuereth enabled auto-merge (squash) December 6, 2024 17:21
@jsuereth jsuereth merged commit 680b9c9 into open-telemetry:main Dec 6, 2024
20 checks passed
@jerbly jerbly deleted the new-stability-definitions branch December 6, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants