feat(instrumentation-build): add event-only generation - #500
Merged
Conversation
johanpel
changed the title
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
johanpel
marked this pull request as ready for review
August 3, 2026 08:56
johanpel
requested review from
9prady9,
cmatzenbach,
dhruv9vats,
felipeblazing,
johallar,
joosthooz and
mbrobbel
as code owners
August 3, 2026 08:56
📝 WalkthroughWalkthroughThe PR adds shared ChangesEvents and code generation
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 the quent-dynamic-attributes dependency definition into the
workspace’s [workspace.dependencies], preserving its local path there, and
update the crate-level dependency declaration to use workspace = true. Remove
the crate-local path declaration from the dependency entry.
In `@crates/events/src/entity_ref.rs`:
- Around line 17-42: Add focused tests for the public EntityRef component:
verify EntityRef::new preserves the target UUID and payload, confirm the default
T = () payload works, and add feature-gated serde coverage ensuring the phantom
_entity field is skipped during serialization/deserialization. Keep the tests
colocated with EntityRef and AnyEntity without changing their public API.
In `@crates/instrumentation-build/src/common.rs`:
- Around line 44-45: Update the path handling before the deduplication logic
using the existing derive-path processing symbols in common.rs: normalize
serde::Serialize and serde::Deserialize paths to the canonical configured form
before generating their token-string keys, so equivalent spellings retain only
one derive. Add a regression test covering both serde path spellings and verify
the generated derives compile without duplicates.
In `@crates/instrumentation-build/src/data_type.rs`:
- Around line 32-35: Replace the MAX_TYPE_DEPTH assertion in map_data_type with
a GenerateError variant for excessive nesting, change map_data_type to return
Result, and propagate that result through event, record, and handle generation.
Update the existing panic-focused test to assert the returned generation error
instead.
In `@crates/instrumentation-build/src/lib.rs`:
- Around line 200-201: Remove the opts.any_event/instrumentation validation in
the generation flow and delete the AnyEventRequiresInstrumentation error
variant. Update the existing test near the AnyEvent validation to cover
instrumentation: false with any_event: true, asserting that generated code uses
quent_events::Event-based AnyEvent output without instrumentation handles,
observers, or model requirements.
🪄 Autofix (Beta)
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: accbb56d-6f58-4238-8de4-7411e9c9e8bb
⛔ 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 (14)
crates/events/Cargo.tomlcrates/events/src/entity_ref.rscrates/events/src/lib.rscrates/instrumentation-build/example/build.rscrates/instrumentation-build/src/any_event.rscrates/instrumentation-build/src/common.rscrates/instrumentation-build/src/data_type.rscrates/instrumentation-build/src/events.rscrates/instrumentation-build/src/lib.rscrates/instrumentation-build/src/records.rscrates/instrumentation-build/src/runtime/handle.rscrates/instrumentation-build/src/runtime/mod.rscrates/instrumentation/src/entity.rscrates/instrumentation/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/instrumentation-build/example/build.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🤖 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 the quent-dynamic-attributes dependency definition into the
workspace’s [workspace.dependencies], preserving its local path there, and
update the crate-level dependency declaration to use workspace = true. Remove
the crate-local path declaration from the dependency entry.
In `@crates/events/src/entity_ref.rs`:
- Around line 17-42: Add focused tests for the public EntityRef component:
verify EntityRef::new preserves the target UUID and payload, confirm the default
T = () payload works, and add feature-gated serde coverage ensuring the phantom
_entity field is skipped during serialization/deserialization. Keep the tests
colocated with EntityRef and AnyEntity without changing their public API.
In `@crates/instrumentation-build/src/common.rs`:
- Around line 44-45: Update the path handling before the deduplication logic
using the existing derive-path processing symbols in common.rs: normalize
serde::Serialize and serde::Deserialize paths to the canonical configured form
before generating their token-string keys, so equivalent spellings retain only
one derive. Add a regression test covering both serde path spellings and verify
the generated derives compile without duplicates.
In `@crates/instrumentation-build/src/data_type.rs`:
- Around line 32-35: Replace the MAX_TYPE_DEPTH assertion in map_data_type with
a GenerateError variant for excessive nesting, change map_data_type to return
Result, and propagate that result through event, record, and handle generation.
Update the existing panic-focused test to assert the returned generation error
instead.
In `@crates/instrumentation-build/src/lib.rs`:
- Around line 200-201: Remove the opts.any_event/instrumentation validation in
the generation flow and delete the AnyEventRequiresInstrumentation error
variant. Update the existing test near the AnyEvent validation to cover
instrumentation: false with any_event: true, asserting that generated code uses
quent_events::Event-based AnyEvent output without instrumentation handles,
observers, or model requirements.
🪄 Autofix (Beta)
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: accbb56d-6f58-4238-8de4-7411e9c9e8bb
⛔ 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 (14)
crates/events/Cargo.tomlcrates/events/src/entity_ref.rscrates/events/src/lib.rscrates/instrumentation-build/example/build.rscrates/instrumentation-build/src/any_event.rscrates/instrumentation-build/src/common.rscrates/instrumentation-build/src/data_type.rscrates/instrumentation-build/src/events.rscrates/instrumentation-build/src/lib.rscrates/instrumentation-build/src/records.rscrates/instrumentation-build/src/runtime/handle.rscrates/instrumentation-build/src/runtime/mod.rscrates/instrumentation/src/entity.rscrates/instrumentation/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/instrumentation-build/example/build.rs
🛑 Comments failed to post (1)
crates/events/src/entity_ref.rs (1)
17-42: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add tests for the new entity-reference component.
EntityRefandAnyEntityare new public types, but this file has no tests. Add coverage forEntityRef::new, the default payload type, and serde behavior when the feature is enabled.As per coding guidelines, “New Rust components must include accompanying 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/events/src/entity_ref.rs` around lines 17 - 42, Add focused tests for the public EntityRef component: verify EntityRef::new preserves the target UUID and payload, confirm the default T = () payload works, and add feature-gated serde coverage ensuring the phantom _entity field is skipped during serialization/deserialization. Keep the tests colocated with EntityRef and AnyEntity without changing their public API.Source: Coding guidelines
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
crates/instrumentation-build/src/lib.rs-157-158 (1)
157-158: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
TypeNestingTooDeep.
GenerateError::TypeNestingTooDeepis public throughGenerateError. Add a doc comment that states when generation returns this variant.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/instrumentation-build/src/lib.rs` around lines 157 - 158, Add a Rust doc comment to the public GenerateError::TypeNestingTooDeep variant describing the generation condition that returns it: type nesting exceeds the configured maximum depth. Keep the existing error annotation and visibility unchanged.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.
Other comments:
In `@crates/instrumentation-build/src/lib.rs`:
- Around line 157-158: Add a Rust doc comment to the public
GenerateError::TypeNestingTooDeep variant describing the generation condition
that returns it: type nesting exceeds the configured maximum depth. Keep the
existing error annotation and visibility unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: c1e861b2-b303-4d99-8b99-ad6bd3407ef9
📒 Files selected for processing (8)
Cargo.tomlcrates/events/Cargo.tomlcrates/instrumentation-build/src/common.rscrates/instrumentation-build/src/data_type.rscrates/instrumentation-build/src/events.rscrates/instrumentation-build/src/lib.rscrates/instrumentation-build/src/records.rscrates/instrumentation-build/src/runtime/handle.rs
This was referenced Aug 3, 2026
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
johanpel
removed request for
9prady9,
cmatzenbach,
dhruv9vats,
felipeblazing,
johallar and
mbrobbel
August 4, 2026 08:27
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/instrumentation-build/src/runtime/mod.rs`:
- Around line 56-68: The public rename from Entity to InstrumentedEntity breaks
downstream users of quent_instrumentation::Entity. Add a compatibility re-export
alias, preferably pub use InstrumentedEntity as Entity, in the existing
quent_instrumentation API while preserving InstrumentedEntity for current code.
🪄 Autofix (Beta)
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: e941440d-9247-4afa-8e0f-8423019703c8
📒 Files selected for processing (8)
crates/instrumentation-build/src/common.rscrates/instrumentation-build/src/lib.rscrates/instrumentation-build/src/runtime/mod.rscrates/instrumentation/src/entity.rscrates/instrumentation/src/handle.rscrates/instrumentation/src/lib.rscrates/instrumentation/src/model.rsintegrations/nvtx/example/tests/thread_id.rs
Comment on lines
+56
to
+68
| pub struct Handle<E: ::quent_instrumentation::InstrumentedEntity<Context = Context<#model>>> { | ||
| inner: ::quent_instrumentation::HandleInner<E>, | ||
| } | ||
|
|
||
| impl<E: ::quent_instrumentation::Entity<Context = Context<#model>>> | ||
| impl<E: ::quent_instrumentation::InstrumentedEntity<Context = Context<#model>>> | ||
| ::core::convert::From<::quent_instrumentation::HandleInner<E>> for Handle<E> | ||
| { | ||
| fn from(inner: ::quent_instrumentation::HandleInner<E>) -> Self { | ||
| Self { inner } | ||
| } | ||
| } | ||
|
|
||
| impl<E: ::quent_instrumentation::Entity<Context = Context<#model>>> ::core::ops::Deref | ||
| impl<E: ::quent_instrumentation::InstrumentedEntity<Context = Context<#model>>> ::core::ops::Deref |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 \
'quent_instrumentation::Entity|pub\s+(trait|type)\s+Entity\b|InstrumentedEntity' \
crates/instrumentation crates/instrumentation-buildRepository: rapidsai/quent
Length of output: 13133
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- instrumentation public exports and trait definitions ---'
sed -n '1,80p' crates/instrumentation/src/lib.rs
sed -n '1,45p' crates/instrumentation/src/entity.rs
printf '%s\n' '--- repository references to the old and new names ---'
rg -n -C 3 \
'\bquent_instrumentation::Entity\b|\bInstrumentedEntity\b|\bpub\s+(trait|type)\s+Entity\b' \
--glob '!target/**' --glob '!dist/**' .
printf '%s\n' '--- generated runtime tests and implementation ---'
sed -n '45,125p' crates/instrumentation-build/src/runtime/mod.rs
sed -n '330,370p' crates/instrumentation-build/src/lib.rs
printf '%s\n' '--- recent file-level change summary ---'
git diff --statRepository: rapidsai/quent
Length of output: 19180
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- commit context ---'
git log --oneline -5 -- crates/instrumentation/src/entity.rs crates/instrumentation/src/lib.rs crates/instrumentation-build/src/runtime/mod.rs
printf '%s\n' '--- previous public API ---'
parent="$(git rev-parse HEAD^)"
for file in crates/instrumentation/src/entity.rs crates/instrumentation/src/lib.rs crates/instrumentation-build/src/runtime/mod.rs; do
printf '\n--- %s at %s ---\n' "$file" "$parent"
git show "$parent:$file" 2>/dev/null | sed -n '1,135p' || true
done
printf '%s\n' '--- compatibility and migration documentation ---'
rg -n -i -C 2 \
'compatib|migrat|breaking|InstrumentedEntity|quent_instrumentation::Entity' \
README.md docs crates --glob '!target/**' --glob '!dist/**' 2>/dev/null || trueRepository: rapidsai/quent
Length of output: 21349
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- refactor commit summaries ---'
git show --stat --oneline 89f7c6d 9406f2d
printf '%s\n' '--- instrumentation trait before and after the refactor ---'
for rev in 89f7c6d^ 89f7c6d 9406f2d^ 9406f2d; do
printf '\n--- %s ---\n' "$rev"
git show "$rev:crates/instrumentation/src/entity.rs" 2>/dev/null \
| sed -n '1,35p' || true
git show "$rev:crates/instrumentation/src/lib.rs" 2>/dev/null \
| rg -n -C 2 'pub use .*entity|Entity|InstrumentedEntity' || true
done
printf '%s\n' '--- runtime generator trait references across the refactor ---'
for rev in 89f7c6d^ 89f7c6d 9406f2d^ 9406f2d; do
printf '\n--- %s ---\n' "$rev"
git show "$rev:crates/instrumentation-build/src/runtime/mod.rs" 2>/dev/null \
| rg -n -C 2 'quent_instrumentation::(Entity|InstrumentedEntity)|impl .*Entity' || true
doneRepository: rapidsai/quent
Length of output: 11658
Preserve or document the Entity rename
quent_instrumentation::Entity was publicly exported before this change and is now absent. Downstream bounds and implementations using it will fail to compile. Add pub use InstrumentedEntity as Entity, or document the migration and add compatibility coverage.
🤖 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/instrumentation-build/src/runtime/mod.rs` around lines 56 - 68, The
public rename from Entity to InstrumentedEntity breaks downstream users of
quent_instrumentation::Entity. Add a compatibility re-export alias, preferably
pub use InstrumentedEntity as Entity, in the existing quent_instrumentation API
while preserving InstrumentedEntity for current code.
Contributor
Author
|
Just an audit failure fixed in #530 /merge |
Contributor
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Allow
quent-instrumentation-buildto generate event types without exposing or depending on the full instrumentation surface.instrumentationoption, enabled by default for compatibility.AnyEventtypes when instrumentation is disabled.debugandserdederive options. Debug is enabled by default; serde remains opt-in.quent-eventsand re-export them fromquent-instrumentation.Related issues
#491
Testing
Passed:
pixi run cargo fmt --all -- --checkpixi run cargo test -p quent-instrumentation-build -p quent-events -p quent-instrumentationpixi run cargo clippy -p quent-instrumentation-build -p quent-events -p quent-instrumentation --all-features --all-targets -- -D warningsWritten by Codex.