feat: add cross-boundary Relay propagation - #7
Closed
willkill07 wants to merge 9 commits into
Closed
Conversation
#### Overview Prefer response-derived LLM model names over requested model names when Relay exports observability data. This keeps ATIF, OpenTelemetry, and OpenInference aligned when a router, alias, or versioned endpoint returns a different model than the request asked for. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details This change fixes model-name precedence for LLM observability exports. - Updates `model_name_for_llm_event()` to prefer, in order, the normalized response model, manual response model, requested or profile model, normalized request model, and manual request fallback. - Updates the OpenTelemetry and OpenInference exporters to preserve one final end-time model-name attribute, preferring response-derived model data and keeping the start or request model only as fallback. - Updates ATIF pair resolution to prefer manual response models when normalized response data is unavailable. - Adds regression coverage for: - normalized response model overriding requested model - requested model remaining the fallback when no response model exists - manual response model overriding requested model across ATIF, OpenTelemetry, and OpenInference Validation run: - `cargo fmt --all` - `cargo clippy --workspace --all-targets -- -D warnings` - `cargo test -p nemo-relay helper_functions_cover_additional_openinference_branches --lib` - `cargo test -p nemo-relay helper_functions_cover_additional_otel_branches --lib` - `cargo test -p nemo-relay test_exporters_agree_on_model_name --lib` - `cargo test -p nemo-relay test_exporters_prefer_response_model_name_over_requested_model --lib` - `cargo test -p nemo-relay test_exporters_prefer_manual_response_model_name_over_requested_model --lib` - `just test-rust` - `just test-python` - `just test-go` - `just test-node` - `PATH="$HOME/.local/nemo-relay-tools/bin:$PATH" uv run pre-commit run --files crates/core/src/observability/atif.rs crates/core/src/observability/mod.rs crates/core/src/observability/openinference.rs crates/core/src/observability/otel.rs crates/core/tests/unit/observability/exporter_parity_tests.rs crates/core/tests/unit/observability/openinference_tests.rs crates/core/tests/unit/observability/otel_tests.rs` #### Where should the reviewer start? Start in `crates/core/src/observability/mod.rs` at `model_name_for_llm_event()`, then review how that precedence is applied in: - `crates/core/src/observability/atif.rs` - `crates/core/src/observability/otel.rs` - `crates/core/src/observability/openinference.rs` The most useful regression coverage is in: - `crates/core/tests/unit/observability/exporter_parity_tests.rs` #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to: none Authors: - Maryam Najafian (https://github.com/mnajafian-nv) Approvers: - Will Killian (https://github.com/willkill07) URL: NVIDIA#543
#### Overview Prepare the repository for the upcoming 0.7 release. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Stop nightly alpha tag creation for `release/0.6`. - Replace 0.6-specific release-note highlights and fixed issues with 0.7 placeholders. - Carry forward known issues and update the migration reference to 0.6 to 0.7. #### Where should the reviewer start? Review `.github/nightly-alpha-branches.yaml` for the nightly-tag change, then `docs/about-nemo-relay/release-notes/index.mdx` for the 0.7 release-note structure. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to: none ## Summary by CodeRabbit * **Documentation** * Updated NVIDIA NeMo Relay release notes to document version **0.7** (including placeholders for **Highlights** and **Fixed Known Issues**). * Updated migration guidance to reference upgrading **0.6 → 0.7**, replacing detailed **0.6** content with a **0.7** placeholder. * **Chores** * Adjusted nightly alpha branch configuration to remove the **release/0.6** branch, leaving **main** only. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Eric Evans II (https://github.com/ericevans-nv) URL: NVIDIA#544
#### Overview Align OpenInference export behavior with the expected attribute contract by stopping Relay from writing prompt text into `llm.system`. System content is now exported as ordered `llm.input_messages` entries instead. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Remove `llm.system` emission from both replay-payload projection and annotated request projection in the OpenInference exporter. - Preserve replay `systemPrompt` content by exporting it as the leading `llm.input_messages.<index>` entry with role `system`. - Preserve `AnnotatedLlmRequest.instructions` as the leading system input message when present. - Preserve annotated `Message::System` content in `llm.input_messages` instead of the legacy `llm.system` field. - Harden replay input projection so `prompt` still falls back when replay `messages` is empty or every replay message is skipped, and skip incomplete replay message objects unless they provide both a string `role` and displayable `content`. - Update unit coverage to assert that `llm.system` is absent, system and user messages are exported in the expected order, replay fallback remains contiguous, and migrated system-message content is preserved. Validation: - `cargo fmt --all` - `just test-rust` - `cargo clippy --workspace --all-targets -- -D warnings` - `just test-python` - `just test-go` - `just test-node` - `uv run pre-commit run --files crates/core/src/observability/openinference.rs crates/core/tests/unit/observability/openinference_tests.rs` #### Where should the reviewer start? Start in `crates/core/src/observability/openinference.rs`, especially the request projection path around `push_llm_request_attributes()` and `push_replay_input_messages()`. The most relevant regression coverage is in `crates/core/tests/unit/observability/openinference_tests.rs`. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to: none ## Summary by CodeRabbit * **Bug Fixes** * Improved OpenInference tracing by representing the system prompt as the first `llm.input_messages` entry with `role: system`, instead of emitting a separate `llm.system` scalar. * Ensured consistent `llm.input_messages` indexing/ordering for both annotated requests (from instructions) and replay inputs, with correct shifting of subsequent messages. * Added more robust replay handling: only well-formed message entries (with required role/content) are projected, and fallback behavior works for empty or incomplete replay inputs. * **Tests** * Updated and expanded OpenInference flattened-attribute tests to match the new system-message semantics and indexing. Authors: - Maryam Najafian (https://github.com/mnajafian-nv) Approvers: - Will Killian (https://github.com/willkill07) URL: NVIDIA#542
#### Overview
Allow ATIF filename templates to derive path-safe route fragments from top-level scope metadata, with concise literal fallbacks when metadata is optional.
- [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license.
- [x] I searched existing issues and open pull requests, and this does not duplicate existing work.
#### Details
- Extend the existing `filename_template` setting with nested `{metadata.<path>}` placeholders and optional `{metadata.<path>:-fallback}` values.
- Render one filename for local, S3, and HTTP storage so every existing destination uses the same metadata-aware routing behavior.
- Accept only path-safe relative metadata fragments and skip only the affected trajectory when a required value is missing, non-string, or unsafe.
- Preserve `{session_id}` as the required per-trajectory identity and log structured `atif_destination_render_failed` warnings for render failures.
- Add focused routing, fallback, path-safety, and recovery coverage, plus user documentation and observability skill guidance.
Validation:
- `cargo fmt --all`
- `just test-rust`
- `cargo clippy --workspace --all-targets -- -D warnings`
- `just test-python` (535 passed)
- `just test-go`
- `just test-node` (277 passed)
- `just docs`
- `uv run pre-commit run --all-files`
- Built `nemo-relay` for `x86_64-unknown-linux-musl`, confirmed it is a static x86-64 ELF, and executed it in a Linux/amd64 Alpine container.
This change is additive and has no breaking configuration changes.
#### Where should the reviewer start?
Start with `render_atif_filename` in `crates/core/src/observability/plugin_component.rs`, then review `atif_filename_template_routes_by_metadata_and_skips_invalid_paths` for the end-to-end behavior and path-safety decision.
#### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
- Relates to: none
## Summary by CodeRabbit
- **New Features**
- Enhanced ATIF `filename_template` to support `{metadata.<path>:-fallback}` placeholders for routing trajectories into nested, metadata-derived directories.
- **Bug Fixes**
- Stricter template validation and runtime rendering: malformed templates or unsafe/missing metadata now skip only the affected trajectory and log `atif_destination_render_failed`.
- **Documentation**
- Updated ATIF docs and references with the metadata placeholder syntax, `:-` fallback rules, path-safety requirements, and behavior when rendering fails.
- **Tests**
- Added/updated unit tests for unclosed placeholder rejection, safe-vs-unsafe metadata interpolation, traversal-like value handling, and updated destination preparation behavior.
Authors:
- Hans Arnholm (https://github.com/cypres)
Approvers:
- Will Killian (https://github.com/willkill07)
URL: NVIDIA#505
#### Overview Fixes an ATIF exporter bug where a continued LLM request after tool work could emit a second duplicate `user` step for the same logical turn. The exporter now keeps one `user` step per fresh user turn, preserves same-turn continuation requests on the matching `agent` step, and avoids carrying an unpaired continuation request onto a later unrelated step. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Update the ATIF LLM-start mapping so only fresh user turns emit a `user` step. - Treat same-turn continuations as part of the matching `agent` step instead of repeating the user message. - Preserve the full continuation request in `Step.extra.llm_request` so request fidelity is not lost. - Add turn-state detection for raw chat requests, OpenAI Responses-style inputs, and annotated requests. - Make raw message turn-state detection content-aware so user messages that carry only tool-use or tool-result continuation content stay in the same logical turn. - Keep OpenAI Responses typed non-user continuation items in the same logical turn instead of re-emitting a duplicate `user` step. - Only treat LLM ends with event data as pairable, which prevents a stashed continuation request from leaking onto a later unrelated `agent` step. - Add regression coverage for OpenAI Responses continuations with `function_call_output` and `shell_call`, Anthropic tool-result continuation, and an empty-end continuation followed by a fresh user turn. - Update the existing full agent-loop test to reflect the corrected ATIF step sequence. **Validation:** - `cargo fmt --all` - `cargo test -p nemo-relay observability::atif::tests::` - `cargo test -p nemo-relay-cli --lib -- --nocapture` - `uv run pre-commit run --files crates/core/src/observability/atif.rs crates/core/tests/unit/atif_tests.rs` #### Where should the reviewer start? Start in `crates/core/src/observability/atif.rs`, especially the turn-state detection around `llm_start_user_step_message()`, `chat_messages_turn_state()`, and the pairability lookup in `EventLookupMaps::from_events_with_correlation_events()`. The most important regression coverage is in `crates/core/tests/unit/atif_tests.rs`. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to: none Authors: - Maryam Najafian (https://github.com/mnajafian-nv) Approvers: - Eric Evans II (https://github.com/ericevans-nv) - Will Killian (https://github.com/willkill07) URL: NVIDIA#545
…VIDIA#547) #### Overview Keep ATIF model attribution aligned with OpenTelemetry and OpenInference when an LLM end event does not provide response-side model attribution. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Update the ATIF agent-step end path to preserve the paired start or request model when the end event has no response-side model attribution. - Keep existing response-first behavior unchanged when a normalized or manual response model is present. - Extend the exporter parity regression to assert that ATIF now matches the existing OpenTelemetry and OpenInference fallback behavior for this case. Validation: - `cargo fmt --all` - `cargo clippy --workspace --all-targets -- -D warnings` - `cargo test -p nemo-relay test_exporters_fall_back_to_requested_model_when_response_model_is_missing --lib` - `cargo test -p nemo-relay test_exporters_prefer_response_model_name_over_requested_model --lib` - `cargo test -p nemo-relay test_exporters_prefer_manual_response_model_name_over_requested_model --lib` - `just test-rust` - `just test-python` - `just test-go` - `just test-node` - `PATH="$HOME/.local/nemo-relay-tools/bin:$PATH" uv run pre-commit run --files crates/core/src/observability/atif.rs crates/core/tests/unit/observability/exporter_parity_tests.rs` #### Where should the reviewer start? Start in `crates/core/src/observability/atif.rs` in `handle_llm_end()`, where the emitted ATIF agent step now falls back to the paired start model only when the end event cannot provide response-side model attribution. The key regression coverage is in `crates/core/tests/unit/observability/exporter_parity_tests.rs`, especially `test_exporters_fall_back_to_requested_model_when_response_model_is_missing`. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to: RELAY-564 ## Summary by CodeRabbit * **Bug Fixes** * Improved observability exports so agent steps retain the requested model name when response metadata is unavailable. * Ensured model names remain consistent across ATIF, OpenTelemetry, and OpenInference exports. * **Tests** * Added coverage verifying the requested model name is preserved in ATIF fallback scenarios. Authors: - Maryam Najafian (https://github.com/mnajafian-nv) Approvers: - Will Killian (https://github.com/willkill07) URL: NVIDIA#547
#### Overview Add optional size-based rotation and retention to Relay operational file-log sinks. File sinks remain append-only unless rotation is explicitly configured. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Add validated `max_file_size_bytes` and `retained_files` settings for file sinks. - Require both rotation settings together and reject zero or unsafe retention values. - Rotate complete log records before the next write would exceed the configured size. - Retain the configured number of managed backups while preserving the active file. - Preserve historical backups outside the current retention window when the configured limit is reduced, avoiding automatic deletion of existing data. - Keep file writes behind the existing asynchronous sink and report rotation failures through its error path. - Detect conflicts between active sink paths and generated backup paths. - Preserve existing append-only behavior when rotation is omitted. - Document the configuration and add focused core and CLI coverage. #### Where should the reviewer start? Start with the rotating writer in `crates/core/src/logging/rotation.rs` and its integration with the asynchronous file sink in `crates/core/src/logging/sink.rs`. Focused behavior coverage is in `crates/core/tests/coverage/logging_tests.rs`. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Closes NVIDIA#553 ## Summary by CodeRabbit - **New Features** - Added size-based rotation for operational file log sinks using `max_file_size_bytes` and `retained_files` (with a `retained_files` max of 9). - When enabled, log writing rotates by size while keeping records intact and appending to the active file. - **Bug Fixes** - Enforced that rotation settings must be configured together; invalid values now fail configuration. - Prevented collisions between active log paths and any potential rotated/retained file targets. - **Documentation** - Updated the operational logging TOML examples and guidance for rotation configuration and limits. - **Tests** - Expanded parsing and integration coverage for rotation, retention ordering, boundary rotation, large-record behavior, and collision scenarios. Authors: - Eric Evans II (https://github.com/ericevans-nv) Approvers: - Will Killian (https://github.com/willkill07) - Maryam Najafian (https://github.com/mnajafian-nv) URL: NVIDIA#555
#### Overview Exposes the active per-call LLM codec to observability sanitizers across in-process and plugin boundaries, then uses that resolved capability to make PII redaction provider-correct and fail-closed for mixed managed LLM traffic. > [!WARNING] > **BREAKING CHANGE:** LLM request and response sanitizers now use the required two-argument contract `(payload, directional_context) -> optional payload`. No registration-time compatibility shim remains. Rust, Python, Node.js, native plugins, raw C FFI consumers, worker plugins, and downstream bindings must update their callbacks. All Rust worker sanitizer callbacks—mark, scope, tool, and LLM—are now async. Native dynamic plugins remain ABI v2 and workers remain `grpc-v1`, but the v2 context/host tables and grpc-v1 invocation/service definitions change in place. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Replaces split legacy/contextual LLM sanitizer registration with one canonical request and response callback representation while preserving priority, array-order tie breaking, scope ownership, registration rollback, snapshots, and short-circuit omission. - Introduces distinct `LlmSanitizeRequestContext` and `LlmSanitizeResponseContext` values with structured codec identities: none, built-in, runtime, and opaque. Every active codec, including opaque codecs, can be resolved for the lifetime of the callback. - Gives request sanitizers `decode(request)` and `encode(annotated, original)`; gives response sanitizers `decode(response)`. - Extends native ABI v2 and the raw C FFI with borrowed directional codec handles and host-owned transformation operations. Safe wrappers reject retained handles after the callback returns, contain codec panics, preserve ownership for aliased results, and fail closed for malformed output or unrepresentable runtime codec IDs. - Extends `grpc-v1` with directional contexts and authenticated, invocation-scoped codec capabilities. Host RPCs reject forged, expired, unauthorized, and wrong-direction capability IDs, and SDK authors see async codec proxies rather than raw IDs. - Makes Rust worker sanitizer registration async-only while Python workers accept immediate values or awaitables under the same required two-argument callback contract. - Keeps sanitizer failures fail-closed for observability: failures or `None`/`null` results omit the payload and annotation without changing the client-visible request or response. - Invalidates caller-supplied normalized annotations whenever sanitization changes a buffered payload, rebuilding only through the active directional codec. Decode failure or no codec omits the stale annotation. - Updates PII redaction to use the resolved active codec per call, use configured `codec` only when no active codec exists, support provider-agnostic policies without a configured codec, and omit codec-dependent observability data when normalization is unavailable. - Hardens normalized target overlays: object-field removals are applied incrementally, unsafe array removals and inconsistent projections fail closed, and OpenAI Responses keeps the top-level `output_text` alias synchronized with sanitized structured output. - Makes Node custom-codec reference cleanup event-loop-affine so early-dropped streams cannot delete N-API references from a Tokio thread. - Documents parameter order, directional contexts, codec identity and resolution, capability lifetime, omission behavior, native/worker contracts, and mixed-provider PII usage with Fern tabs. Adds the 0.7 breaking-change release note and a complete 0.6-to-0.7 migration procedure. Validation: - Repository-wide pre-commit suite passed before the final review fixes; the final changed-file pre-commit suite also passed, including FFI header sync, Cargo fmt/Clippy/check, Node formatting, and documentation links. - Node: 290 tests passed. - Python: 556 tests passed. - Go: all package tests passed. - Raw FFI: 82 unit tests and 77 integration tests passed. - Targeted buffered and streaming OpenAI Responses PII regressions passed and verified serialized events contain no original secret. - The public Rust native-plugin example passes its standalone `cargo check`. - The affected Rust core, PII, native-plugin, worker protocol/SDK, and binding capability suites passed. One unrelated CLI test in the workspace was contaminated by an ancestor user plugin configuration; the exact test passed from an isolated checkout of this commit. - Documentation build completed with 0 errors. #### Where should the reviewer start? Start with `crates/core/src/api/runtime/callbacks.rs` and `crates/core/src/api/llm.rs` for the canonical callback contract, codec propagation, and annotation invalidation. Then review `crates/plugin/src/lib.rs`, `crates/core/src/plugin/dynamic/native.rs`, `crates/core/src/plugin/dynamic/worker.rs`, and `crates/worker-proto/proto/nemo/relay/worker/v1/plugin_worker.proto` for cross-boundary capability lifetime and authorization. Finish with `crates/pii-redaction/src/builtin.rs` and `crates/pii-redaction/src/overlay.rs` for per-call codec selection and fail-closed redaction, then review `docs/reference/migration-guides.mdx` for the public upgrade contract. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Closes NVIDIA#526 - Closes RELAY-555 ## Summary by CodeRabbit - **New Features** - Codec-aware LLM sanitizers: callbacks now receive per-call request/response codec context for directional normalization/redaction. - Sanitizers can omit observability payloads/annotations by returning no value (`null`/`None`). - Codec-aware support expanded across Node.js, Python, Go, Rust, native plugins, and worker integrations. - PII redaction now selects the active codec per call for mixed-provider traffic. - **Breaking Changes** - Callback signatures changed to `(payload, context)` and must return an optional payload (`null`/`None` to omit). - Native plugins now require Native ABI v2 (rebuilt with the matching SDK/protocol). - **Documentation** - Added migration guidance and updated middleware/plugin docs for codec-aware sanitizer behavior. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Alex Fournier (https://github.com/afourniernv) URL: NVIDIA#546
Signed-off-by: Will Killian <wkillian@nvidia.com>
|
Important Review skippedToo many files! This PR contains 125 files, which is 25 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (125)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
Closing this fork PR; the upstream PR is NVIDIA#562. |
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
4 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.
Overview
Implement RELAY-485 transport-neutral Relay propagation across Rust, the C FFI, Python, Node.js, Go, and observability exporters.
Details
Where should the reviewer start?
Start with
crates/core/src/api/runtime/scope_stack.rsfor the context and imported-stack semantics, then review the exporter parent projection incrates/core/src/observability/otel.rs.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)