-
Notifications
You must be signed in to change notification settings - Fork 60
Schema v2 live-check #1022
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
Schema v2 live-check #1022
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1022 +/- ##
=======================================
- Coverage 78.7% 78.6% -0.1%
=======================================
Files 82 82
Lines 6612 6701 +89
=======================================
+ Hits 5204 5270 +66
- Misses 1408 1431 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jsuereth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great! Wanted to check on V2 + custom rego policies
| pub struct TypeAdvisor; | ||
|
|
||
| /// Trait to abstract over different attribute types for checking | ||
| trait CheckableAttribute { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I like this idea
| } | ||
| _ => { | ||
| let _ = semconv_attributes.insert(attribute.name.clone(), attribute_rc); | ||
| VersionedRegistry::V2(registry) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how it's more clear what signals are supported here.
| } | ||
|
|
||
| #[test] | ||
| fn test_custom_rego() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does custom rego policy work with V2? Does this need a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, custom rego does work with V2. The test policies here were unchanged, and the builtin otel policies also. This is due to the way I provide the signal as input to the policy "untagged" by serde. If you happen to need one of the fields that has moved position in the JSON then you would need to fix your policy. We should fix this with documentation: #694
There are three more examples in these tests that exercise v1 and v2 with custom rego. This is the only one I didn't do that with because it doesn't look at the semconv attribute, only the sample so it's moot. However, I should make it a dual test for consistency. I'll do that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, cool, can you open a task on the tracking bug to update the docs for this then?
|
Great! |
Convert live-check to work with both v1 and v2 resolved registries. This has been achieved with enums and generics to limit the amount of code duplication since we'll have dual support for some time.
This approach also means that the interface to rego policies is very similar and will rarely require any changes. In fact all the rego policies for live-check in the tests are dual version compatible and therefore unchanged in this PR.