Metrics and Events don't require attributes

[jerbly]

Fix for issue #405

• Updated the EBNF and JSON schema to define the extends or attributes requirement mandatory for all group types except metric and event.
• Added a group validity check as a warning.

[lquerel]

LGTM. Thanks

[lmolkova]

LGTM and thank you!

[lquerel]

@jerbly This PR is still in draft mode. Is that intentional? If not, please click “Ready for Review,” and I’ll proceed with merging it. Thanks!

[jerbly]

@lquerel it's currently failing unrelated clippy checks. One is a simple fix. The other is documentation and formatting in the generated code in weaver-codegen - I'm not sure what to do with it. Perhaps change clippy settings in that crate?

[lquerel]

Regarding Clippy, I suggest implementing the changes proposed in Clippy’s report regarding the possibility of eliding certain lifetimes. Currently, our build pipeline is configured to always use the latest version of the Rust toolchain, which has the advantage of benefiting from the latest changes and improvements but also the drawback of introducing new Clippy errors unrelated to the PR. We could pin the toolchain version to avoid this, but at the same time, the current approach ensures we don’t forget to update it.

[jerbly]

The lifetimes one is easy. The doc problems in the generated code was where I had my doubts it was worth the effort. I'll work on it.

[jerbly]

@lquerel - I've fixed those issues so this is ready to merge now. :)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.9%. Comparing base (19c5746) to head (c7d9005).
Report is 2 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #494   +/-   ##
=====================================
  Coverage   73.9%   73.9%           
=====================================
  Files         50      50           
  Lines       3902    3911    +9     
=====================================
+ Hits        2886    2893    +7     
- Misses      1016    1018    +2     

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

[lquerel]

I missed something but I didn’t understand the changes to the code generation section. What was the issue exactly? Why comment out that part of the code generation?

[lquerel]

Sorry, I didn’t understand these changes. Why comment out this part of the code generation?

[jerbly]

Clippy was complaining about the documentation and formatting. The jinja template previously created:

/// Attributes for the `system` namespace.
pub mod system;

for example.

Then here:

weaver/crates/weaver_codegen_test/build.rs

Lines 84 to 89 in 4220324

	// For the purpose of the integration test located in `tests/codegen.rs`, we need to:
	// - Combine all generated files to form a single file containing all the generated code
	// organized in nested modules.
	// - Replace `//!` with `//`
	// - Remove `pub mod` lines
	create_single_generated_rs_file(target_dir.as_path());

the pub mod lines are removed leaving a floating /// doc comment.

[lquerel]

Ah, ok. I had forgotten about that part of the plumbing in the code :-)

[lquerel]

Same question.

[jerbly]

Same answer ;)