feat(events): generate opt-in model event umbrellas - #518
Conversation
2fdec36 to
80e3420
Compare
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
80e3420 to
560774c
Compare
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
📝 WalkthroughWalkthroughChangesThe PR separates model metadata from instrumentation traits, adds shared model and event contracts, generates model and umbrella event code, and migrates runtime and macro integrations to the new APIs. Model and event contract refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
crates/build-info/src/lib.rs-278-278 (1)
278-278: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate the test module on
sidecarinstead ofserde.The test module uses
serde_jsonthroughout, butserde_jsonis only available when thesidecarfeature is enabled. Theserdefeature alone enables onlydep:serde. A build withserdeenabled but notsidecarwill fail to compile the test module. Change the module guard tosidecar, which already includesserdeas a dependency.Proposed fix
-#[cfg(all(test, feature = "serde"))] +#[cfg(all(test, feature = "sidecar"))] mod tests {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/build-info/src/lib.rs` at line 278, Update the test module’s cfg guard from the serde feature to the sidecar feature so the module only compiles when serde_json is available; leave the test implementation unchanged.crates/events/src/lib.rs-13-15 (1)
13-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new public re-exports.
These re-exports expand the public API but have no item documentation.
crates/events/src/lib.rs#L13-L15: add docs that describe the build-info, dynamic-attribute, and UUID re-exports.crates/instrumentation/src/lib.rs#L20-L20: add docs that describe the InstrumentedModel re-export.crates/instrumentation/src/lib.rs#L31-L31: add docs that describe the Model and ModelEvents re-exports.As per path instructions, “New
pubitems need a doc comment and justified visibility; preferpub(crate)ordoc(hidden)when not part of the curated public API.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/events/src/lib.rs` around lines 13 - 15, Add documentation comments to the public re-exports across three locations to document their purpose and visibility. At crates/events/src/lib.rs lines 13-15, add doc comments above each of the three pub use statements describing what build_info, DynamicAttributes, and Uuid provide. At crates/instrumentation/src/lib.rs line 20, add a doc comment above the InstrumentedModel re-export describing its purpose. At crates/instrumentation/src/lib.rs line 31, add doc comments above the Model and ModelEvents re-exports describing their purpose. Each comment should clearly explain what the re-exported item is and why it is part of the public API.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/events/Cargo.toml`:
- Line 12: Move quent-build-info into the root workspace dependencies using the
specified crates/build-info path, then update the dependency declaration in the
events crate to use workspace = true while preserving default-features = false.
---
Other comments:
In `@crates/build-info/src/lib.rs`:
- Line 278: Update the test module’s cfg guard from the serde feature to the
sidecar feature so the module only compiles when serde_json is available; leave
the test implementation unchanged.
In `@crates/events/src/lib.rs`:
- Around line 13-15: Add documentation comments to the public re-exports across
three locations to document their purpose and visibility. At
crates/events/src/lib.rs lines 13-15, add doc comments above each of the three
pub use statements describing what build_info, DynamicAttributes, and Uuid
provide. At crates/instrumentation/src/lib.rs line 20, add a doc comment above
the InstrumentedModel re-export describing its purpose. At
crates/instrumentation/src/lib.rs line 31, add doc comments above the Model and
ModelEvents re-exports describing their purpose. Each comment should clearly
explain what the re-exported item is and why it is part of the public API.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 052d01be-5aa2-4e24-94cf-d06de3517a66
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!Cargo.lockcrates/instrumentation-build/example/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
crates/build-info/Cargo.tomlcrates/build-info/src/lib.rscrates/codegen/src/cxx_bridge.rscrates/codegen/src/pyo3_bridge.rscrates/events/Cargo.tomlcrates/events/src/lib.rscrates/instrumentation-build/src/lib.rscrates/instrumentation-build/src/model.rscrates/instrumentation-build/src/namespace.rscrates/instrumentation-build/src/runtime/context.rscrates/instrumentation-build/src/runtime/mod.rscrates/instrumentation/src/entity.rscrates/instrumentation/src/lib.rscrates/instrumentation/src/model.rscrates/model-macros/src/entity_macro.rscrates/model-macros/src/fsm_macro.rscrates/model-macros/src/model_macro.rscrates/model-macros/src/resource_derive.rscrates/model/src/lib.rscrates/model/tests/define_model.rscrates/model/tests/entity_and_events.rscrates/open/src/viewer.rs
💤 Files with no reviewable changes (1)
- crates/open/src/viewer.rs
| serde = ["dep:serde", "quent-dynamic-attributes/serde", "uuid/serde"] | ||
|
|
||
| [dependencies] | ||
| quent-build-info = { path = "../build-info", default-features = false } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'quent-build-info\s*=' Cargo.toml crates/events/Cargo.tomlRepository: rapidsai/quent
Length of output: 456
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- workspace dependency declarations ---'
rg -n -C 3 'quent-build-info|workspace.dependencies' Cargo.toml crates/*/Cargo.toml
printf '%s\n' '--- relevant manifests ---'
cat -n Cargo.toml | sed -n '1,100p'
cat -n crates/events/Cargo.toml | sed -n '1,40p'Repository: rapidsai/quent
Length of output: 7305
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- workspace dependency section ---'
cat -n Cargo.toml | sed -n '120,240p'
printf '%s\n' '--- build-info manifest ---'
cat -n crates/build-info/Cargo.toml | sed -n '1,120p'
printf '%s\n' '--- all direct build-info dependency forms ---'
rg -n -C 1 'quent-build-info\s*=' --glob 'Cargo.toml'Repository: rapidsai/quent
Length of output: 2023
Use the workspace dependency declaration. Add quent-build-info = { path = "crates/build-info" } under [workspace.dependencies], then change line 12 to quent-build-info = { workspace = true, default-features = false }. The current direct path dependency violates the workspace dependency policy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/events/Cargo.toml` at line 12, Move quent-build-info into the root
workspace dependencies using the specified crates/build-info path, then update
the dependency declaration in the events crate to use workspace = true while
preserving default-features = false.
Source: Path instructions
|
/merge |
Description
Generate model markers independently of instrumentation and optionally generate nested, model-wide event enums through
Options::umbrella_event.Generated umbrellas:
ModelEvents::UmbrellaEvent;AnyEventtype erasure.This also separates shared event/model contracts from instrumentation-specific contracts, records the analyzer package in model provenance, and
makes build-info serialization and sidecar support optional.
Umbrella generation remains disabled by default. Generated Rust name collisions are intentionally reported by the Rust compiler rather than
prevalidated by the generator.
ModelInfo::type_pathis removed because analyzer discovery uses the model name, package, and analyzer package. New readers accept legacy sidecarscontaining that field; old readers cannot deserialize newly written sidecars without it.
Testing
pixi run cargo fmt --all -- --checkpixi run cargo test -p quent-events -p quent-instrumentation -p quent-instrumentation-build --all-featurespixi run cargo clippy -p quent-events -p quent-instrumentation -p quent-instrumentation-build --all-targets --all-features -- -D warningspixi run cargo test -p quent-build-info --all-featuresWritten by Codex.
Related Issues
Part of #491 and #191
Ultimately not needed after migrating analysis to something like analysis-build, but for now this helps migrating the instrumentation / event side over to schema arch.