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

Detect and Process Policy Files into SemConv Registry + Generic Diagnostic Reporting #153

Merged
merged 20 commits into from
May 20, 2024

Conversation

lquerel
Copy link
Contributor

@lquerel lquerel commented May 9, 2024

Main updates

This PR introduces several enhancements:

  1. Policy Enforcement: It detects policy files (.rego) embedded in semantic convention registries and uses the policy engine to enforce all found policies. In this PR, only the before_resolution policy stage is supported.

  2. Enhanced Error Reporting: All WeaverError instances are now extended to support Miette diagnostics, enabling rich annotations and improved error reporting. This extension is utilized by the diagnostic message rendering process described below.

  3. Generic Diagnostic Message Output: A new mechanism for outputting diagnostic messages in various formats is introduced, including ANSI-compatible text, JSON, and gh_workflow_command. Users can create new formats by adding a Jinja template, which will be used to render diagnostic messages in the new format.

  4. Centralized Error Processing and Exit Code Determination: All errors and diagnostic messages are now processed and rendered in the main function, which also determines the exit code.

The ANSI template renders policy violations in a specific way, while all other diagnostic messages are rendered using the Miette "graphical" renderer.

Below are the newly supported parameters that can be used to configure the diagnostic rendering process:

      --diagnostic-format <DIAGNOSTIC_FORMAT>
          Format used to render diagnostic messages. Predefined formats are: ansi, json, gh_workflow_command

          [default: ansi]

      --diagnostic-template <DIAGNOSTIC_TEMPLATE>
          Path to the directory where the diagnostic templates are located

          [default: diagnostic_templates]

Other minor updates/improvements

  • Used assert_cmd to test the command line.
  • Removed the diagnostic channel infrastructure as we have decided not to pursue this direction.
  • Removed all the telemetry schema examples which are no longer supported by this version of weaver.

Future PRs

  • Implementation of the after_resolution policy stage.
  • Nice annotations on all errors/diagnostics.

@lquerel lquerel self-assigned this May 9, 2024
@lquerel lquerel added the enhancement New feature or request label May 9, 2024
crates/weaver_checker/src/lib.rs Fixed Show fixed Hide fixed
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 May 9, 2024

Codecov Report

Attention: Patch coverage is 74.61538% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 75.0%. Comparing base (b1e8588) to head (1bb4931).

Files Patch % Lines
crates/weaver_forge/src/lib.rs 58.8% 7 Missing ⚠️
crates/weaver_resolver/src/lib.rs 74.0% 7 Missing ⚠️
crates/weaver_checker/src/lib.rs 80.0% 6 Missing ⚠️
crates/weaver_common/src/diagnostic.rs 72.2% 5 Missing ⚠️
crates/weaver_forge/src/extensions/util.rs 84.2% 3 Missing ⚠️
crates/weaver_semconv_gen/src/lib.rs 40.0% 3 Missing ⚠️
crates/weaver_semconv_gen/src/gen.rs 60.0% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #153     +/-   ##
=======================================
- Coverage   75.2%   75.0%   -0.2%     
=======================================
  Files         45      43      -2     
  Lines       2704    2729     +25     
=======================================
+ Hits        2034    2049     +15     
- Misses       670     680     +10     

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

@jsuereth
Copy link
Contributor

A few thoughts:

  1. Ideally you shouldn't have a different set of policies for output, the rendering is just a matter of where you're running. E.g. you'd have something like weaver registry check ... --output console in your makefile for your dev workflow and weaver registry check ... --output gh_workflow for the github workflow. I don't think you be altering the policies along with the template output. Not sure you called this out in your flow.

  2. There's a lot of things in weaver.yaml you may want to share between outputs (e.g. acronyms). For now we can keep it separate, but I think eventually we may want something shared.

  3. I assume the registry directory is because of the registry command. Throwing diagnostics in there now feels kinda weird. We could "flatten" the templates/ directory, or expand to have something like:

templates/
   registry/
       check/
          {output_target}/
       generate/
       snippet/

With corresponding weaver registry {check|generate|snippet} commands. I personally would prefer a more flat structure, as I found I was sharing a lot of templates between the generate/markdown-update (maybe we should call this snippet?) commands.

crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_common/src/diagnostic.rs Fixed Show fixed Hide fixed
crates/weaver_forge/src/lib.rs Fixed Show fixed Hide fixed
crates/weaver_forge/src/lib.rs Fixed Show fixed Hide fixed
crates/weaver_forge/src/lib.rs Fixed Show fixed Hide fixed
@lquerel lquerel marked this pull request as ready for review May 17, 2024 23:14
@lquerel lquerel requested a review from jsuereth as a code owner May 17, 2024 23:14
@lquerel lquerel changed the title [WIP] Detect and process policy files into semconv registry Detect and Process Policy Files into SemConv Registry + Generic Diagnostic Reporting May 17, 2024
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.

So far this looks really good. There's a LOT to this so it took a while for me to sort through it all.

Have one major question around what installation of weaver looks like with these diagnostic templates, but otherwise just a lot of nits / cleanups.

crates/weaver_semconv_gen/src/gen.rs Outdated Show resolved Hide resolved
crates/weaver_common/src/diagnostic.rs Show resolved Hide resolved
crates/weaver_checker/src/lib.rs Show resolved Hide resolved
crates/weaver_checker/src/lib.rs Show resolved Hide resolved
deny.toml Show resolved Hide resolved
diagnostic_templates/ansi/errors.txt.j2 Show resolved Hide resolved
@lquerel
Copy link
Contributor Author

lquerel commented May 20, 2024

@jsuereth My goal in an upcoming PR is to embed the default templates within the binary to avoid depending on an external directory. However, a user will still be able to create an external directory to (re)define templates as they wish. We could even imagine an init command to initialize this directory with the default content.

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.

Ok to merge as long as we consider the template issue a blocker for release.

@lquerel lquerel merged commit 9c4c88f into open-telemetry:main May 20, 2024
18 of 20 checks passed
@lquerel lquerel deleted the policy-engine-ext branch May 20, 2024 21:10
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