feat(observability): derive OTLP IDs from Relay UUIDs#453
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (12)**/*.rs📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Files:
{crates/core,crates/adaptive}/**/*📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Files:
crates/core/**/*.rs📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
Files:
**/*.{rs,py}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,js,mjs,cjs,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,go,js,ts,c,h}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,go,js,ts}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
crates/{core,adaptive}/**/*📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{rs,py,go,js,ts}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
crates/{core,adaptive}/**/*.rs⚙️ CodeRabbit configuration file
Files:
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
WalkthroughRelay UUIDs are converted into OpenTelemetry ChangesDeterministic OpenTelemetry IDs
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RelayEvent
participant OtelEventProcessor
participant OpenInferenceEventProcessor
participant SpanBuilder
RelayEvent->>OtelEventProcessor: process_start or process_mark with UUID
RelayEvent->>OpenInferenceEventProcessor: Start or Mark with UUID
OtelEventProcessor->>SpanBuilder: set UUID-derived trace_id and span_id
OpenInferenceEventProcessor->>SpanBuilder: set UUID-derived trace_id and span_id
SpanBuilder-->>OtelEventProcessor: emit span
SpanBuilder-->>OpenInferenceEventProcessor: emit span
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
9b45eee to
66eeb7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/core/src/observability/mod.rs`:
- Around line 576-589: Remove the inline tests module from observability/mod.rs
and relocate relay_id_conversions_preserve_zero_bytes into
crates/core/tests/unit/observability. Wire the relocated test into the crate
using the existing #[path] inclusion pattern used by openinference_tests.rs,
otel_tests.rs, and related observability tests, preserving access to
relay_trace_id and relay_span_id.
In `@crates/core/src/observability/openinference.rs`:
- Around line 745-746: Update the trace and span ID assignment in the
event-processing flow around relay_trace_id and relay_span_id to preserve the
same precedence behavior required by process_start: ensure relay-derived IDs do
not override the event’s existing IDs when those are already available, while
retaining relay IDs as the fallback.
- Around line 709-710: Update the trace and span ID assignment in the
event-building flow around relay_trace_id and relay_span_id to preserve the same
precedence behavior required by process_start: use the event’s existing IDs when
present and only fall back to relay-derived IDs when absent. Keep the current
builder flow unchanged apart from correcting this precedence.
- Around line 638-639: Update the span-building chain around with_trace_id and
with_span_id to apply relay_trace_id(event.uuid()) only when is_trace_root is
true. Leave child spans without an explicit trace_id so they inherit the parent
trace ID, while preserving the existing root and span ID behavior.
In `@crates/core/src/observability/otel.rs`:
- Around line 703-704: Update the trace and span ID assignment in the
event-processing flow to preserve the intended precedence, matching the
correction applied to otel.rs process_start and openinference.rs. Ensure
relay_trace_id and relay_span_id do not overwrite higher-priority existing
identifiers.
- Around line 731-732: Update the trace and span ID assignment in the
surrounding observability event builder to preserve the intended precedence,
matching the correction applied to openinference.rs and the otel.rs
process_start path. Adjust the .with_trace_id and .with_span_id calls so
relay-derived IDs cannot incorrectly override the intended existing IDs.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 0cc7e4bf-267a-410c-9ed9-4bec9071a222
⛔ Files ignored due to path filters (1)
crates/core/tests/fixtures/native_plugin/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/core/src/observability/mod.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (14)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/otel.rs
{crates/core,crates/adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/otel.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/otel.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/otel.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/otel.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/otel.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/otel.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/otel.rs
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/otel.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/otel.rs
crates/{core,adaptive}/**/*.rs
⚙️ CodeRabbit configuration file
crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/otel.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/src/observability/otel.rs
crates/core/src/observability/{atif,otel,openinference}.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
When changing the core event model, emitted fields, exporter behavior, subscriber config, or lifecycle, keep the ATIF, OpenTelemetry, and OpenInference implementations in sync.
Files:
crates/core/src/observability/openinference.rscrates/core/src/observability/otel.rs
🔇 Additional comments (6)
crates/core/src/observability/mod.rs (1)
75-86: LGTM!crates/core/src/observability/openinference.rs (1)
28-29: LGTM!crates/core/src/observability/otel.rs (2)
25-29: LGTM!
631-632: 🗄️ Data Integrity & IntegrationKeep ATIF aligned with this identity change. If
atif.rshas an equivalent event identifier, wireevent.uuid()through it too; otherwise make the ATIF exemption explicit.crates/core/tests/unit/observability/openinference_tests.rs (1)
2364-2374: LGTM!Solid deterministic coverage of root, child, and orphan derivation with byte-level assertions and parent-linkage checks. As per path instructions, this "prefer[s] assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests."
Also applies to: 2404-2416, 2425-2455
Source: Path instructions
crates/core/tests/unit/observability/otel_tests.rs (1)
1052-1062: LGTM!Also applies to: 1092-1104, 1113-1143
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/src/observability/mod.rs`:
- Around line 75-85: Ensure nil UUIDs are rejected or replaced before they reach
relay_trace_id and relay_span_id, preventing zero-valued OpenTelemetry IDs from
being produced. Add the guard at the event boundary when event UUIDs are
handled, or map nil values to an established non-zero fallback while preserving
normal UUID conversion.
In `@crates/core/tests/unit/observability/openinference_tests.rs`:
- Around line 2366-2370: The orphan-mark tests currently use
MarkProjection::Tool and bypass the normal process_mark path. In
crates/core/tests/unit/observability/openinference_tests.rs at lines 2366-2370,
add a default-projection orphan-mark case using MarkProjection::default() and
assert the Relay-derived trace/span bytes; add the equivalent assertion in
crates/core/tests/unit/observability/otel_tests.rs at lines 1054-1058.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 0ccb2a9d-b404-4097-8de8-ceeb8b522b7d
📒 Files selected for processing (5)
crates/core/src/observability/mod.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (14)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rs
{crates/core,crates/adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation
Files:
crates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/core/src/observability/mod.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rs
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rs
crates/{core,adaptive}/**/*.rs
⚙️ CodeRabbit configuration file
crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/core/src/observability/mod.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/observability/openinference_tests.rs
crates/core/src/observability/{atif,otel,openinference}.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
When changing the core event model, emitted fields, exporter behavior, subscriber config, or lifecycle, keep the ATIF, OpenTelemetry, and OpenInference implementations in sync.
Files:
crates/core/src/observability/openinference.rscrates/core/src/observability/otel.rs
🔇 Additional comments (2)
crates/core/src/observability/mod.rs (2)
576-589: Duplicate: relocate this inline unit test.As per coding guidelines, “Do not add tests under
src; Rust tests belong in cratetests/trees.”Source: Coding guidelines
75-85: 📐 Maintainability & Code QualityRun the required
crates/corevalidation matrix.Please provide successful output for
cargo fmt --all, strict Clippy,just test-rust,cargo deny check,validate-change,uv run pre-commit run --all-files, and the Rust/Python/Go/Node matrix. As per coding guidelines, “Changes tocrates/coreorcrates/adaptivemust run the full language matrix.”Source: Coding guidelines
Signed-off-by: Will Killian <wkillian@nvidia.com>
66eeb7e to
ddd1abe
Compare
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/core/src/observability/mod.rs`:
- Around line 81-84: Update relay_span_id to guard against UUIDs whose low 64
bits are zero, preventing creation of SpanId::INVALID; make the conversion
reject or return a fallible result and update callers such as with_span_id
accordingly. Also ensure nil UUIDs cannot produce invalid OpenTelemetry context
IDs.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: b6e15c18-6c81-493d-bd23-5f08d52eac53
📒 Files selected for processing (5)
crates/core/src/observability/mod.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (14)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rs
{crates/core,crates/adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/openinference.rs
crates/core/src/observability/{atif,otel,openinference}.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
When changing the core event model, emitted fields, exporter behavior, subscriber config, or lifecycle, keep the ATIF, OpenTelemetry, and OpenInference implementations in sync.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/openinference.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rs
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rs
crates/{core,adaptive}/**/*.rs
⚙️ CodeRabbit configuration file
crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rs
🔇 Additional comments (6)
crates/core/src/observability/mod.rs (2)
577-589: Move this test to the crate test tree.As per coding guidelines, “Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.”Source: Coding guidelines
75-85: 📐 Maintainability & Code QualityProvide the required core-runtime validation results.
Run
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust,cargo deny check,validate-change, and the Rust/Python/Go/Node matrix before handoff.As per coding guidelines, “If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation” and “Ifcrates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.”Source: Coding guidelines
crates/core/src/observability/openinference.rs (1)
28-29: LGTM!Also applies to: 638-639, 709-710, 745-746
crates/core/src/observability/otel.rs (1)
28-29: LGTM!Also applies to: 631-632, 703-704, 731-732
crates/core/tests/unit/observability/openinference_tests.rs (1)
2372-2455: LGTM!crates/core/tests/unit/observability/otel_tests.rs (1)
1060-1143: LGTM!
Signed-off-by: Will Killian <wkillian@nvidia.com>
mnajafian-nv
left a comment
There was a problem hiding this comment.
Great direction overall. I left two focused comments on the OTLP ID derivation edge case so we can make sure the new exporter contract stays valid for all caller-supplied Relay UUIDs.
mnajafian-nv
left a comment
There was a problem hiding this comment.
Thanks, conditional approval. The helper thread still has one suggestion ready for your review.
|
/merge |
Overview
Derive OpenTelemetry and OpenInference trace and span identifiers from Relay event UUIDs.
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.
Details
Where should the reviewer start?
Review
crates/core/src/observability/mod.rsfor the shared conversions, then the OTel and OpenInference event processors and their deterministic mapping tests.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Enhancements
Tests