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

Add support for post-resolution policies #214

Merged
merged 8 commits into from
Jun 26, 2024

Conversation

lquerel
Copy link
Contributor

@lquerel lquerel commented Jun 19, 2024

With this PR, it is now possible to define policies to validate a resolved semconv registry (the previous version of Weaver was only supporting pre-resolution policies).

As before, all the *.rego files are imported into the policy engine. The package names are used to identify groups of rules to apply either before or after the resolution process (package before_resolution vs package after_resolution). It is valid to define multiple policy files with the same package name. In this case, the rules defined in these files will be combined and evaluated together. This option can be used to logically split the rules per topic or domain of control.

Note: The definition of policies comparing two versions of the resolved semconv will be added in a future PR.

@lquerel lquerel requested a review from jsuereth as a code owner June 19, 2024 00:12
@lquerel lquerel self-assigned this Jun 19, 2024
@lquerel lquerel added the enhancement New feature or request label Jun 19, 2024
crates/weaver_checker/src/lib.rs Fixed Show fixed Hide fixed
crates/weaver_checker/src/lib.rs Fixed Show fixed Hide fixed
Copy link

codecov bot commented Jun 19, 2024

Codecov Report

Attention: Patch coverage is 19.14894% with 38 lines in your changes missing coverage. Please review.

Project coverage is 73.5%. Comparing base (183fdeb) to head (cfc4f25).

Current head cfc4f25 differs from pull request most recent head c94c786

Please upload reports for the commit c94c786 to get more accurate results.

Files Patch % Lines
crates/weaver_common/src/diagnostic.rs 0.0% 17 Missing ⚠️
crates/weaver_checker/src/lib.rs 39.1% 14 Missing ⚠️
crates/weaver_semconv/src/path.rs 0.0% 7 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #214     +/-   ##
=======================================
- Coverage   74.5%   73.5%   -1.0%     
=======================================
  Files         44      45      +1     
  Lines       2938    2983     +45     
=======================================
+ Hits        2189    2194      +5     
- Misses       749     789     +40     

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

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.

Just minor nits and wording concerns.

pub trait ResultExt<T, E> {
/// Captures the diagnostic messages into the provided `DiagnosticMessages`
/// or returns the value if there are no diagnostic messages.
fn capture_diag_msgs_into(self, diags: &mut DiagnosticMessages) -> Option<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like these additions. Will need to figure out if we have the right set of combinators here.

src/registry/check.rs Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
@jsuereth
Copy link
Contributor

One other comment, code coverage is pretty poor here. We need to figure out negative tests where we expect failures and diagnostic messages to print.

@lquerel
Copy link
Contributor Author

lquerel commented Jun 26, 2024

One other comment, code coverage is pretty poor here. We need to figure out negative tests where we expect failures and diagnostic messages to print.

I improved the comments, but regarding the tests, it's not easy to add more from my iPad. The overall coverage is still greater than 70%, so I'm going to merge this PR. I will create a GitHub issue to address this in a future PR once I'm back.

@lquerel lquerel merged commit 77d0d9b into open-telemetry:main Jun 26, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants