refactor(instrumentation-build): use generic runtime types - #466
Conversation
aa3333a to
42bb324
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe instrumentation runtime now separates inner runtime state from model contexts, adds typed entity and observer abstractions, updates generated code and bindings, and migrates benchmarks, tests, examples, macros, and NVTX integration to the new APIs. ChangesInstrumentation runtime and generated API
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/instrumentation/src/handle.rs (1)
29-113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd direct unit tests for the new typed Entity/Observer/Model/Context/HandleInner core. This new public API surface — the central refactor of this PR — is currently only exercised indirectly (codegen string-matching assertions and a non-
#[test]example binary), not by#[test]s inquent-instrumentationitself.
crates/instrumentation/src/handle.rs#L29-L113: add tests foremit_once's bit tracking andHandleError::OnceAlreadyEmitted, plus the newas_entity_ref*/as_any_entity_ref*builders.crates/instrumentation/src/entity.rs#L37-L56: add a test thatObserver::handle_with_id(id)produces a handle whoseuuid()equalsid, and thathandle()produces fresh, distinct ids.crates/instrumentation/src/model.rs#L46-L89: add a test constructing a minimalContext<M>(a hand-writtenModelimpl, or reuse a generated test schema) and exercisingtry_new/observer/id.🤖 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/src/handle.rs` around lines 29 - 113, Add direct #[test] coverage for the new core APIs: in crates/instrumentation/src/handle.rs:29-113, test emit_once bit tracking, OnceAlreadyEmitted errors, and all as_entity_ref*/as_any_entity_ref* builders; in crates/instrumentation/src/entity.rs:37-56, verify handle_with_id preserves the supplied UUID and handle produces distinct UUIDs; in crates/instrumentation/src/model.rs:46-89, construct a minimal Context<M> and exercise try_new, observer, and id.Source: Coding guidelines
🟡 Other comments (1)
crates/instrumentation-build/src/lib.rs-146-152 (1)
146-152: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the public error variant.
GeneratedTypeCollisionis externally matchable, but only its fields are documented. Add Rustdoc stating when generation returns this error.Proposed fix
+ /// Returned when a generated observer type name collides with a schema type. #[error("generated observer type `{generated}` conflicts with schema type `{schema_path}`")] GeneratedTypeCollision {As per path instructions, “New
pubitems need a doc comment and justified visibility.”🤖 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 146 - 152, Add a Rustdoc comment directly above the public GeneratedTypeCollision variant explaining that generation returns it when a generated observer type name conflicts with a schema type name. Keep the existing field documentation and visibility unchanged.Source: Path instructions
🧹 Nitpick comments (3)
crates/instrumentation-build/src/runtime/mod.rs (1)
140-175: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo negative test for
GeneratedTypeCollision.Only the happy path is asserted here; there's no test exercising a schema whose entity/record name collides with
{Schema}Observers(or the other reserved names raised in the consolidated comment). Consider adding one alongsidegenerates_generic_instrumentation_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/runtime/mod.rs` around lines 140 - 175, Add a negative test alongside generates_generic_instrumentation_api that builds a schema containing an entity or record named after the generated reserved types, including {Schema}Observers and other names covered by GeneratedTypeCollision. Assert schema/runtime generation rejects the collision with the expected GeneratedTypeCollision error, while leaving the existing happy-path test unchanged.Source: Coding guidelines
crates/instrumentation-build/src/runtime/context.rs (2)
17-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCollision detection for generator-reserved identifiers is incomplete.
observer_storage'sGeneratedTypeCollisioncheck only guards the{Schema}Observersname, butentity_types/reexportsinject several more bare names into the same generated module scope that can equally collide with a user-named entity or record, producing a confusing raw compiler error (duplicate definition/ambiguous import) instead of the intended actionable diagnostic.
crates/instrumentation-build/src/runtime/context.rs#L17-L54: extend the reserved-name check to also cover the schema model marker name (model_ident) and the names inreexports(), e.g. via a shared constant list of reserved idents checked once against all entity/record paths.crates/instrumentation-build/src/runtime/mod.rs#L51-L89:entity_types's literalHandleident andreexports()'s bare names (Context,Observer,AnyEntity,EntityRef,Event,HandleError,DynamicAttributes,Uuid) should participate in the same collision check.🤖 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/context.rs` around lines 17 - 54, The reserved-name collision check spanning crates/instrumentation-build/src/runtime/context.rs:17-54 and crates/instrumentation-build/src/runtime/mod.rs:51-89 is incomplete. Define a shared reserved-identifier set covering observers_ident, model_ident, entity_types’ Handle, and every bare name emitted by reexports (Context, Observer, AnyEntity, EntityRef, Event, HandleError, DynamicAttributes, and Uuid), then have observer_storage validate all entity and record paths against it and report GeneratedTypeCollision consistently.Source: Path instructions
56-151: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEntity observers are built sequentially, not concurrently, unlike sibling bridges.
Each field of the generated struct literal is a separate
context.observer::<EventTy>(...).await?; Rust evaluates struct-literal field initializers left-to-right, so with N entities this serializes N independent exporter-construction futures.crates/codegen/src/cxx_bridge.rs'semit_context_bridgeandcrates/codegen/src/pyo3_bridge.rs'semit_contextbuild the analogous per-entity observers concurrently via#q::tokio::try_join!(...)for the same conceptual step. For schemas with several entities and I/O-bound exporters (e.g. network-backed exporters), this makes schema-driven context construction slower than the macro/FFI-bridge paths without a clear reason for the difference.♻️ Suggested direction: build via `tokio::try_join!` instead of sequential struct-literal awaits
fn observer_storage_initializer(schema: &Schema, active: bool) -> TokenStream { let storage = observers_ident(schema); - let entity_fields = schema.entities().map(|entity| { - let field = entity_observer_field(entity); - let entity_ty = marker_ident(entity); - let event_ty = event_ident(entity); - let observer = if active { - quote! { - context - .observer::<`#event_ty`>(::core::clone::Clone::clone(options)) - .await? - } - } else { - quote! { - ::quent_instrumentation::ObserverInner::<`#event_ty`>::noop() - } - }; - quote! { - `#field`: ::quent_instrumentation::Observer::<`#entity_ty`>::new( - ::std::sync::Arc::new(`#observer`), - ) - } - }); - quote! { - `#storage` { - #(`#entity_fields`,)* - } - } + let fields: Vec<_> = schema.entities().map(entity_observer_field).collect(); + let entity_tys: Vec<_> = schema.entities().map(marker_ident).collect(); + let event_tys: Vec<_> = schema.entities().map(event_ident).collect(); + if active { + quote! { + { + let (#(`#fields`,)*) = ::quent_instrumentation::tokio::try_join!( + #(context.observer::<`#event_tys`>(::core::clone::Clone::clone(options)),)* + )?; + `#storage` { + #(`#fields`: ::quent_instrumentation::Observer::<`#entity_tys`>::new( + ::std::sync::Arc::new(`#fields`), + ),)* + } + } + } + } else { + quote! { + `#storage` { + #(`#fields`: ::quent_instrumentation::Observer::<`#entity_tys`>::new( + ::std::sync::Arc::new(::quent_instrumentation::ObserverInner::<`#event_tys`>::noop()), + ),)* + } + } + } }This requires
tokio::try_join!(or equivalent) to be accessible through thequent-instrumentationcrate, mirroring howcxx_bridge.rs/pyo3_bridge.rsreference#q::tokio::try_join!.🤖 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/context.rs` around lines 56 - 151, Update observer_storage_initializer and its active path in schema_model to construct all entity observers concurrently with the instrumentation crate’s tokio::try_join! (or equivalent), rather than awaiting each observer in the generated struct literal sequentially. Preserve the existing noop initialization for inactive observers and propagate any exporter-construction error while assigning each joined result to the corresponding entity field.
🤖 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/src/context.rs`:
- Around line 92-95: The no-op ContextInner::noop currently leaves block_on
without a runtime, causing caller-visible panics for pending futures. Update
noop and the public block_on path to provide a driveable no-op runtime or
propagate a defined Result error instead of reaching unreachable!, and revise
the associated test to verify the supported pending-future behavior.
---
Outside diff comments:
In `@crates/instrumentation/src/handle.rs`:
- Around line 29-113: Add direct #[test] coverage for the new core APIs: in
crates/instrumentation/src/handle.rs:29-113, test emit_once bit tracking,
OnceAlreadyEmitted errors, and all as_entity_ref*/as_any_entity_ref* builders;
in crates/instrumentation/src/entity.rs:37-56, verify handle_with_id preserves
the supplied UUID and handle produces distinct UUIDs; in
crates/instrumentation/src/model.rs:46-89, construct a minimal Context<M> and
exercise try_new, observer, and id.
---
Other comments:
In `@crates/instrumentation-build/src/lib.rs`:
- Around line 146-152: Add a Rustdoc comment directly above the public
GeneratedTypeCollision variant explaining that generation returns it when a
generated observer type name conflicts with a schema type name. Keep the
existing field documentation and visibility unchanged.
---
Nitpick comments:
In `@crates/instrumentation-build/src/runtime/context.rs`:
- Around line 17-54: The reserved-name collision check spanning
crates/instrumentation-build/src/runtime/context.rs:17-54 and
crates/instrumentation-build/src/runtime/mod.rs:51-89 is incomplete. Define a
shared reserved-identifier set covering observers_ident, model_ident,
entity_types’ Handle, and every bare name emitted by reexports (Context,
Observer, AnyEntity, EntityRef, Event, HandleError, DynamicAttributes, and
Uuid), then have observer_storage validate all entity and record paths against
it and report GeneratedTypeCollision consistently.
- Around line 56-151: Update observer_storage_initializer and its active path in
schema_model to construct all entity observers concurrently with the
instrumentation crate’s tokio::try_join! (or equivalent), rather than awaiting
each observer in the generated struct literal sequentially. Preserve the
existing noop initialization for inactive observers and propagate any
exporter-construction error while assigning each joined result to the
corresponding entity field.
In `@crates/instrumentation-build/src/runtime/mod.rs`:
- Around line 140-175: Add a negative test alongside
generates_generic_instrumentation_api that builds a schema containing an entity
or record named after the generated reserved types, including {Schema}Observers
and other names covered by GeneratedTypeCollision. Assert schema/runtime
generation rejects the collision with the expected GeneratedTypeCollision error,
while leaving the existing happy-path test unchanged.
🪄 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: 3f582f6e-8362-482c-b984-20a89e743701
📒 Files selected for processing (20)
crates/codegen/src/cxx_bridge.rscrates/codegen/src/pyo3_bridge.rscrates/instrumentation-build/example/src/main.rscrates/instrumentation-build/src/lib.rscrates/instrumentation-build/src/runtime/context.rscrates/instrumentation-build/src/runtime/handle.rscrates/instrumentation-build/src/runtime/mod.rscrates/instrumentation-build/src/runtime/observer.rscrates/instrumentation/benches/event_emit.rscrates/instrumentation/src/context.rscrates/instrumentation/src/entity.rscrates/instrumentation/src/handle.rscrates/instrumentation/src/lib.rscrates/instrumentation/src/model.rscrates/instrumentation/src/observer.rscrates/instrumentation/tests/collector_roundtrip.rscrates/instrumentation/tests/runtime_flavors.rscrates/model-macros/src/model_macro.rscrates/model/src/lib.rsintegrations/nvtx/example/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/instrumentation-build/src/runtime/observer.rs
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
42bb324 to
dde7df7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/instrumentation-build/src/runtime/mod.rs (1)
149-174: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winNo test covers the new
GeneratedTypeCollisionerror path.Add a case with an entity/record named
DemoObservers(or whatever the reserved set ends up being) assertinggenerate_runtime_typesreturnsGenerateError::GeneratedTypeCollision.As per coding guidelines, "New Rust components must include accompanying tests", and per path instructions, "Cover new public contracts and error/edge paths".
🤖 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 149 - 174, Extend generates_generic_instrumentation_api with a collision case using an entity or record named DemoObservers, then assert generate_runtime_types returns GenerateError::GeneratedTypeCollision. Use the actual reserved generated-type name set if it differs, and preserve the existing successful-generation assertions.Sources: Coding guidelines, Path instructions
🟡 Other comments (2)
crates/instrumentation-build/src/runtime/context.rs-85-97 (1)
85-97: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winEntity-less schemas still make
optionsunused. SinceSchemaBuilderallows empty schemas, theSome(options)arm will emit an unused binding when there are no entities, which becomes a hard error under-D warnings. Use_optionsin that case or avoid binding it altogether.🤖 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/context.rs` around lines 85 - 97, Update the exporter match in the generated observer initialization to avoid binding the Some payload as options when the schema has no entities, preventing an unused-variable error under -D warnings. Use an ignored binding or otherwise preserve the existing Some/None behavior without introducing an unused options symbol.Source: Coding guidelines
crates/instrumentation/tests/collector_roundtrip.rs-84-84 (1)
84-84: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPropagate setup errors in this test. Return
Resultfromcollector_roundtrip.rsand use?for bothContextInner::try_new(id)andctx.observer(...)instead ofunwrap().🤖 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/tests/collector_roundtrip.rs` at line 84, Update the test function in collector_roundtrip.rs to return a Result, propagate setup failures from ContextInner::try_new(id) and ctx.observer(...) with ?, and remove the corresponding unwrap calls while preserving the existing test behavior.Source: Path instructions
🧹 Nitpick comments (5)
crates/instrumentation-build/src/runtime/handle.rs (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGenerated
Handlename is duplicated across modules.
raw_ident("Handle")here must stay in sync with the literalHandlewritten inentity_types(crates/instrumentation-build/src/runtime/mod.rs Lines 56-78). Hoist a singlehandle_ident()helper inruntime/mod.rsand use it from both sites so the generated type name has one definition.Also applies to: 124-124
🤖 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/handle.rs` at line 29, Centralize the generated Handle identifier by adding a shared handle_ident() helper in runtime/mod.rs, then replace the local raw_ident("Handle") construction in the handle-generation code and the literal Handle usage in entity_types with that helper. Ensure both generation sites derive the type name from this single definition.crates/instrumentation-build/src/runtime/context.rs (1)
123-151: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winObservers are built serially; the macro path builds them concurrently.
Each entity's
context.observer::<E>(…).await?is awaited in sequence inside one async block, whereascrates/model-macros/src/model_macro.rs(Lines 486-493) usestokio::try_join!so every exporter is constructed concurrently. For schemas with many entities and non-trivial exporter setup (filesystem/network), context construction latency now scales linearly. Consider emitting atry_join!(orjoin_all) form here to preserve the existing startup characteristics.🤖 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/context.rs` around lines 123 - 151, Update observer_storage_initializer so active observers are initialized concurrently rather than awaiting each context.observer call serially. Emit a try_join! or equivalent concurrent future aggregation for all entity observers, then construct the storage fields from the results while preserving existing error propagation. Keep the inactive path using ObserverInner::noop() without unnecessary async work.crates/instrumentation/src/model.rs (2)
46-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the deliberate field order for Drop safety.
observersis declared beforeinnerso Rust dropsM::Observers(flushing eachObserverInner) beforeinner: ContextInner'sRuntimecan be torn down — this is what makesObserverInner::sender()-based'staticproducers and in-flight forwarders safe to flush even as the context goes away. This ordering is easy to break silently in a future edit (e.g. reordering fields or adding a field between them). A short comment would prevent that regression.📝 Suggested comment
pub struct Context<M: Model> { + // Drop order matters: `observers` must be dropped before `inner` so each + // `ObserverInner`'s forwarder flushes on the still-alive shared `Runtime`. observers: M::Observers, inner: ContextInner, }🤖 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/src/model.rs` around lines 46 - 49, Add a concise comment directly above the fields in Context explaining that observers must remain declared before inner so Rust drops M::Observers and flushes ObserverInner instances before ContextInner tears down its Runtime. Make the ordering requirement explicit, including that future fields must not be inserted between them.
19-43: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift
Model::build_observersalso returnsBox<dyn std::error::Error>.Same error-typing concern as
ContextInnerincontext.rs; see the consolidated comment at the end of this review.🤖 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/src/model.rs` around lines 19 - 43, Update the Model::build_observers return type to use the same concrete, thread-safe error type required by ContextInner instead of Box<dyn std::error::Error>, and adjust its documentation or callers as needed to preserve error propagation.Source: Path instructions
crates/instrumentation/src/context.rs (1)
75-152: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift
ContextInnerfallible APIs useBox<dyn std::error::Error>instead of typed/thiserror errors.
try_new,observer, and (inmodel.rs)Model::build_observers/Context::try_newall returnResult<_, Box<dyn std::error::Error>>. This is a shared root cause across both files; a consolidated note is added at the end of this review covering both sites.🤖 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/src/context.rs` around lines 75 - 152, Replace Box<dyn std::error::Error> in ContextInner::try_new and ContextInner::observer, plus Model::build_observers and Context::try_new, with the crate’s typed thiserror-based error type. Add or reuse variants that preserve runtime-resolution and exporter-construction failures, and propagate those errors through the existing call paths without changing successful behavior.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/instrumentation-build/src/runtime/context.rs`:
- Around line 17-30: Extract the existing collision logic from observer_storage
into a shared helper that checks every generated type name: the observers_ident
result, Handle, and the generated model marker name emitted by
generate_runtime_types. Compare each reserved name against Pascal-cased record
and entity paths, return GeneratedTypeCollision with the conflicting schema
path, and invoke this helper before generating the runtime types.
In `@crates/instrumentation-build/src/runtime/mod.rs`:
- Around line 70-78: The generated Handle implementation must stop implementing
Deref to HandleInner<E>, which currently exposes the untyped emit API and other
unintended methods. Remove that Deref implementation and explicitly forward only
the curated accessors required by Handle, such as id(), using justified
visibility and doc comments for any new public items; preserve the generated
typed event methods and their once-event ordering contract.
---
Outside diff comments:
In `@crates/instrumentation-build/src/runtime/mod.rs`:
- Around line 149-174: Extend generates_generic_instrumentation_api with a
collision case using an entity or record named DemoObservers, then assert
generate_runtime_types returns GenerateError::GeneratedTypeCollision. Use the
actual reserved generated-type name set if it differs, and preserve the existing
successful-generation assertions.
---
Other comments:
In `@crates/instrumentation-build/src/runtime/context.rs`:
- Around line 85-97: Update the exporter match in the generated observer
initialization to avoid binding the Some payload as options when the schema has
no entities, preventing an unused-variable error under -D warnings. Use an
ignored binding or otherwise preserve the existing Some/None behavior without
introducing an unused options symbol.
In `@crates/instrumentation/tests/collector_roundtrip.rs`:
- Line 84: Update the test function in collector_roundtrip.rs to return a
Result, propagate setup failures from ContextInner::try_new(id) and
ctx.observer(...) with ?, and remove the corresponding unwrap calls while
preserving the existing test behavior.
---
Nitpick comments:
In `@crates/instrumentation-build/src/runtime/context.rs`:
- Around line 123-151: Update observer_storage_initializer so active observers
are initialized concurrently rather than awaiting each context.observer call
serially. Emit a try_join! or equivalent concurrent future aggregation for all
entity observers, then construct the storage fields from the results while
preserving existing error propagation. Keep the inactive path using
ObserverInner::noop() without unnecessary async work.
In `@crates/instrumentation-build/src/runtime/handle.rs`:
- Line 29: Centralize the generated Handle identifier by adding a shared
handle_ident() helper in runtime/mod.rs, then replace the local
raw_ident("Handle") construction in the handle-generation code and the literal
Handle usage in entity_types with that helper. Ensure both generation sites
derive the type name from this single definition.
In `@crates/instrumentation/src/context.rs`:
- Around line 75-152: Replace Box<dyn std::error::Error> in
ContextInner::try_new and ContextInner::observer, plus Model::build_observers
and Context::try_new, with the crate’s typed thiserror-based error type. Add or
reuse variants that preserve runtime-resolution and exporter-construction
failures, and propagate those errors through the existing call paths without
changing successful behavior.
In `@crates/instrumentation/src/model.rs`:
- Around line 46-49: Add a concise comment directly above the fields in Context
explaining that observers must remain declared before inner so Rust drops
M::Observers and flushes ObserverInner instances before ContextInner tears down
its Runtime. Make the ordering requirement explicit, including that future
fields must not be inserted between them.
- Around line 19-43: Update the Model::build_observers return type to use the
same concrete, thread-safe error type required by ContextInner instead of
Box<dyn std::error::Error>, and adjust its documentation or callers as needed to
preserve error propagation.
🪄 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: 7bcc3372-4d1a-47cf-b14c-ab292babee8a
📒 Files selected for processing (20)
crates/codegen/src/cxx_bridge.rscrates/codegen/src/pyo3_bridge.rscrates/instrumentation-build/example/src/main.rscrates/instrumentation-build/src/lib.rscrates/instrumentation-build/src/runtime/context.rscrates/instrumentation-build/src/runtime/handle.rscrates/instrumentation-build/src/runtime/mod.rscrates/instrumentation-build/src/runtime/observer.rscrates/instrumentation/benches/event_emit.rscrates/instrumentation/src/context.rscrates/instrumentation/src/entity.rscrates/instrumentation/src/handle.rscrates/instrumentation/src/lib.rscrates/instrumentation/src/model.rscrates/instrumentation/src/observer.rscrates/instrumentation/tests/collector_roundtrip.rscrates/instrumentation/tests/runtime_flavors.rscrates/model-macros/src/model_macro.rscrates/model/src/lib.rsintegrations/nvtx/example/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/instrumentation-build/src/runtime/observer.rs
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
dhruv9vats
left a comment
dhruv9vats
left a comment
There was a problem hiding this comment.
Thanks @johanpel, this is a nice generalization and feels more natural this way.
joosthooz
left a comment
joosthooz
left a comment
There was a problem hiding this comment.
If you don't see the need for the coderabbit comments to be addressed, this looks good. One small comment is that there's now a quent_model::Observer and a quent_instrumentation::Observer which have different types, but one of them (?) is also referenced as #q::Observer so that could be confusing.
|
|
/merge |
# Description Stacked on #466. Adds qualified schema type path support to `quent-instrumentation-build`. Schema namespaces become Rust modules instead of being flattened: ```rust Foo::BarBaz → foo::BarBaz FooBar::Baz → foo_bar::Baz ``` This preserves path boundaries and allows equal leaf names such as Foo::Query and Bar::Query to coexist. Records, entities, events, references, handles, and observer storage follow the generated module hierarchy. Namespace-local AnyEvent enums compose into their parent namespace, while namespaces without events generate no aggregate. ## Related Issues Follow-up to #449. Closes #442. _Written by Codex._ Authors: - Johan Peltenburg (https://github.com/johanpel) Approvers: - Dhruv Vats (https://github.com/dhruv9vats) URL: #468
3 participants
Description
Simplify the generated instrumentation API by replacing per-entity observer and handle types with generic types parameterized by schema markers.
Previously, a model generated types such as:
It now exposes a consistent API:
This avoids collisions between generated helper names and valid schema types such as
QueryObserverorQueryHandle. Shared behavior now lives inquent-instrumentationbehindContextInner,ObserverInner, andHandleInner; generated code retains the model and entity markers, typed observer storage, generic handle wrapper, and entity-specific event methods.Observer lookup uses generated typed fields and
ObserverAccess<E>, retaining exactly one observer transport per entity and context without runtime downcasts or tuple indices.This PR intentionally continues rejecting qualified schema paths. Support for namespace modules and qualified types follows in the stacked
instrumentation-paths-2PR.Related Issues
Part of #442.
Written by Codex.