feat(observability)!: add multi-sink ATOF export#416
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:XL
|
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 (7)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (25)**/*.rs📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Files:
crates/ffi/**📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
Files:
crates/ffi/**/*.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:
**/*.{rs,py,go,js,ts}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
crates/{python,ffi,node}/**/*⚙️ CodeRabbit configuration file
Files:
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}⚙️ CodeRabbit configuration file
Files:
**/*.py📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
{crates/**/src/**/*.rs,python/**/*.py}📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Files:
**/*.{py,go,js,ts}📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Files:
python/tests/**/*.py📄 CodeRabbit inference engine (.agents/skills/test-python-binding/SKILL.md)
Files:
go/nemo_relay/**/*.go📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
Files:
**/*.go📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Files:
{python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go}📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Files:
go/nemo_relay/**📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Files:
go/nemo_relay/**/*⚙️ CodeRabbit configuration file
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:
crates/{core,adaptive}/**/*📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
crates/{core,adaptive}/**/*.rs⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (7)
WalkthroughATOF configuration changes from endpoint-based settings to typed file and stream sinks. Core exporters, plugin validation and registration, CLI diagnostics, FFI behavior, and Node, Python, and Go bindings now use the sink model with observability configuration version 2. ChangesATOF sink migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Plugin
participant AtofExporter
participant FileSink
participant StreamSink
Client->>Plugin: provide version-2 sinks configuration
Plugin->>AtofExporter: build one exporter per sink
AtofExporter->>FileSink: write JSONL for file sinks
AtofExporter->>StreamSink: enqueue raw JSON for stream sinks
Client->>AtofExporter: request exporter path
AtofExporter-->>Client: file path or null for stream sink
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
go/nemo_relay/nemo_relay.go (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
MarshalJSONre-declares every field to inject the"type"discriminator — drift risk. All four sinkMarshalJSONmethods duplicate the exact field list and JSON tags of their outer struct in an unexported anonymous struct purely to avoid infinite recursion when adding"type". Adding a field to the sink struct later and forgetting to mirror it here will silently drop that field from the wire format.
go/nemo_relay/nemo_relay.go#L1710-1719: replace the manualfileSinkJSONstruct inAtofFileSinkConfig.MarshalJSONwith the type-alias trick (type alias AtofFileSinkConfig; json.Marshal(struct{ Type string \json:"type"`; alias }{"file", alias(config)})`).go/nemo_relay/nemo_relay.go#L1756-1767: apply the same alias-based fix toAtofStreamSinkConfig.MarshalJSON.go/nemo_relay/observability_plugin.go#L53-62: apply the same alias-based fix toObservabilityAtofFileSinkConfig.MarshalJSON.go/nemo_relay/observability_plugin.go#L76-88: apply the same alias-based fix toObservabilityAtofStreamSinkConfig.MarshalJSON.♻️ Example fix (applies analogously to all four sites)
func (config AtofFileSinkConfig) MarshalJSON() ([]byte, error) { - type fileSinkJSON struct { - Type string `json:"type"` - OutputDirectory string `json:"output_directory,omitempty"` - Mode AtofExporterMode `json:"mode,omitempty"` - Filename string `json:"filename,omitempty"` - } - return json.Marshal(fileSinkJSON{"file", config.OutputDirectory, config.Mode, config.Filename}) + type alias AtofFileSinkConfig + return json.Marshal(struct { + Type string `json:"type"` + alias + }{"file", alias(config)}) }🤖 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 `@go/nemo_relay/nemo_relay.go` at line 1, Replace the manually mirrored JSON helper structs in AtofFileSinkConfig.MarshalJSON, AtofStreamSinkConfig.MarshalJSON, ObservabilityAtofFileSinkConfig.MarshalJSON, and ObservabilityAtofStreamSinkConfig.MarshalJSON with the type-alias embedding pattern: define an alias of the receiver type, embed it alongside the type discriminator field, and marshal the combined value. Preserve each method’s existing discriminator value and JSON key.crates/core/src/observability/atof.rs (2)
625-639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "endpoints" terminology leaks into user-facing errors post-migration to sinks.
start_endpoint_workersis now only ever invoked with a single-element slice (&[stream_sink]fromAtofExporter::new, line ~384), so theenumerate()/index here is vestigial, and the emittedeprintln!/AtofExporterError::InvalidEndpointtext still says"endpoints[{index}]: ..."(always index 0) while the rest of this PR consistently renamed this concept tosinks[{index}](seeplugin_component.rs'sbuild_atof_sink_configanddoctor.rs's probe messages). This stale wording can surface directly in plugin registration error messages/logs, confusing users mid-migration.♻️ Suggested simplification
-fn start_endpoint_workers(configs: &[AtofStreamSinkConfig]) -> Result<Vec<AtofEndpointWorker>> { - let mut workers = Vec::with_capacity(configs.len()); - for (index, config) in configs.iter().enumerate() { - match start_endpoint_worker(index, config.clone()) { - Ok(worker) => workers.push(worker), - Err(error) => { - eprintln!("nemo_relay: invalid ATOF endpoint[{index}]: {error}"); - return Err(AtofExporterError::InvalidEndpoint(format!( - "endpoints[{index}]: {error}" - ))); - } - } - } - Ok(workers) -} +fn start_endpoint_workers(configs: &[AtofStreamSinkConfig]) -> Result<Vec<AtofEndpointWorker>> { + let mut workers = Vec::with_capacity(configs.len()); + for (index, config) in configs.iter().enumerate() { + match start_endpoint_worker(index, config.clone()) { + Ok(worker) => workers.push(worker), + Err(error) => { + eprintln!("nemo_relay: invalid ATOF stream sink: {error}"); + return Err(AtofExporterError::InvalidEndpoint(error.to_string())); + } + } + } + Ok(workers) +}🤖 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/core/src/observability/atof.rs` around lines 625 - 639, Update start_endpoint_workers to use the migrated “sinks” terminology in user-facing diagnostics, replacing the stale “endpoints” wording in both eprintln! and AtofExporterError::InvalidEndpoint messages. Remove the vestigial enumerate/index handling if the single-sink invocation contract permits, while preserving the existing worker startup and error propagation behavior.
302-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUndocumented silent no-op when sink is a stream.
with_output_directory/with_mode/with_filenamesilently do nothing onceself.sinkisStream(e.g. if called afterwith_stream_sink/with_endpoint, or on a config built for streaming).with_endpoint's doc comment already warns about override semantics, but these three setters don't mention the no-op behavior at all — a direct Rust API consumer building a config in the "wrong" order loses settings with no error/warning.📝 Suggested doc addition
/// Override the output directory. + /// + /// No-op when the configured sink is a stream sink. pub fn with_output_directory(mut self, output_directory: impl Into<PathBuf>) -> Self {(repeat for
with_mode/with_filename)🤖 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/core/src/observability/atof.rs` around lines 302 - 330, Document the silent no-op behavior in the doc comments for AtofExporterConfig::with_output_directory, with_mode, and with_filename: each setter only applies when self.sink is AtofSinkConfig::File and does nothing when the sink is Stream, including when configured via with_stream_sink or with_endpoint. Preserve the existing implementation and mention the ordering requirement for callers.crates/ffi/tests/integration/api_tests.rs (1)
811-818: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRename
invalid_endpointtoinvalid_sink. TheInvalidArgcheck is correct here becausewebsocketrequires aws://orwss://URL.🤖 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/ffi/tests/integration/api_tests.rs` around lines 811 - 818, Rename the local variable invalid_endpoint to invalid_sink in the test while preserving the existing JSON payload and InvalidArg assertion.
🤖 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/tests/unit/observability/plugin_component_tests.rs`:
- Around line 371-374: Update the schema completeness property list in the
relevant observability plugin component test to include "header_env" alongside
the other ATOF sink properties. Ensure the test verifies that
AtofStreamSinkSectionConfig exposes header_env in the generated JSON schema.
In `@crates/python/src/py_types/observability.rs`:
- Around line 256-331: Validate that self.url is non-empty at the start of the
"stream" arm in PyAtofExporterConfig::to_rust_config, returning a PyValueError
with the message "stream sink requires url" when missing. Preserve the existing
endpoint conversion for valid URLs and add a Python binding test covering stream
configuration without a URL.
In `@go/nemo_relay/nemo_relay.go`:
- Around line 1683-1719: Refactor AtofFileSinkConfig.MarshalJSON and the
similarly structured serializer near the referenced sink configuration to avoid
duplicating every field and JSON tag solely to add the "type" discriminator.
Reuse the existing config serialization and inject the discriminator through a
minimal wrapper or equivalent mechanism while preserving all current field
names, omission behavior, and output.
In `@python/nemo_relay/__init__.py`:
- Line 97: Expose AtofFileSinkConfig alongside AtofStreamSinkConfig in the
package-root exports and __all__ entries in python/nemo_relay/__init__.py at
lines 97-97 and 481-481, and add the same symbol to the stub exports in
python/nemo_relay/__init__.pyi at lines 60-62, preserving the existing
observability import path and type declarations.
In `@python/tests/test_types.py`:
- Around line 484-488: Remove the unused tmp_path fixture parameter from
test_endpoint_field_name_policy_is_validated, leaving the test setup and
stream-sink validation behavior unchanged.
---
Outside diff comments:
In `@crates/core/src/observability/atof.rs`:
- Around line 625-639: Update start_endpoint_workers to use the migrated “sinks”
terminology in user-facing diagnostics, replacing the stale “endpoints” wording
in both eprintln! and AtofExporterError::InvalidEndpoint messages. Remove the
vestigial enumerate/index handling if the single-sink invocation contract
permits, while preserving the existing worker startup and error propagation
behavior.
- Around line 302-330: Document the silent no-op behavior in the doc comments
for AtofExporterConfig::with_output_directory, with_mode, and with_filename:
each setter only applies when self.sink is AtofSinkConfig::File and does nothing
when the sink is Stream, including when configured via with_stream_sink or
with_endpoint. Preserve the existing implementation and mention the ordering
requirement for callers.
In `@crates/ffi/tests/integration/api_tests.rs`:
- Around line 811-818: Rename the local variable invalid_endpoint to
invalid_sink in the test while preserving the existing JSON payload and
InvalidArg assertion.
In `@go/nemo_relay/nemo_relay.go`:
- Line 1: Replace the manually mirrored JSON helper structs in
AtofFileSinkConfig.MarshalJSON, AtofStreamSinkConfig.MarshalJSON,
ObservabilityAtofFileSinkConfig.MarshalJSON, and
ObservabilityAtofStreamSinkConfig.MarshalJSON with the type-alias embedding
pattern: define an alias of the receiver type, embed it alongside the type
discriminator field, and marshal the combined value. Preserve each method’s
existing discriminator value and JSON key.
🪄 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: 166a4e39-4fac-4878-9bde-d46bf9a57c97
📒 Files selected for processing (33)
crates/cli/src/doctor.rscrates/cli/src/launcher.rscrates/cli/tests/coverage/doctor_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/observability/atof.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/ffi/nemo_relay.hcrates/ffi/src/api/observability.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/tests/integration/api_tests.rscrates/ffi/tests/unit/api/core_tests.rscrates/node/observability.d.tscrates/node/observability.jscrates/node/src/api/mod.rscrates/node/tests/atof_tests.mjscrates/node/tests/observability_plugin_tests.mjscrates/pii-redaction/tests/unit/component_tests.rscrates/python/src/py_types/observability.rsgo/nemo_relay/atof_test.gogo/nemo_relay/nemo_relay.gogo/nemo_relay/observability_plugin.gogo/nemo_relay/observability_plugin_test.gojustfilepython/nemo_relay/__init__.pypython/nemo_relay/__init__.pyipython/nemo_relay/_native.pyipython/nemo_relay/observability.pypython/nemo_relay/observability.pyipython/tests/test_observability_plugin.pypython/tests/test_types.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (35)
**/*.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/pii-redaction/tests/unit/component_tests.rscrates/ffi/tests/integration/api_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/src/api/observability.rscrates/cli/src/launcher.rscrates/ffi/tests/unit/api/core_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/coverage/session_tests.rscrates/node/src/api/mod.rscrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/doctor.rscrates/core/src/observability/atof.rscrates/python/src/py_types/observability.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/observability/plugin_component.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/pii-redaction/tests/unit/component_tests.rscrates/ffi/tests/integration/api_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/src/api/observability.rscrates/cli/src/launcher.rscrates/ffi/tests/unit/api/core_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/coverage/session_tests.rspython/nemo_relay/__init__.pypython/nemo_relay/observability.pypython/tests/test_types.pypython/tests/test_observability_plugin.pycrates/node/src/api/mod.rscrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/doctor.rscrates/core/src/observability/atof.rscrates/python/src/py_types/observability.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/observability/plugin_component.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/pii-redaction/tests/unit/component_tests.rscrates/ffi/tests/integration/api_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/src/api/observability.rscrates/cli/src/launcher.rscrates/ffi/tests/unit/api/core_tests.rscrates/node/tests/atof_tests.mjscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/coverage/session_tests.rscrates/node/observability.d.tspython/nemo_relay/__init__.pypython/nemo_relay/observability.pypython/tests/test_types.pycrates/node/observability.jspython/tests/test_observability_plugin.pycrates/node/tests/observability_plugin_tests.mjscrates/node/src/api/mod.rscrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/doctor.rscrates/core/src/observability/atof.rscrates/python/src/py_types/observability.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/observability/plugin_component.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/pii-redaction/tests/unit/component_tests.rscrates/ffi/tests/integration/api_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/nemo_relay.hcrates/ffi/src/api/observability.rscrates/cli/src/launcher.rscrates/ffi/tests/unit/api/core_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/coverage/session_tests.rscrates/node/observability.d.tspython/nemo_relay/__init__.pypython/nemo_relay/observability.pypython/tests/test_types.pycrates/node/observability.jsgo/nemo_relay/atof_test.gopython/tests/test_observability_plugin.pygo/nemo_relay/observability_plugin.gocrates/node/src/api/mod.rsgo/nemo_relay/observability_plugin_test.gocrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/doctor.rsgo/nemo_relay/nemo_relay.gocrates/core/src/observability/atof.rscrates/python/src/py_types/observability.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/observability/plugin_component.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/pii-redaction/tests/unit/component_tests.rscrates/ffi/tests/integration/api_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/src/api/observability.rscrates/cli/src/launcher.rscrates/ffi/tests/unit/api/core_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/coverage/session_tests.rscrates/node/observability.d.tscrates/node/observability.jsgo/nemo_relay/atof_test.gogo/nemo_relay/observability_plugin.gocrates/node/src/api/mod.rsgo/nemo_relay/observability_plugin_test.gocrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/doctor.rsgo/nemo_relay/nemo_relay.gocrates/core/src/observability/atof.rscrates/python/src/py_types/observability.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/observability/plugin_component.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/pii-redaction/tests/unit/component_tests.rscrates/ffi/tests/integration/api_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/nemo_relay.hcrates/ffi/src/api/observability.rscrates/cli/src/launcher.rscrates/ffi/tests/unit/api/core_tests.rscrates/node/tests/atof_tests.mjscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/coverage/session_tests.rscrates/node/observability.d.tspython/nemo_relay/__init__.pypython/nemo_relay/observability.pypython/nemo_relay/__init__.pyipython/tests/test_types.pycrates/node/observability.jsgo/nemo_relay/atof_test.gopython/nemo_relay/observability.pyijustfilepython/tests/test_observability_plugin.pygo/nemo_relay/observability_plugin.gocrates/node/tests/observability_plugin_tests.mjscrates/node/src/api/mod.rsgo/nemo_relay/observability_plugin_test.gocrates/core/tests/unit/observability/atof_tests.rspython/nemo_relay/_native.pyicrates/cli/src/doctor.rsgo/nemo_relay/nemo_relay.gocrates/core/src/observability/atof.rscrates/python/src/py_types/observability.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/observability/plugin_component.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/pii-redaction/tests/unit/component_tests.rscrates/ffi/tests/integration/api_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/src/api/observability.rscrates/cli/src/launcher.rscrates/ffi/tests/unit/api/core_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/coverage/session_tests.rscrates/node/observability.d.tspython/nemo_relay/__init__.pypython/nemo_relay/observability.pypython/tests/test_types.pycrates/node/observability.jsgo/nemo_relay/atof_test.gopython/tests/test_observability_plugin.pygo/nemo_relay/observability_plugin.gocrates/node/src/api/mod.rsgo/nemo_relay/observability_plugin_test.gocrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/doctor.rsgo/nemo_relay/nemo_relay.gocrates/core/src/observability/atof.rscrates/python/src/py_types/observability.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/observability/plugin_component.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/pii-redaction/tests/unit/component_tests.rscrates/ffi/tests/integration/api_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/tests/unit/api/core_tests.rscrates/node/tests/atof_tests.mjscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/coverage/session_tests.rspython/tests/test_types.pygo/nemo_relay/atof_test.gopython/tests/test_observability_plugin.pycrates/node/tests/observability_plugin_tests.mjsgo/nemo_relay/observability_plugin_test.gocrates/core/tests/unit/observability/atof_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rs
crates/ffi/**
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
Rebuild the FFI crate in release mode so the shared library and header stay in sync when making changes to crates/ffi
Files:
crates/ffi/tests/integration/api_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/nemo_relay.hcrates/ffi/src/api/observability.rscrates/ffi/tests/unit/api/core_tests.rs
crates/ffi/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/ffi, also usetest-ffi-surfacefor validationUse C FFI export names prefixed with
nemo_relay_in the raw C FFI layer.
Files:
crates/ffi/tests/integration/api_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/src/api/observability.rscrates/ffi/tests/unit/api/core_tests.rs
crates/{python,ffi,node}/**/*
⚙️ CodeRabbit configuration file
crates/{python,ffi,node}/**/*: Treat binding changes as public API changes. Check for parity with the other language bindings, FFI ownership/lifetime safety,
callback error propagation, stable type conversion, and consistent async/stream semantics.
Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere.
Files:
crates/ffi/tests/integration/api_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/nemo_relay.hcrates/ffi/src/api/observability.rscrates/ffi/tests/unit/api/core_tests.rscrates/node/tests/atof_tests.mjscrates/node/observability.d.tscrates/node/observability.jscrates/node/tests/observability_plugin_tests.mjscrates/node/src/api/mod.rscrates/python/src/py_types/observability.rs
crates/ffi/nemo_relay.h
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
Check the generated header diff when any exported symbol or type changed in the FFI surface
Update generated or generated-from-build surfaces such as
crates/ffi/nemo_relay.hthrough the proper build step.
Files:
crates/ffi/nemo_relay.h
{crates/ffi/src/api/*.rs,crates/ffi/nemo_relay.h}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Add or update the shared C/FFI surface in the relevant
crates/ffi/src/api/*.rsmodule, re-export it throughcrates/ffi/src/api/mod.rs, and keep the generatedcrates/ffi/nemo_relay.hheader correct.
Files:
crates/ffi/nemo_relay.hcrates/ffi/src/api/observability.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/ffi/src/api/observability.rscrates/cli/src/launcher.rspython/nemo_relay/__init__.pypython/nemo_relay/observability.pypython/tests/test_types.pypython/tests/test_observability_plugin.pycrates/node/src/api/mod.rscrates/cli/src/doctor.rscrates/core/src/observability/atof.rscrates/python/src/py_types/observability.rscrates/core/src/observability/plugin_component.rs
crates/node/**/*.{js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
camelCasefor Node.js public APIs.
Files:
crates/node/tests/atof_tests.mjscrates/node/observability.d.tscrates/node/observability.jscrates/node/tests/observability_plugin_tests.mjs
crates/node/**/*.{js,ts,jsx,tsx,json}
📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md)
Format changed Node files with
npm run format --workspace=nemo-relay-node
Files:
crates/node/observability.d.tscrates/node/observability.js
crates/node/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md)
Use
npm run check:docstrings --workspace=nemo-relay-nodeto validate public API docstring checks when surface docs changed
Files:
crates/node/observability.d.ts
**/*.{py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Keep Python, Go, and Node.js config objects and subscriber/exporter methods aligned so all bindings expose the same logical knobs and semantics.
Files:
crates/node/observability.d.tspython/nemo_relay/__init__.pypython/nemo_relay/observability.pypython/tests/test_types.pycrates/node/observability.jsgo/nemo_relay/atof_test.gopython/tests/test_observability_plugin.pygo/nemo_relay/observability_plugin.gogo/nemo_relay/observability_plugin_test.gogo/nemo_relay/nemo_relay.go
python/nemo_relay/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python wrapper modules live under
python/nemo_relay/, and the native extension is built fromcrates/pythonwithmaturin.
Files:
python/nemo_relay/__init__.pypython/nemo_relay/observability.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: When changing the Python wrapper package, tests, or docs tooling, lint with Ruff (E,F,W,I), format with Ruff formatter (120-character lines, double quotes), and passtytype checking.
Add the SPDX license header to all Python source files using the#comment form.
Files:
python/nemo_relay/__init__.pypython/nemo_relay/observability.pypython/tests/test_types.pypython/tests/test_observability_plugin.py
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
python/nemo_relay/__init__.pypython/nemo_relay/observability.pypython/nemo_relay/__init__.pyigo/nemo_relay/atof_test.gopython/nemo_relay/observability.pyigo/nemo_relay/observability_plugin.gocrates/node/src/api/mod.rsgo/nemo_relay/observability_plugin_test.gopython/nemo_relay/_native.pyigo/nemo_relay/nemo_relay.go
{python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update language wrapper helpers such as Python wrapper modules, Python type stubs, and Go shorthand packages when the new behavior belongs in those helper layers.
Files:
python/nemo_relay/__init__.pypython/nemo_relay/observability.pypython/nemo_relay/__init__.pyigo/nemo_relay/atof_test.gopython/nemo_relay/observability.pyigo/nemo_relay/observability_plugin.gogo/nemo_relay/observability_plugin_test.gopython/nemo_relay/_native.pyigo/nemo_relay/nemo_relay.go
python/nemo_relay/**/*
⚙️ CodeRabbit configuration file
python/nemo_relay/**/*: Review Python wrapper changes for typed API consistency, contextvars-based scope isolation, async behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/nemo_relay/__init__.pypython/nemo_relay/observability.pypython/nemo_relay/__init__.pyipython/nemo_relay/observability.pyipython/nemo_relay/_native.pyi
python/tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/test-python-binding/SKILL.md)
python/tests/**/*.py: Pytest is used to run tests.
Do not add@pytest.mark.asyncioto any test; async tests are automatically detected and run by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecconstructor argument when necessary.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures; if a fixture is needed in multiple test files, place it in aconftest.pyfile.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the fixture function asdef <fixture_name>_fixture() -> <return_type>:; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
Files:
python/tests/test_types.pypython/tests/test_observability_plugin.py
go/nemo_relay/**/*.go
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
go/nemo_relay/**/*.go: Format changed Go packages withcd go/nemo_relay && go fmt ./...
Run Go tests withjust test-goto build and test the NeMo Relay Go binding
Usejust build-gowhen you want an explicit build-only pass or need the artifact for other work
Usejust ci=true test-gowhen you need the CI-style coverage and JUnit path
On macOS, setDYLD_LIBRARY_PATHto the../../target/releasedirectory before running the rawgo testcommand directlyUse
PascalCasefor public Go APIs.
Files:
go/nemo_relay/atof_test.gogo/nemo_relay/observability_plugin.gogo/nemo_relay/observability_plugin_test.gogo/nemo_relay/nemo_relay.go
**/*.go
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When changing the experimental Go binding, format Go code with
gofmtand keepgo vet ./...passing.
Files:
go/nemo_relay/atof_test.gogo/nemo_relay/observability_plugin.gogo/nemo_relay/observability_plugin_test.gogo/nemo_relay/nemo_relay.go
go/nemo_relay/**
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Keep shared plugin helpers in
go/nemo_relayaligned with plugin registration, composition, and lifecycle behavior.
Files:
go/nemo_relay/atof_test.gogo/nemo_relay/observability_plugin.gogo/nemo_relay/observability_plugin_test.gogo/nemo_relay/nemo_relay.go
go/nemo_relay/**/*
⚙️ CodeRabbit configuration file
go/nemo_relay/**/*: Review Go binding changes for cgo memory ownership, race safety, callback cleanup, idiomatic exported APIs, and parity with Rust/FFI behavior.
Any API change should include focused Go tests and consider race-test behavior.
Files:
go/nemo_relay/atof_test.gogo/nemo_relay/observability_plugin.gogo/nemo_relay/observability_plugin_test.gogo/nemo_relay/nemo_relay.go
{justfile,codecov.yml,codecov.yaml,.github/workflows/**/*.yml,.github/workflows/**/*.yaml}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
justfile, Codecov, and CI package/test workflows must include new plugin crates and packages.
Files:
justfile
justfile
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Keep
justfilebuild, test, clean, version, and package recipes for plugin crates and packages aligned with the current packaging layout.
Files:
justfile
{.github/**,.gitlab-ci.yml,.pre-commit-config.yaml,justfile,scripts/**}
⚙️ CodeRabbit configuration file
{.github/**,.gitlab-ci.yml,.pre-commit-config.yaml,justfile,scripts/**}: Review automation changes for reproducibility, pinned versions where appropriate, secret handling, and consistency with the documented validation matrix.
Pay attention to commands that need generated native artifacts, FFI libraries, or platform-specific environment variables.
Files:
justfile
{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/atof_tests.rscrates/core/src/observability/atof.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/observability/plugin_component.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/atof_tests.rscrates/core/src/observability/atof.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/observability/plugin_component.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/atof_tests.rscrates/core/src/observability/atof.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/observability/plugin_component.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/atof_tests.rscrates/core/src/observability/atof.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/observability/plugin_component.rs
🧠 Learnings (3)
📚 Learning: 2026-05-07T18:04:44.387Z
Learnt from: mnajafian-nv
Repo: NVIDIA/NeMo-Flow PR: 67
File: integrations/openclaw/src/modules.ts:1-2
Timestamp: 2026-05-07T18:04:44.387Z
Learning: In NVIDIA/NeMo-Flow, TypeScript source files should use `//` line comments for SPDX headers (e.g., `// SPDX-FileCopyrightText: ...` and `// SPDX-License-Identifier: ...`) rather than C-style block comments (`/* ... */`). The repo’s copyright checker enforces this mapping, so `//` SPDX headers in `.ts` files should not be flagged as a style violation.
Applied to files:
crates/node/observability.d.ts
📚 Learning: 2026-07-14T02:53:55.471Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 414
File: crates/node/observability.d.ts:61-61
Timestamp: 2026-07-14T02:53:55.471Z
Learning: In `crates/node/observability.d.ts` and `crates/node/observability.js`, treat `OtlpConfig`/`otlpConfig` and related helpers as an intentional mirror of the snake_case TOML/plugin configuration schema consumed by `plugin.initialize()`. Do not apply the usual “Node.js public APIs use camelCase” naming review expectation to this plugin-config schema surface. Instead, camelCase review expectations should apply to the native binding surface (e.g., `OpenTelemetrySubscriber`/`OpenInferenceSubscriber` constructors and their `attributeMappings`), which expose camelCase separately.
Applied to files:
crates/node/observability.d.tscrates/node/observability.js
📚 Learning: 2026-07-14T02:53:44.529Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 414
File: crates/node/tests/observability_plugin_tests.mjs:34-34
Timestamp: 2026-07-14T02:53:44.529Z
Learning: Do not flag camelCase style violations for keys returned by observability plugin configuration helpers (e.g., `observability.otlpConfig()` and similar helpers like `atofConfig()` / `atifConfig()`) in the Node observability module and its tests. These helpers intentionally return the snake_case plugin configuration schema consumed by `plugin.initialize()` and written/read via TOML. This is distinct from the Node public API / native subscriber options (e.g., fields like `attributeMappings`) which follow the camelCase guideline; only the plugin-config schema helpers should be exempt.
Applied to files:
crates/node/observability.jscrates/node/tests/observability_plugin_tests.mjs
🪛 ast-grep (0.44.1)
go/nemo_relay/nemo_relay.go
[warning] 345-345: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(status)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🪛 Ruff (0.15.21)
python/nemo_relay/observability.py
[warning] 88-88: Remove quotes from type annotation
Remove quotes
(UP037)
python/tests/test_types.py
[warning] 484-484: Unused method argument: tmp_path
(ARG002)
🔇 Additional comments (42)
crates/node/observability.d.ts (1)
11-34: LGTM!python/nemo_relay/observability.py (1)
58-120: LGTM!Also applies to: 255-255, 298-299
python/nemo_relay/observability.pyi (1)
23-46: LGTM!python/tests/test_observability_plugin.py (1)
18-19: LGTM!Also applies to: 33-33, 57-74, 104-122, 147-153
crates/node/observability.js (1)
13-19: LGTM! The version bump to 2 and dropping the stale top-levelmodedefault are consistent with the new per-sink schema.Based on learnings, the snake_case fields here (
version,enabled) intentionally mirror the plugin configuration schema and are exempt from the Node camelCase convention.Also applies to: 22-32
go/nemo_relay/nemo_relay.go (2)
337-346: LGTM! Always marshalingconfigand delegating tocreate_from_jsonis a clean simplification and correctly aligns with the FFI contract (nemo_relay_atof_exporter_create_from_jsondeserializes the tagged sink JSON).
1769-1811: LGTM!NewAtofExporterConfig/NewAtofFileSinkConfig/NewAtofStreamSinkConfigdefaults match the Rust core defaults, andPath()'s pointer semantics correctly represent "no local path" for stream-backed exporters.go/nemo_relay/observability_plugin.go (2)
34-49: LGTM! TheSinks-based model correctly matches the coreAtofFileSinkConfig/AtofStreamSinkConfigfield sets, and the sealedatofSinkConfig()interface method appropriately restricts sink implementations to this package.The
MarshalJSONfield duplication here shares the same pattern flagged ingo/nemo_relay/nemo_relay.go; see the consolidated comment.Also applies to: 64-72
235-253: LGTM! Version 2 default and the sink-config constructors' defaults (appendfor file,http_post/3000ms/preservefor stream) match core defaults.go/nemo_relay/atof_test.go (1)
17-44: LGTM! Sink-based construction,Path()pointer handling, and mode assertions are all correctly updated for the new API.Also applies to: 46-61, 87-87, 102-138
go/nemo_relay/observability_plugin_test.go (1)
29-104: LGTM! Test assertions correctly track theSinks-based config shape and the version-2 default.Also applies to: 229-247, 322-338
crates/pii-redaction/tests/unit/component_tests.rs (1)
672-672: LGTM! Correctly adapts topath()now returningOption<PathBuf>, with a clear panic message if the file-sink invariant is ever violated.justfile (1)
1127-1129: LGTM! IsolatingXDG_CONFIG_HOMEper test run correctly prevents Python/Node tests from picking up a real~/.config/nemo-relay, matching core'suser_config_dir()resolution order. Confirmed npm's own per-user config defaults to$HOME/.npmrcrather thanXDG_CONFIG_HOME, so this isolation doesn't interfere withnpm install/npm test.Also applies to: 1369-1371
crates/node/src/api/mod.rs (3)
175-240: LGTM!
3352-3378: LGTM!
3395-3401: LGTM!crates/node/tests/atof_tests.mjs (1)
36-71: LGTM!crates/node/tests/observability_plugin_tests.mjs (1)
21-23: LGTM!Also applies to: 42-42, 53-66, 76-78, 111-114, 160-164
crates/python/src/py_types/observability.rs (4)
179-184: LGTM!
186-254: LGTM!
349-355: LGTM!
169-169: 🎯 Functional Correctness
from_py_objectis a valid#[pyclass]option here; no change needed.> Likely an incorrect or invalid review comment.python/nemo_relay/_native.pyi (1)
833-886: LGTM!python/tests/test_types.py (1)
15-16: LGTM!Also applies to: 453-483, 493-524
crates/core/src/observability/atof.rs (4)
169-297: LGTM!
332-353: LGTM!Also applies to: 368-397
444-520: LGTM!
400-420: 🎯 Functional CorrectnessNo issue here The workspace targets Rust 2024 and pins
rust-toolchain.tomlto1.93.0, so thisletchain is supported.> Likely an incorrect or invalid review comment.crates/core/tests/unit/observability/atof_tests.rs (1)
352-368: LGTM!Also applies to: 371-400, 506-527, 530-548, 551-572, 576-612, 688-869, 872-971, 974-1041, 1044-1100, 1103-1199, 1202-1251, 1272-1287, 1652-1673
crates/core/src/observability/plugin_component.rs (3)
150-218: LGTM!Also applies to: 486-566
750-794: LGTM!Also applies to: 1593-1610, 1698-1718, 1841-1854, 1890-2136, 2442-2444
706-748: 🩺 Stability & AvailabilityCheck
AtofExporterteardown on drop. IfDropdoes not fully stop the endpoint worker and close any opened handles, a later sink failure can leave partially constructed exporters leaking resources.crates/core/tests/unit/observability/plugin_component_tests.rs (1)
21-91: LGTM!Also applies to: 119-147, 253-305, 358-370, 375-427, 473-633, 627-765, 768-828, 831-925
crates/cli/src/doctor.rs (1)
738-799: LGTM!Also applies to: 960-1049, 1131-1284
crates/cli/src/launcher.rs (1)
671-695: LGTM!crates/cli/tests/coverage/doctor_tests.rs (1)
875-891: LGTM!crates/cli/tests/coverage/plugins_tests.rs (1)
12-14: LGTM!Also applies to: 2538-2555
crates/cli/tests/coverage/session_tests.rs (1)
3022-3022: LGTM!Also applies to: 3165-3165, 3509-3509
crates/ffi/nemo_relay.h (1)
1296-1296: LGTM!crates/ffi/src/api/observability.rs (1)
471-471: LGTM!Correctly mirrors the core
path() -> Option<PathBuf>contract and returns a null pointer withNemoRelayStatus::Okfor stream sinks, consistent with the Python (Option<String>) and Node bindings' handling of the same optional-path semantics.Also applies to: 489-493
crates/ffi/tests/integration/api/coverage_sweeps_tests.rs (1)
1086-1086: LGTM!crates/ffi/tests/unit/api/core_tests.rs (1)
204-212: LGTM!Also applies to: 241-241, 332-332
Signed-off-by: Will Killian <wkillian@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/core/src/observability/plugin_component.rs (3)
750-794: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMode/transport/field_name_policy parsing and error strings are duplicated between
build_atof_sink_configandvalidate_atof_stream_sink_values.Both functions independently re-parse
mode,transport, andfield_name_policywith hard-coded, duplicated error text. If one copy's accepted values or message wording changes, the other can silently drift, causing validation and registration to disagree.Consider extracting shared parse-or-diagnostic helpers (e.g.
parse_atof_mode(index, &str) -> Result<AtofExporterMode, String>) used by both the validation and build 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/core/src/observability/plugin_component.rs` around lines 750 - 794, Extract shared parse-and-diagnostic helpers for AtofExporterMode, AtofEndpointTransport, and AtofEndpointFieldNamePolicy, using the existing index-aware invalid-value messages. Update build_atof_sink_config and validate_atof_stream_sink_values to call these helpers so accepted values and diagnostics remain consistent.
737-745: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShutdown only surfaces the first exporter's error; later failures are silently dropped.
The loop correctly continues shutting down every exporter after one fails (good), but only the first error is returned/reported — if a second sink also fails to flush/close, that failure is invisible to callers/logs.
♻️ Suggested: collect and log all shutdown errors
Box::new(move || { - let mut first_error = None; + let mut errors = Vec::new(); for exporter in &exporters { if let Err(error) = exporter.shutdown() { - first_error.get_or_insert_with(|| observability_registration_error(error)); + errors.push(observability_registration_error(error)); } } - first_error.map_or(Ok(()), Err) + errors.into_iter().next().map_or(Ok(()), |error| { + tracing::warn!(?errors, "additional ATOF sink shutdown errors"); + Err(error) + }) }),🤖 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/core/src/observability/plugin_component.rs` around lines 737 - 745, Update the shutdown closure around the exporter loop to collect every error returned by exporter.shutdown(), rather than retaining only the first via first_error. Ensure all exporters are still attempted, and aggregate or otherwise report all collected errors through the returned Result so no shutdown failure is silently discarded.
710-794: 📐 Maintainability & Code Quality | 🔵 TrivialRun the required validation matrix for this
crates/corechange. Includecargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust, and the broader Rust/Python/Go/Node matrix.🤖 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/core/src/observability/plugin_component.rs` around lines 710 - 794, Validate the ATOF changes around build_atof_sink_config and its registration flow by running cargo fmt --all, cargo clippy --workspace --all-targets -- -D warnings, and just test-rust. Then run the repository’s documented broader Rust, Python, Go, and Node validation matrix, fixing any failures before completing the change.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@crates/core/src/observability/plugin_component.rs`:
- Around line 750-794: Extract shared parse-and-diagnostic helpers for
AtofExporterMode, AtofEndpointTransport, and AtofEndpointFieldNamePolicy, using
the existing index-aware invalid-value messages. Update build_atof_sink_config
and validate_atof_stream_sink_values to call these helpers so accepted values
and diagnostics remain consistent.
- Around line 737-745: Update the shutdown closure around the exporter loop to
collect every error returned by exporter.shutdown(), rather than retaining only
the first via first_error. Ensure all exporters are still attempted, and
aggregate or otherwise report all collected errors through the returned Result
so no shutdown failure is silently discarded.
- Around line 710-794: Validate the ATOF changes around build_atof_sink_config
and its registration flow by running cargo fmt --all, cargo clippy --workspace
--all-targets -- -D warnings, and just test-rust. Then run the repository’s
documented broader Rust, Python, Go, and Node validation matrix, fixing any
failures before completing the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: bfd7630d-bb8d-447a-9b99-b65eb20e698e
📒 Files selected for processing (8)
crates/cli/tests/cli_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/observability/plugin_component.rsintegrations/openclaw/test/live-smoke.test.ts
📜 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,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:
integrations/openclaw/test/live-smoke.test.tscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/core/src/observability/plugin_component.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:
integrations/openclaw/test/live-smoke.test.tscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/core/src/observability/plugin_component.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:
integrations/openclaw/test/live-smoke.test.tscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/core/src/observability/plugin_component.rs
**/*.{py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Keep Python, Go, and Node.js config objects and subscriber/exporter methods aligned so all bindings expose the same logical knobs and semantics.
Files:
integrations/openclaw/test/live-smoke.test.ts
**/*
📄 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:
integrations/openclaw/test/live-smoke.test.tscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/core/src/observability/plugin_component.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:
integrations/openclaw/test/live-smoke.test.tscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/core/src/observability/plugin_component.rs
**/*.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/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/core/src/observability/plugin_component.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/core/src/observability/plugin_component.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/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/tests/coverage/doctor_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/plugins_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/plugin_component.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/plugin_component.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/plugin_component.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/plugin_component.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/plugin_component.rs
🧠 Learnings (1)
📚 Learning: 2026-05-07T18:04:44.387Z
Learnt from: mnajafian-nv
Repo: NVIDIA/NeMo-Flow PR: 67
File: integrations/openclaw/src/modules.ts:1-2
Timestamp: 2026-05-07T18:04:44.387Z
Learning: In NVIDIA/NeMo-Flow, TypeScript source files should use `//` line comments for SPDX headers (e.g., `// SPDX-FileCopyrightText: ...` and `// SPDX-License-Identifier: ...`) rather than C-style block comments (`/* ... */`). The repo’s copyright checker enforces this mapping, so `//` SPDX headers in `.ts` files should not be flagged as a style violation.
Applied to files:
integrations/openclaw/test/live-smoke.test.ts
🔇 Additional comments (15)
integrations/openclaw/test/live-smoke.test.ts (1)
35-35: LGTM!crates/core/src/observability/plugin_component.rs (4)
160-217: LGTM!
1895-1963: LGTM! Sink-empty guard and per-sink mode/url/transport validation logic look correct — empty-URL and unknown-transport diagnostics are mutually exclusive from the URL-format check as intended.
2047-2068: LGTM! The#[cfg(not(feature = "atof-streaming"))] let _ = value;correctly silences the unused-parameter warning when the reqwest-gated header-value validation is compiled out.
710-722: 🩺 Stability & AvailabilityNo cleanup gap here.
AtofExporter’s worker threads are owned by the exporter’s sender; if construction fails before registration, dropping the temporaryArc<AtofExporter>values closes the channel and the worker loops exit.> Likely an incorrect or invalid review comment.crates/cli/tests/coverage/plugins_tests.rs (3)
188-213: LGTM!
652-695: LGTM! Fixture and assertion updates correctly reflect the newatof.sinkstagged-union shape, including defaultmode: "append"injection.Also applies to: 768-795, 817-883
1478-1506: LGTM!Also applies to: 1985-2013, 2284-2297, 2575-2592
crates/cli/tests/coverage/server_tests.rs (1)
573-664: LGTM! Fixtures consistently migrated to theatof.sinksfile-sink format, and the invalid-mode assertion correctly matches the updated core validation error path.Also applies to: 684-770, 773-973, 976-1122, 1125-1241, 1244-1536, 1539-1672, 1839-1871
crates/cli/tests/coverage/doctor_tests.rs (2)
1097-1145: LGTM! The expected check name "ATOF stream sink" and sink-indexed structure matchprobe_atof_stream_sinkincrates/cli/src/doctor.rs.
1183-1215: LGTM!crates/cli/tests/coverage/launcher_tests.rs (1)
339-398: LGTM!crates/cli/tests/coverage/session_tests.rs (2)
98-120: LGTM!
159-165: LGTM!read_atof_eventsitself is unchanged; callers correctly.expect()the now-optional file sink path before invoking it.crates/cli/tests/cli_tests.rs (1)
1541-1591: LGTM!
Signed-off-by: Will Killian <wkillian@nvidia.com>
|
/merge |
#### Overview Documents the v2 multi-sink ATOF export configuration introduced by #416. This is a documentation and E2E-fixture follow-up; it does not change runtime behavior. - [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 - Updates ATOF, ATIF, observability-configuration, and plugin-configuration-file guidance for named sink configurations. - Updates the coding-agent E2E fixtures and relevant agent-skill references to use the v2 schema. - Covers tagged sinks, configuration precedence, and migration from the legacy single-sink configuration. - Contains no implementation changes. - Breaking changes: none. Validation: - `just docs` - `just docs-linkcheck` - targeted `uv run pre-commit run --files ...` - `bash -n` for the three updated E2E scripts #### Where should the reviewer start? Start with `docs/configure-plugins/observability/atof.mdx`. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to: #416 Authors: - Will Killian (https://github.com/willkill07) Approvers: - Maryam Najafian (https://github.com/mnajafian-nv) - https://github.com/lvojtku URL: #417
2 participants
Warning
BREAKING CHANGE — ATOF configuration and manual exporter APIs: observability config now requires
components.config.version = 2for ATOF and represents destinations as a taggedatof.sinksarray. Legacyoutput_directory,filename,mode, andendpointsfields are rejected rather than translated. Manual ATOF exporters now accept one typed file or stream sink;pathis absent for stream-backed exporters.Overview
Add fan-out ATOF export through independently configured file and stream sinks.
Details
header_envstream headers.Where should the reviewer start?
Start with
crates/core/src/observability/plugin_component.rsfor the v2 validation and fan-out lifecycle, thencrates/core/src/observability/atof.rsfor the single-sink exporter contract.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Closes RELAY-459
Summary by CodeRabbit
Summary by CodeRabbit
New Features
sinks(file and stream), including multi-sink destination support.header_envsupport for stream sinks.Bug Fixes
sinks[...]details, with clearer messaging when no file sinks are configured or streaming is unavailable.Documentation
sinksmodel (with compatibility aliases for prior endpoint naming).