feat(observability)!: unify OpenTelemetry exporters#556
Conversation
WalkthroughThe observability system now uses version 3 typed OpenTelemetry endpoints for ChangesTyped OpenTelemetry observability
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:XXL
github-actions
Bot
added
lang:go
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
python/tests/test_types.py (1)
566-598: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParametrize duplicated config tests across OpenTelemetry/OpenInference.
TestOpenTelemetryTypes.test_config_defaults_mutation_and_repr/test_config_rejects_invalid_map_valuesand theirTestOpenInferenceTypescounterparts are now nearly identical bodies, differing only by theotel_typestring literal ("full"vs"openinference"), since both now share the sameOpenTelemetryConfigclass. Consider merging into@pytest.mark.parametrize("otel_type", ["full", "openinference"])to remove the duplication.As per path instructions for
python/tests/**/*.py: "Preferpytest.mark.parametrizeover creating individual tests for different input types."Also applies to: 690-721
🤖 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 `@python/tests/test_types.py` around lines 566 - 598, Merge the duplicated OpenTelemetry and OpenInference configuration tests into parametrized tests using pytest.mark.parametrize("otel_type", ["full", "openinference"]). Update both test_config_defaults_mutation_and_repr and test_config_rejects_invalid_map_values to accept otel_type and construct OpenTelemetryConfig with it, preserving all existing assertions and validation coverage.Source: Path instructions
crates/python/tests/coverage/py_types_coverage_tests.rs (1)
446-501: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the per-type lifecycle/invalid-input tests.
test_openinference_typed_otel_config_and_subscriber_cover_lifecycleandtest_openinference_typed_otel_config_rejects_invalid_inputsare near-verbatim copies of thefullvariants; only the type string and subscriber name prefix differ. Driving both from a loop over["full", "gen_ai", "openinference"]also gainsgen_aicoverage, which is currently missing here.🤖 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/python/tests/coverage/py_types_coverage_tests.rs` around lines 446 - 501, Deduplicate the lifecycle and invalid-input coverage by replacing the separate full/openinference test variants with loop-driven tests over the type strings "full", "gen_ai", and "openinference". Parameterize PyOpenTelemetryConfig construction and subscriber-name prefixes from the loop while preserving all existing assertions and lifecycle operations, thereby adding gen_ai coverage without duplicating test bodies.crates/core/src/observability/otel.rs (1)
687-698: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
u32::MAXattribute caps remove the only bound on per-span memory.The rationale for lifting the 128 default is sound, but
u32::MAXis effectively unbounded. Thefullprojection flattens arbitrary event JSON into attributes viapush_top_level_json_attributes, so a long-running agent span that accumulates many marks and large payloads now grows the SDK's in-memory span without limit until it ends. Prefer a large finite ceiling that still fails safe.🔧 Proposed fix
- .with_max_attributes_per_span(u32::MAX) - .with_max_attributes_per_event(u32::MAX); + // Large but finite: consumers legitimately emit hundreds of attributes on + // long-running spans, while a hard ceiling keeps per-span memory bounded. + .with_max_attributes_per_span(65_536) + .with_max_attributes_per_event(4_096);🤖 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/otel.rs` around lines 687 - 698, Replace the unbounded u32::MAX limits in the SdkTracerProvider builder with a large finite attribute cap that accommodates expanded event payloads while preserving a safe upper bound for per-span memory. Apply the same bounded value to both with_max_attributes_per_span and with_max_attributes_per_event, leaving the resource and ID generator configuration unchanged.
🤖 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 @.agents/skills/maintain-observability/SKILL.md:
- Around line 36-37: Update the validation checklist in the observability skill
to explicitly require the Rust test target for exporter-configuration and
lifecycle changes, in addition to event-field changes. Ensure both exporter and
lifecycle paths cannot bypass the existing required Rust validation.
- Around line 29-32: Update the affected lists in maintain-observability
documentation with complete lead-in sentences, including “Keep these surfaces
synchronized:” and “Use the following design checklist:” where applicable, and
add question marks to every checklist question. Preserve the existing list
content and structure while applying the same punctuation fixes to the sections
referenced by the additional ranges.
In `@ATTRIBUTIONS-Rust.md`:
- Around line 2911-2915: Update the attribution generator/template that emits
dependency license sections such as the aws-lc-rs 1.17.3 block, adding blank
lines around Markdown headings and fenced license content and specifying a
language such as text on license fences. Regenerate ATTRIBUTIONS-Rust.md so all
affected license blocks are corrected consistently rather than editing a single
occurrence.
- Around line 24039-24048: Update the Apache License boilerplate in the
attribution block to use the appendix’s exact “[yyyy]” and “[name of copyright
owner]” placeholders, preserving the surrounding text verbatim.
In `@crates/cli/src/diagnostics/mod.rs`:
- Around line 653-679: The endpoint probes in the loop are awaited serially,
causing total diagnostics time to grow with the number of endpoints. Update this
endpoint-checking flow to create one future per endpoint and join them
concurrently, preserving the existing per-endpoint probe selection, details,
failure handling, and output ordering.
In `@crates/cli/src/diagnostics/probes.rs`:
- Around line 87-98: Update the endpoint connection logic around
parsed.host_str() and TcpStream::connect to pass an unbracketed IPv6 host, using
Url::host() or stripping surrounding brackets before connecting. Preserve
existing host validation and port handling, and add an IPv6 coverage case to
tcp_tests.
In `@crates/core/src/observability/openinference.rs`:
- Around line 683-711: Define module-local constants for all OpenInference
attribute keys used by start_attributes, end_attributes,
push_projected_mark_attributes, common_attributes, and the push/remove helpers,
following the const-block pattern in otel_genai.rs. Replace every matching
inline key literal, including the duplicated tool.name and
tool_call.function.name keys, with the shared constants while preserving the
existing attribute values and behavior.
In `@crates/core/src/observability/otel_genai.rs`:
- Around line 307-312: Update the tool description lookup in the event attribute
handling to remove the generic "description" fallback or constrain it to the
top-level event object. Preserve the existing namespaced and tool_description
lookups, and ensure nested usage/request/response values cannot populate
gen_ai.tool.description.
- Around line 404-417: Align the status-code comparison in push_error_attributes
and set_span_status_from_event_metadata so both use the same case-sensitivity
policy. Preserve consistent handling of metadata values such as "error": the
span status and error.type attribute must agree, and valid case variants must
not trigger invalid_status_code warnings.
In `@crates/core/src/observability/otel.rs`:
- Around line 562-603: Bound both owned exporter runtime handoffs: in
crates/core/src/observability/otel.rs lines 562-603, update
build_owned_tracer_provider to use result_receiver.recv_timeout with a duration
derived from config.timeout, and document that construction blocks the calling
thread; in lines 324-339, call Runtime::shutdown_timeout inside the spawned
runtime closure with the configured timeout so teardown cannot wait indefinitely
before thread joining.
In `@crates/core/src/observability/plugin_component.rs`:
- Around line 2032-2048: Update the diagnostic message passed to
push_policy_diag in the REMOVED-field branch so removed OpenTelemetry fields
explicitly state that the control was removed and provide the version-3
migration guidance, matching the legacy openinference wording near line 1895;
retain the existing unknown-field message for non-REMOVED fields.
- Around line 1762-1775: Update the blank-endpoint and invalid-transport errors
in the OpenTelemetry endpoint validation to prefix messages with the in-scope
endpoint index using the existing `endpoints[{index}]` format. Also update the
`initialization_fails_for_invalid_enabled_file_exporters` assertion to match the
new indexed transport error while preserving its existing validation intent.
In `@crates/core/tests/integration/middleware_tests.rs`:
- Around line 848-862: Restore explicit tool-projection coverage in the OTel
setup around from_tracer_provider by constructing the subscriber with
MarkProjection::Tool where required, while preserving the existing pending-mark
assertions. Add an OpenInference-specific assertion for openinference.span.kind
equal to "CHAIN" on the OpenInference span, so its exporter-specific projection
remains distinguished from the full projection.
In `@crates/core/tests/unit/observability/openinference_tests.rs`:
- Around line 716-734: Update invalid_grpc_headers_are_rejected to call
OpenTelemetrySubscriber::new directly instead of creating a Tokio runtime and
wrapping the call in runtime.block_on; preserve the existing invalid-header
error assertion and configuration unchanged.
- Around line 684-700: Strengthen
config_defaults_and_builder_overrides_are_applied by reading back and asserting
every configured builder value, including the header via
OpenTelemetryConfig::header(), rather than only checking construction and
lifecycle calls. Update grpc_metadata_and_runtime_builder_paths_succeed
similarly to assert returned results and use unwrap() for expected success
instead of discarding them with ok(). Avoid starting real runtimes and exporters
for these unit tests where possible, while preserving coverage of the promised
configuration and error behavior.
- Around line 3210-3214: Replace the hardcoded 4096 in the span eviction test’s
span_count calculation with the reachable COMPLETED_SPAN_CONTEXT_LIMIT constant,
preserving the existing +2 setup so the test continues to exercise eviction when
the configured limit changes.
In `@crates/core/tests/unit/observability/otel_tests.rs`:
- Around line 585-593: Update grpc_config_owns_its_tokio_runtime to call
subscriber.force_flush() before shutdown, asserting the result succeeds so the
test exercises dispatch through the subscriber’s owned Tokio runtime rather than
only validating construction.
- Around line 641-672: Update direct_config_rejects_process_global_otel_headers
so each child process captures its output and verifies the targeted test
actually ran, such as requiring the libtest summary to report one test passed,
before accepting status.success(). Preserve the existing error assertion and
environment-variable loop, while ensuring a renamed or relocated test cannot
silently pass with zero tests executed.
In `@crates/core/tests/unit/observability/plugin_component_tests.rs`:
- Around line 166-201: Harden start_otlp_capture_server by matching the
buffered, tolerant approach used by start_otlp_http_collector: replace the
single-byte header loop with buffered reads, handle chunked or missing
content-length without an unconditional unwrap, and provide an explicit
assertion message when the header is absent. Send the captured body before
writing the response, while preserving the existing endpoint and channel
behavior.
In `@crates/core/tests/unit/plugin_tests.rs`:
- Around line 497-499: Update the assertions for the non-observability component
alpha in the merge_plugin_config_value tests to expect its overlay config.list
as json!([9]) rather than concatenating lists; adjust the nearby test comment to
state that list concatenation applies only to observability components while
non-observability lists are replaced.
In `@docs/build-plugins/language-binding/code-examples.mdx`:
- Around line 183-187: Update both Python and Node.js examples to invoke each
exporter’s documented flush operation before calling plugin.clear(). Preserve
the existing shutdown sequence otherwise, ensuring queued native subscriber
callbacks are delivered before the plugin is cleared.
In `@docs/resources/troubleshooting/trace-incident-runbook.mdx`:
- Around line 152-156: Update the troubleshooting guidance around the typed
OpenTelemetry export settings and the following attribute-check bullet so
`nemo_relay.uuid` and `nemo_relay.parent_uuid` are expected only for `full` and
`openinference` endpoints, not `gen_ai` endpoints; preserve the existing
validation guidance for non-`gen_ai` projections.
In `@examples/switchyard/hermes-ollama-plugins.toml`:
- Around line 57-59: Remove the duplicate TOML table headers in
examples/switchyard/hermes-ollama-plugins.toml: retain exactly one
[components.config] declaration at lines 57-59 and exactly one
[[components.config.opentelemetry.endpoints]] declaration before its endpoint
fields at lines 91-100.
In `@go/nemo_relay/openinference_test.go`:
- Around line 22-24: Prevent the shared NewOtelTestServer handler from blocking
during shutdown by making its request channel send non-blocking or increasing
its buffer; update go/nemo_relay/openinference_test.go lines 22-24 and
go/nemo_relay/otel_test.go lines 205-246 consistently, including the requests
channel in TestOpenTelemetrySubscriberExportsGenAIAgentProjection.
---
Outside diff comments:
In `@crates/core/src/observability/otel.rs`:
- Around line 687-698: Replace the unbounded u32::MAX limits in the
SdkTracerProvider builder with a large finite attribute cap that accommodates
expanded event payloads while preserving a safe upper bound for per-span memory.
Apply the same bounded value to both with_max_attributes_per_span and
with_max_attributes_per_event, leaving the resource and ID generator
configuration unchanged.
In `@crates/python/tests/coverage/py_types_coverage_tests.rs`:
- Around line 446-501: Deduplicate the lifecycle and invalid-input coverage by
replacing the separate full/openinference test variants with loop-driven tests
over the type strings "full", "gen_ai", and "openinference". Parameterize
PyOpenTelemetryConfig construction and subscriber-name prefixes from the loop
while preserving all existing assertions and lifecycle operations, thereby
adding gen_ai coverage without duplicating test bodies.
In `@python/tests/test_types.py`:
- Around line 566-598: Merge the duplicated OpenTelemetry and OpenInference
configuration tests into parametrized tests using
pytest.mark.parametrize("otel_type", ["full", "openinference"]). Update both
test_config_defaults_mutation_and_repr and
test_config_rejects_invalid_map_values to accept otel_type and construct
OpenTelemetryConfig with it, preserving all existing assertions and validation
coverage.
🪄 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: accc008a-b705-4c25-a893-4fe6a54a6e66
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (118)
.agents/skills/maintain-observability/SKILL.mdATTRIBUTIONS-Rust.mdCargo.tomlREADME.mdabout.tomlcrates/cli/Cargo.tomlcrates/cli/README.mdcrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/probes.rscrates/cli/src/error.rscrates/cli/src/gateway/tls.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/shared/error_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/core/Cargo.tomlcrates/core/README.mdcrates/core/src/lib.rscrates/core/src/observability/manual.rscrates/core/src/observability/mod.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/otel_genai.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin.rscrates/core/tests/integration/atif_storage_tests.rscrates/core/tests/integration/middleware_tests.rscrates/core/tests/unit/observability/exporter_parity_tests.rscrates/core/tests/unit/observability/manual_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugin_tests.rscrates/ffi/Cargo.tomlcrates/ffi/README.mdcrates/ffi/nemo_relay.hcrates/ffi/src/api/mod.rscrates/ffi/src/api/observability.rscrates/ffi/src/types/mod.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/tests/unit/api/core_tests.rscrates/ffi/tests/unit/api/coverage_sweeps_tests.rscrates/ffi/tests/unit/api/plugin_tests.rscrates/ffi/tests/unit/api/registry_tests.rscrates/ffi/tests/unit/types_tests.rscrates/node/Cargo.tomlcrates/node/README.mdcrates/node/observability.d.tscrates/node/observability.jscrates/node/src/api/mod.rscrates/node/tests/observability_plugin_tests.mjscrates/node/tests/openinference_tests.mjscrates/node/tests/otel_tests.mjscrates/pii-redaction/Cargo.tomlcrates/pii-redaction/tests/unit/component_tests.rscrates/python/Cargo.tomlcrates/python/src/py_types/mod.rscrates/python/src/py_types/observability.rscrates/python/tests/coverage/py_types_coverage_tests.rsdeny.tomldocs/about-nemo-relay/agent-runtime-primer.mdxdocs/about-nemo-relay/concepts/codecs.mdxdocs/about-nemo-relay/concepts/events.mdxdocs/about-nemo-relay/concepts/plugins.mdxdocs/about-nemo-relay/concepts/subscribers.mdxdocs/about-nemo-relay/ecosystem.mdxdocs/about-nemo-relay/overview.mdxdocs/about-nemo-relay/release-notes/index.mdxdocs/build-plugins/language-binding/code-examples.mdxdocs/configure-plugins/about.mdxdocs/configure-plugins/observability/about.mdxdocs/configure-plugins/observability/atif.mdxdocs/configure-plugins/observability/atof.mdxdocs/configure-plugins/observability/configuration.mdxdocs/configure-plugins/observability/openinference.mdxdocs/configure-plugins/observability/opentelemetry.mdxdocs/configure-plugins/pii-redaction/configuration.mdxdocs/configure-plugins/plugin-configuration-files.mdxdocs/configure-plugins/switchyard/configuration.mdxdocs/contribute/runtime-contract-docs.mdxdocs/getting-started/configuration.mdxdocs/integrate-into-frameworks/provider-response-codecs.mdxdocs/nemo-relay-cli/claude-code.mdxdocs/reference/migration-guides.mdxdocs/resources/glossary.mdxdocs/resources/troubleshooting/trace-incident-runbook.mdxdocs/supported-integrations/openclaw-plugin.mdxexamples/switchyard/hermes-ollama-plugins.tomlgo/nemo_relay/README.mdgo/nemo_relay/coverage_gap_test.gogo/nemo_relay/nemo_relay.gogo/nemo_relay/observability_plugin.gogo/nemo_relay/observability_plugin_test.gogo/nemo_relay/openinference_test.gogo/nemo_relay/otel_test.gogo/nemo_relay/top_level_coverage_test.gointegrations/coding-agents/README.mdintegrations/openclaw/README.mdintegrations/openclaw/index.tsintegrations/openclaw/openclaw.plugin.jsonintegrations/openclaw/src/config.tsintegrations/openclaw/src/health.tsintegrations/openclaw/test/config.test.tsintegrations/openclaw/test/live-smoke.test.tspython/nemo_relay/README.mdpython/nemo_relay/__init__.pypython/nemo_relay/__init__.pyipython/nemo_relay/_native.pyipython/nemo_relay/observability.pypython/nemo_relay/observability.pyipython/tests/integrations/deepagents_tests/test_deepagents_integration.pypython/tests/test_observability_plugin.pypython/tests/test_types.pyscripts/licensing/attributions_lockfile_md.py
💤 Files with no reviewable changes (7)
- crates/core/src/observability/manual.rs
- crates/python/src/py_types/mod.rs
- crates/ffi/tests/unit/types_tests.rs
- python/nemo_relay/init.pyi
- crates/cli/tests/coverage/shared/error_tests.rs
- crates/ffi/src/types/mod.rs
- crates/core/tests/unit/observability/manual_tests.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (24)
.agents/skills/maintain-observability/SKILL.md (2)
29-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add lead-in sentences and list punctuation.
These changed lists need complete lead-ins, and the checklist questions should end with
?to comply with the Markdown documentation guidelines. For example, introduce the sections with “Keep these surfaces synchronized:” and “Use the following design checklist:”.Also applies to: 34-51, 64-66
🤖 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 @.agents/skills/maintain-observability/SKILL.md around lines 29 - 32, Update the affected lists in maintain-observability documentation with complete lead-in sentences, including “Keep these surfaces synchronized:” and “Use the following design checklist:” where applicable, and add question marks to every checklist question. Preserve the existing list content and structure while applying the same punctuation fixes to the sections referenced by the additional ranges.Source: Coding guidelines
36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover exporter and lifecycle validation explicitly.
This skill covers exporter behavior and subscriber configuration, but its validation guidance only requires the Rust test target when event fields change. Add an explicit validation requirement for exporter-configuration and lifecycle changes so those paths cannot bypass the required Rust checks.
Also applies to: 49-51
🤖 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 @.agents/skills/maintain-observability/SKILL.md around lines 36 - 37, Update the validation checklist in the observability skill to explicitly require the Rust test target for exporter-configuration and lifecycle changes, in addition to event-field changes. Ensure both exporter and lifecycle paths cannot bypass the existing required Rust validation.Source: Learnings
ATTRIBUTIONS-Rust.md (2)
2911-2915: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Regenerate the attribution file without Markdown lint violations.
This block—and the other reported license blocks—needs blank lines around headings and fences, and the license fence should specify a language such as
text. Fix the attribution generator/template rather than hand-editing one occurrence.🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 2911-2911: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 2914-2914: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above(MD022, blanks-around-headings)
[warning] 2914-2914: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 2915-2915: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 2915-2915: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@ATTRIBUTIONS-Rust.md` around lines 2911 - 2915, Update the attribution generator/template that emits dependency license sections such as the aws-lc-rs 1.17.3 block, adding blank lines around Markdown headings and fenced license content and specifying a language such as text on license fences. Regenerate ATTRIBUTIONS-Rust.md so all affected license blocks are corrected consistently rather than editing a single occurrence.Source: Linters/SAST tools
24039-24048: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the affected section with line numbers. sed -n '24030,24055p' ATTRIBUTIONS-Rust.md # Check whether the file uses square-bracket placeholders elsewhere in Apache text. rg -n '\{yyyy\}|\{name of copyright owner\}|\[yyyy\]|\[name of copyright owner\]' ATTRIBUTIONS-Rust.md # Find nearby Apache license text blocks to compare formatting. rg -n 'Apache License|Copyright \{yyyy\}|Copyright \[yyyy\]' ATTRIBUTIONS-Rust.md | head -n 40Repository: NVIDIA/NeMo-Relay
Length of output: 17841
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect only the changed hunks for the affected Apache appendix block. git diff --unified=0 -- ATTRIBUTIONS-Rust.md | rg -n -C 3 'Copyright \{yyyy\} \{name of copyright owner\}|\bfields enclosed by brackets\b|\[yyyy\] \[name of copyright owner\]' # Show the exact final-state lines around the reported location. sed -n '24036,24050p' ATTRIBUTIONS-Rust.mdRepository: NVIDIA/NeMo-Relay
Length of output: 155
🏁 Script executed:
#!/bin/bash set -euo pipefail # Compare a bracket-style and brace-style Apache appendix block side by side. sed -n '300,316p' ATTRIBUTIONS-Rust.md printf '\n---\n' sed -n '590,606p' ATTRIBUTIONS-Rust.mdRepository: NVIDIA/NeMo-Relay
Length of output: 3648
Restore the Apache appendix placeholders
Keep this block verbatim with[yyyy]and[name of copyright owner]; the{}placeholders diverge from the Apache 2.0 appendix text.🤖 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 `@ATTRIBUTIONS-Rust.md` around lines 24039 - 24048, Update the Apache License boilerplate in the attribution block to use the appendix’s exact “[yyyy]” and “[name of copyright owner]” placeholders, preserving the surrounding text verbatim.Source: MCP tools
crates/cli/src/diagnostics/mod.rs (1)
653-679: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Probe endpoints concurrently; serialized timeouts now scale with endpoint count.
Each iteration awaits a probe with
NETWORK_TIMEOUT. With the version-3 model an operator can configure many endpoints, sodoctorworst-case latency becomesendpoints × NETWORK_TIMEOUTinstead of the previous bounded two sections. Collecting futures and joining keeps ordering while bounding wall time to a single timeout.♻️ Suggested direction
- let mut checks = Vec::with_capacity(endpoints.len()); - for (index, endpoint) in endpoints.iter().enumerate() { + let mut probes = Vec::with_capacity(endpoints.len()); + for (index, endpoint) in endpoints.iter().enumerate() { ... - checks.push(match endpoint.get("endpoint").and_then(Value::as_str) { + probes.push(async move { /* same match, awaited inside */ }); } - checks + futures::future::join_all(probes).await📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let mut probes = Vec::with_capacity(endpoints.len()); for (index, endpoint) in endpoints.iter().enumerate() { let endpoint_type = endpoint .get("type") .and_then(Value::as_str) .unwrap_or("unknown"); let label = "OpenTelemetry endpoint"; let transport = endpoint .get("transport") .and_then(Value::as_str) .unwrap_or("http_binary"); probes.push(async move { match endpoint.get("endpoint").and_then(Value::as_str) { Some(url) => { let mut check = if transport == "grpc" { probe_tcp_named(label, url).await } else { probe_http_named(label, url).await }; check.details = format!("endpoints[{index}] ({endpoint_type}): {}", check.details); check } None => Check { name: label, status: Status::Fail, details: format!("endpoints[{index}] ({endpoint_type}): endpoint is required"), }, } }); } futures::future::join_all(probes).await🤖 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/cli/src/diagnostics/mod.rs` around lines 653 - 679, The endpoint probes in the loop are awaited serially, causing total diagnostics time to grow with the number of endpoints. Update this endpoint-checking flow to create one future per endpoint and join them concurrently, preserving the existing per-endpoint probe selection, details, failure handling, and output ordering.crates/cli/src/diagnostics/probes.rs (1)
87-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
rust url crate host_str IPv6 brackets included Url::host Host::Ipv6💡 Result:
In the Rust url crate, the behavior regarding IPv6 brackets is as follows: When you use Url::host_str, the returned string representation of an IPv6 address always includes square brackets ([ and ]) [1][2][3]. This aligns with the requirement to distinguish the colons in an IPv6 address from the port separator [1][4]. Regarding the Host enum: - The Host::Ipv6 variant holds the parsed IPv6 address as an Ipv6Addr type, not as a string [1][5]. - Therefore, the variant itself does not contain brackets [1][5]. - When you use Url::host, it returns an Option<Host<&str>> where the Ipv6 variant contains the structured Ipv6Addr [3]. If you are manually parsing a host string and need to handle brackets, the Host::parse method expects the input to be either a domain or an IPv6 address enclosed in square brackets [1][4]. When using the internal parsing logic, the brackets are consumed during the parsing of the IPv6 address [4][6].
Citations:
- 1: https://docs.rs/url/latest/url/enum.Host.html
- 2: https://docs.rs/url/1.6.1/url/struct.Url.html
- 3: https://docs.rs/url/latest/url/struct.Url.html
- 4: https://docs.rs/url/latest/src/url/host.rs.html
- 5: https://docs.rs/url/2.5.2/url/enum.Host.html
- 6: https://codebrowser.dev/rust/crates/url/src/host.rs.html
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant probe implementation and its tests. sed -n '1,180p' crates/cli/src/diagnostics/probes.rs printf '\n--- tests mentioning tcp/grpc/ipv6 ---\n' rg -n "tcp_tests|ipv6|host_str|host\(|TcpStream::connect|grpc_endpoint_port" crates/cli/src/diagnostics/probes.rsRepository: NVIDIA/NeMo-Relay
Length of output: 6555
🌐 Web query:
Rust url crate Url::host_str IPv6 returns brackets or bare address docs💡 Result:
Yes, in the Rust
urlcrate, theUrl::host_str()method returns the IPv6 address enclosed within square brackets ([and]) [1][2][3]. According to the official documentation and source code, for anIpv6host,Url::host_str()provides the serialized address in the format specified by RFC 5952, which includes enclosing the address in brackets [1][4][5]. This behavior is consistent across versions of the crate [2][6].Citations:
- 1: https://docs.rs/url/latest/src/url/host.rs.html
- 2: https://docs.rs/url/latest/url/struct.Url.html
- 3: https://codebrowser.dev/rust/crates/url/src/host.rs.html
- 4: https://docs.rs/url/latest/url/enum.Host.html
- 5: https://docs.rs/url/2.5.2/url/enum.Host.html
- 6: https://docs.rs/url/1.6.1/url/struct.Url.html
🌐 Web query:
tokio TcpStream::connect bracketed IPv6 hostname resolve fail ToSocketAddrs💡 Result:
In Rust and the Tokio runtime, the
ToSocketAddrstrait is used to resolve various address formats intoSocketAddrinstances, whichTcpStream::connectthen uses to establish connections [1][2][3]. The issue where a bracketed IPv6 address (e.g.,"[::1]:8080") might appear to fail or cause unexpected behavior usually stems from how specific libraries or downstream components handle string parsing, rather than a failure in Tokio's coreToSocketAddrsimplementation itself [4]. Key points regarding IPv6 bracketed hostnames andToSocketAddrs: 1. Parsing andToSocketAddrs: The standard library'sToSocketAddrsimplementation for&stris designed to handle strings by either parsing them as aSocketAddr(which correctly supports the[ipv6]:portformat) or by performing a DNS lookup [3][4]. If the string is a valid bracketed IPv6 address and port, it should resolve correctly as a local address [3][5]. 2. Downstream library issues: Many reported failures involving bracketed IPv6 addresses are not rooted intokio::net::TcpStream::connect, but in libraries that consume these strings (e.g.,rustlsortonic) [6][7]. These libraries often parse the host component of a URL separately. If they expect a raw IP address (without brackets) but receive a URI-formatted host (with brackets), they will fail to validate the address [6][7]. 3. Troubleshooting tips: - Verify if the error is coming fromTokio's connection attempt or a higher-level parsing step (like TLS configuration or URI parsing) [7]. - If you are building a URI or manually constructing a string, ensure that the consumer of that string expects the bracketed format. - If you need to manually resolve a host to avoid ambiguous errors fromToSocketAddrs(which returns multiple addresses and can hide specific connection errors), usetokio::net::lookup_hostto inspect individual addresses before attempting to connect [8][2][4]. IfTcpStream::connectis returning an error, check the specific I/O error returned. BecauseToSocketAddrscan return multiple addresses, if all connection attempts fail,TcpStream::connectreturns the error from the last attempted address, which might not be the primary or expected address, potentially leading to misleading error messages [9][8].Citations:
- 1: https://docs.rs/tokio/latest/tokio/net/struct.TcpStream.html
- 2: https://docs.rs/tokio/latest/tokio/net/trait.ToSocketAddrs.html
- 3: https://doc.rust-lang.org/stable/std/net/trait.ToSocketAddrs.html
- 4: https://github.com/tokio-rs/tokio/blob/c637f6e7/tokio/src/net/addr.rs
- 5: https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html
- 6: snapview/tokio-tungstenite#280
- 7: grpc/grpc-rust#2422
- 8: tokio-rs/tokio#7708
- 9: https://github.com/tokio-rs/tokio/blob/bc8dcdeb58dfca234d85b2a93a7a20af738f48a2/tokio/src/net/tcp/stream.rs
🌐 Web query:
Rust ToSocketAddrs bracketed IPv6 string "[::1]" hostname resolution💡 Result:
In Rust, the ToSocketAddrs trait supports bracketed IPv6 strings (e.g., "[::1]:8080") because it treats them as valid string representations of a SocketAddr [1][2]. When you pass a string to to_socket_addrs, the implementation first attempts to parse the string as a SocketAddr [3]. Because SocketAddr and SocketAddrV6 have FromStr implementations that recognize the bracketed IPv6 format—consistent with IETF RFC 5952—this parsing succeeds [2][4]. If the string is successfully parsed as a SocketAddr, the trait returns an iterator containing that specific address rather than attempting DNS hostname resolution [3]. In summary: - Bracketed IPv6 strings are supported and parsed directly as socket addresses [2][4]. - DNS resolution is only triggered if the string cannot be parsed as a valid SocketAddr (or an IpAddr with a port) [3]. - To use an IPv6 address with a port, the bracketed format [ipv6]:port is required, as the brackets are necessary to disambiguate the IPv6 colons from the port separator [2][5]. Top results: [1][3][2][4]
Citations:
- 1: https://doc.rust-lang.org/stable/std/net/trait.ToSocketAddrs.html
- 2: https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html
- 3: https://doc.rust-lang.org/src/std/net/socket_addr.rs.html
- 4: https://doc.rust-lang.org/std/net/enum.SocketAddr.html
- 5: https://blog.apnic.net/2020/06/02/ipv6-and-rust/
🌐 Web query:
std net ToSocketAddrs (&str, u16) bracketed IPv6 host "[::1]" invalid💡 Result:
In Rust, the implementation of ToSocketAddrs for (&str, u16) does not support passing a bracketed IPv6 address as the host string because it expects the host to be either a raw IP address (parsable as IpAddr) or a hostname for DNS resolution [1][2]. When you provide a bracketed IPv6 string (e.g., "[::1]") to the host parameter, the following occurs: 1. The implementation first attempts to parse the host string as an IpAddr [1][2]. Because "[::1]" includes brackets, it is not a valid IpAddr, and this parsing step fails [1][3][2]. 2. It then falls back to treating the host string as a hostname and passes it to the system's DNS resolver [1][2]. Since "[::1]" is not a valid hostname, the lookup fails [3][4]. To use an IPv6 address with ToSocketAddrs in this way, you have two primary options: - Use the raw IP address string: Pass the IPv6 address without brackets, such as ("::1", 8080) [5][6]. - Parse the address manually: If you must handle bracketed strings (often because they originate from URI parsers), strip the brackets before passing the address to the tuple [3][4]. Note that the single &str implementation of ToSocketAddrs does support the bracketed format (e.g., "[::1]:8080") because it first attempts to parse the entire string as a SocketAddr, which explicitly supports that format [5][1][6][7]. The limitation is specific to the split (&str, u16) tuple implementation [1][2].
Citations:
- 1: https://doc.rust-lang.org/stable/src/std/net/socket_addr.rs.html
- 2: https://doc.rust-lang.org/src/std/net/socket_addr.rs.html
- 3: https://stackoverflow.com/questions/71481475/resolve-host-of-httpuriuri-to-ip-addresses
- 4: vectordotdev/vector#3573
- 5: https://doc.rust-lang.org/stable/std/net/trait.ToSocketAddrs.html
- 6: https://blog.apnic.net/2020/06/02/ipv6-and-rust/
- 7: https://kosalab.dev/posts/rust-ipv6-socketaddr
🌐 Web query:
Tokio TcpStream::connect tuple host port bracketed IPv6 host string💡 Result:
When connecting to an IPv6 address using tokio::net::TcpStream::connect, the host portion of the address must be enclosed in square brackets if a port is specified [1][2]. The expected format is "[host]:port" [1][2]. For example: TcpStream::connect("[2001:db8::1]:8080").await?; This bracketed format is necessary to disambiguate the colon used in the IPv6 address from the colon used to separate the host from the port [1]. If you pass a raw IPv6 address without brackets to a function expecting a "host:port" string, parsing will fail because the multiple colons make the port identification ambiguous [3]. It is important to note that this requirement applies to the string representation of the address [1][2]. If you are working with higher-level URI parsing (such as those provided by the url crate), be aware that the host field may contain the brackets, and you should ensure the final string passed to TcpStream::connect is formatted correctly as [host]:port [3]. If you are manually constructing strings, always include the brackets around the IPv6 portion [2]. Finally, TcpStream::connect accepts any type that implements the ToSocketAddrs trait [4]. While passing a formatted string is common, you can also resolve addresses into SocketAddr objects (which include IPv6 variants) beforehand to avoid string parsing issues entirely [5][6].
Citations:
- 1: https://github.com/st-hannibal/ArkTunnel
- 2: https://oneuptime.com/blog/post/2026-03-20-ipv6-rust-tokio-async/view
- 3: transact-rs/sqlx#1739
- 4: https://docs.rs/tokio/latest/tokio/net/struct.TcpStream.html
- 5: https://docs.rs/tokio/latest/tokio/net/struct.TcpSocket.html
- 6: https://doc.rust-lang.org/nightly/std/net/struct.TcpStream.html
Strip brackets before
TcpStream::connectUrl::host_str()returns[::1], butTcpStream::connect((host, port))expects a raw socket host, so bracketed IPv6 literals are treated as hostnames and fail. UseUrl::host()or strip the brackets before connecting, and add an IPv6 case totcp_tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cli/src/diagnostics/probes.rs` around lines 87 - 98, Update the endpoint connection logic around parsed.host_str() and TcpStream::connect to pass an unbracketed IPv6 host, using Url::host() or stripping surrounding brackets before connecting. Preserve existing host validation and port handling, and add an IPv6 coverage case to tcp_tests.crates/core/src/observability/openinference.rs (1)
683-711: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hoist the OpenInference attribute keys into module-local
consts.Dropping
openinference-semantic-conventionsleaves ~20 spec-defined keys as inline literals spread acrossstart_attributes,end_attributes,push_projected_mark_attributes,common_attributes, and the newpush_*/remove_*helpers.tool.nameandtool_call.function.namealready appear in two places each, and there is no longer compile-time protection against a typo.otel_genai.rslines 27-30 already uses the const-block pattern for keys missing from the generated crate; mirroring it here keeps the spec surface auditable in one place.♻️ Sketch
+const OI_SPAN_KIND: &str = "openinference.span.kind"; +const OI_LLM_MODEL_NAME: &str = "llm.model_name"; +const OI_TOOL_NAME: &str = "tool.name"; +const OI_TOOL_FUNCTION_NAME: &str = "tool_call.function.name"; +const OI_OUTPUT_VALUE: &str = "output.value"; +const OI_OUTPUT_MIME_TYPE: &str = "output.mime_type"; +const OI_METADATA: &str = "metadata"; + fn push_projected_mark_attributes(attributes: &mut Vec<KeyValue>, event: &Event) { let mark_name = event.name().to_string(); - attributes.push(KeyValue::new("tool.name", mark_name.clone())); - attributes.push(KeyValue::new("tool_call.function.name", mark_name)); + attributes.push(KeyValue::new(OI_TOOL_NAME, mark_name.clone())); + attributes.push(KeyValue::new(OI_TOOL_FUNCTION_NAME, mark_name));Also applies to: 747-758
🤖 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/openinference.rs` around lines 683 - 711, Define module-local constants for all OpenInference attribute keys used by start_attributes, end_attributes, push_projected_mark_attributes, common_attributes, and the push/remove helpers, following the const-block pattern in otel_genai.rs. Replace every matching inline key literal, including the duplicated tool.name and tool_call.function.name keys, with the shared constants while preserving the existing attribute values and behavior.crates/core/src/observability/otel_genai.rs (2)
307-312: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The bare
"description"fallback can pick up an unrelated nested value.
object_value(lines 475-482) falls back into nestedusage/request/responseobjects, so an un-namespaced key likedescriptionwill match, for example,response.descriptionon an error payload and emit it asgen_ai.tool.description. Every other lookup in this module uses a namespaced or clearly scoped key. Drop the generic fallback or restrict it to the top-level object.🔧 Proposed fix
if let Some(value) = scalar_string( event, - &["gen_ai.tool.description", "tool_description", "description"], + &["gen_ai.tool.description", "tool_description"], ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if let Some(value) = scalar_string( event, &["gen_ai.tool.description", "tool_description"], ) { attributes.push(KeyValue::new("gen_ai.tool.description", value)); }🤖 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/otel_genai.rs` around lines 307 - 312, Update the tool description lookup in the event attribute handling to remove the generic "description" fallback or constrain it to the top-level event object. Preserve the existing namespaced and tool_description lookups, and ensure nested usage/request/response values cannot populate gen_ai.tool.description.
404-417: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Error detection here is case-insensitive but span status derivation is not — align them.
Line 410 uses
eq_ignore_ascii_case("ERROR"), whileset_span_status_from_event_metadataincrates/core/src/observability/mod.rs(lines 559-578) matches the literal"ERROR"and logsinvalid_status_codeotherwise. Metadata carrying"error"therefore emitserror.typeon the GenAI span while the span status staysUnsetand a warning is logged. Pick one comparison policy so status and error attribute cannot disagree.🔧 Proposed fix
- .is_some_and(|status| status.eq_ignore_ascii_case("ERROR")); + .is_some_and(|status| status == "ERROR");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn push_error_attributes(attributes: &mut Vec<KeyValue>, event: &Event) { let is_error = event .metadata() .and_then(Value::as_object) .and_then(|metadata| metadata.get("otel.status_code")) .and_then(Value::as_str) .is_some_and(|status| status == "ERROR"); if !is_error { return; } let error_type = scalar_string(event, &[semconv::ERROR_TYPE, "error_type"]) .unwrap_or_else(|| "_OTHER".to_string()); attributes.push(KeyValue::new(semconv::ERROR_TYPE, error_type)); }🤖 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/otel_genai.rs` around lines 404 - 417, Align the status-code comparison in push_error_attributes and set_span_status_from_event_metadata so both use the same case-sensitivity policy. Preserve consistent handling of metadata values such as "error": the span status and error.type attribute must agree, and valid case variants must not trigger invalid_status_code warnings.crates/core/src/observability/otel.rs (1)
562-603: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No handoff with the owned exporter runtime thread is time-bounded. The new owned-runtime design coordinates with a dedicated OS thread at two points — readiness on construction and teardown on drop — and neither has a deadline, so a wedged exporter (unreachable collector, stalled TLS root loading, blocked flush) hangs the caller indefinitely in both directions.
crates/core/src/observability/otel.rs#L562-L603: replace the unboundedresult_receiver.recv()withrecv_timeoutderived fromconfig.timeout, and document thatnew()blocks the calling thread (the tests call it from insideblock_on).crates/core/src/observability/otel.rs#L324-L339: bound teardown by callingruntime.shutdown_timeout(...)inside the spawned closure rather than relying onRuntime::dropbeforethread.join(), so subscriber drop at process teardown cannot block forever.📍 Affects 1 file
crates/core/src/observability/otel.rs#L562-L603(this comment)crates/core/src/observability/otel.rs#L324-L339🤖 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/otel.rs` around lines 562 - 603, Bound both owned exporter runtime handoffs: in crates/core/src/observability/otel.rs lines 562-603, update build_owned_tracer_provider to use result_receiver.recv_timeout with a duration derived from config.timeout, and document that construction blocks the calling thread; in lines 324-339, call Runtime::shutdown_timeout inside the spawned runtime closure with the configured timeout so teardown cannot wait indefinitely before thread joining.crates/core/src/observability/plugin_component.rs (2)
1762-1775: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Include the endpoint index in these two errors.
Every other failure in this function is prefixed with
endpoints[{index}], but the blank-endpoint and bad-transport messages are not, so with a fan-out configuration the operator cannot tell which entry failed.indexis already in scope.♻️ Proposed change
if section.endpoint.trim().is_empty() { return Err(PluginError::InvalidConfig( - "OpenTelemetry endpoint must be a nonblank string".to_string(), + format!("OpenTelemetry endpoints[{index}].endpoint must be a nonblank string"), )); } let transport = match section.transport.as_str() { "http_binary" => OtlpTransport::HttpBinary, "grpc" => OtlpTransport::Grpc, other => { return Err(PluginError::InvalidConfig(format!( - "OpenTelemetry transport must be 'http_binary' or 'grpc', got {other:?}" + "OpenTelemetry endpoints[{index}].transport must be 'http_binary' or 'grpc', got {other:?}" ))); } };
initialization_fails_for_invalid_enabled_file_exportersasserts on the substring"OpenTelemetry transport", so update that assertion accordingly.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if section.endpoint.trim().is_empty() { return Err(PluginError::InvalidConfig( format!("OpenTelemetry endpoints[{index}].endpoint must be a nonblank string"), )); } let transport = match section.transport.as_str() { "http_binary" => OtlpTransport::HttpBinary, "grpc" => OtlpTransport::Grpc, other => { return Err(PluginError::InvalidConfig(format!( "OpenTelemetry endpoints[{index}].transport must be 'http_binary' or 'grpc', got {other:?}" ))); } };🤖 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 1762 - 1775, Update the blank-endpoint and invalid-transport errors in the OpenTelemetry endpoint validation to prefix messages with the in-scope endpoint index using the existing `endpoints[{index}]` format. Also update the `initialization_fails_for_invalid_enabled_file_exporters` assertion to match the new indexed transport error while preserving its existing validation intent.
2032-2048: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Removed fields get an "unknown field" message, hiding the migration path.
Fields in
REMOVEDare correctly escalated toErrorwith codeobservability.legacy_opentelemetry_field, but the message text is the sharedunknown OpenTelemetry endpoint field {field:?}. For a breaking version-3 migration this is the one place that should tell the operator the control was removed, matching the wording used for the legacyopeninferencesection at Line 1895.♻️ Proposed change
- let behavior = if REMOVED.contains(&field.as_str()) { - UnsupportedBehavior::Error - } else { - policy.unknown_field - }; + let removed = REMOVED.contains(&field.as_str()); + let (behavior, code, message) = if removed { + ( + UnsupportedBehavior::Error, + "observability.legacy_opentelemetry_field", + format!( + "OpenTelemetry endpoint field {field:?} was removed in observability config version 3" + ), + ) + } else { + ( + policy.unknown_field, + "observability.unknown_field", + format!("unknown OpenTelemetry endpoint field {field:?}"), + ) + }; push_policy_diag( diagnostics, behavior, - if REMOVED.contains(&field.as_str()) { - "observability.legacy_opentelemetry_field" - } else { - "observability.unknown_field" - }, + code, Some("opentelemetry".to_string()), Some(format!("endpoints[{index}].{field}")), - format!("unknown OpenTelemetry endpoint field {field:?}"), + message, );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let removed = REMOVED.contains(&field.as_str()); let (behavior, code, message) = if removed { ( UnsupportedBehavior::Error, "observability.legacy_opentelemetry_field", format!( "OpenTelemetry endpoint field {field:?} was removed in observability config version 3" ), ) } else { ( policy.unknown_field, "observability.unknown_field", format!("unknown OpenTelemetry endpoint field {field:?}"), ) }; push_policy_diag( diagnostics, behavior, code, Some("opentelemetry".to_string()), Some(format!("endpoints[{index}].{field}")), message, );🤖 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 2032 - 2048, Update the diagnostic message passed to push_policy_diag in the REMOVED-field branch so removed OpenTelemetry fields explicitly state that the control was removed and provide the version-3 migration guidance, matching the legacy openinference wording near line 1895; retain the existing unknown-field message for non-REMOVED fields.crates/core/tests/integration/middleware_tests.rs (1)
848-862: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This test lost both its tool-projection and its OpenInference-specific coverage.
from_tracer_providerresolves toMarkProjection::default()(Inherit), so the OTel exporter no longer exercisesMarkProjection::Toolin this test despite the name..._pending_marks_project_through_trace_exporters_only. The new assertions at lines 947-948 and 979-980 are now byte-identical, so nothing distinguishes the OpenInference projection from thefullprojection here — the previousopeninference.span.kindcheck was the only exporter-specific signal.The assertions themselves are correct (the mark arrives after the tool span ends, so
process_marktakes the orphan branch whileparent_contextstill resolves throughcompleted_span_contexts). The gap is coverage: add back anopeninference.span.kind == "CHAIN"assertion on the OpenInference span, and keep aMarkProjection::Toolvariant if that path still matters at the integration level.As per path instructions, tests should "prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests."
Also applies to: 979-980
🤖 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/tests/integration/middleware_tests.rs` around lines 848 - 862, Restore explicit tool-projection coverage in the OTel setup around from_tracer_provider by constructing the subscriber with MarkProjection::Tool where required, while preserving the existing pending-mark assertions. Add an OpenInference-specific assertion for openinference.span.kind equal to "CHAIN" on the OpenInference span, so its exporter-specific projection remains distinguished from the full projection.Source: Path instructions
crates/core/tests/unit/observability/openinference_tests.rs (3)
684-700: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
These builder tests assert nothing about what they claim to verify.
config_defaults_and_builder_overrides_are_appliedsets seven builder options and then only checks that construction, flush, and shutdown do not error — no override is ever read back.otel.rsline 246 adds a#[cfg(test)] header()accessor for exactly this purpose and it is unused here.grpc_metadata_and_runtime_builder_paths_succeedgoes further and discards both results with.ok()(lines 3797-3798), so it can only fail ifnew()errors.All three also now spin up a real Tokio runtime, OS thread, and batch OTLP exporter aimed at a port with no listener, so they pay real setup cost for near-zero signal. Assert on the configured values, and use
unwrap()where the operation is expected to succeed.As per path instructions, tests should "cover the behavior promised by the changed API surface, including error paths" and prefer real assertions "over shallow smoke tests."
💚 Sketch for the defaults test
.with_instrumentation_scope("demo-scope") .with_timeout(Duration::from_millis(1250)); + assert_eq!(config.header("authorization"), Some("Bearer token")); let subscriber = crate::observability::otel::OpenTelemetrySubscriber::new(config).expect("valid config");Also applies to: 3763-3800
🤖 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/tests/unit/observability/openinference_tests.rs` around lines 684 - 700, Strengthen config_defaults_and_builder_overrides_are_applied by reading back and asserting every configured builder value, including the header via OpenTelemetryConfig::header(), rather than only checking construction and lifecycle calls. Update grpc_metadata_and_runtime_builder_paths_succeed similarly to assert returned results and use unwrap() for expected success instead of discarding them with ok(). Avoid starting real runtimes and exporters for these unit tests where possible, while preserving coverage of the promised configuration and error behavior.Source: Path instructions
716-734: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
The Tokio runtime here is now dead scaffolding.
OpenTelemetrySubscriber::newowns its exporter runtime — that is precisely whatgrpc_config_owns_its_tokio_runtime(lines 703-714) asserts. Wrapping this call inblock_onno longer tests anything and instead exercises the blocking-in-async pattern flagged onbuild_owned_tracer_provider. Callnew()directly.🤖 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/tests/unit/observability/openinference_tests.rs` around lines 716 - 734, Update invalid_grpc_headers_are_rejected to call OpenTelemetrySubscriber::new directly instead of creating a Tokio runtime and wrapping the call in runtime.block_on; preserve the existing invalid-header error assertion and configuration unchanged.
3210-3214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use
COMPLETED_SPAN_CONTEXT_LIMITinstead of hardcoding4096.This file compiles inside the crate, so the constant at
crates/core/src/observability/otel.rsline 52 is reachable. Hardcoding means raising the limit turns this into a confusing failure at line 3233 rather than a test that still measures eviction.♻️ Proposed change
- let span_count = 4096 + 2; + let span_count = crate::observability::otel::COMPLETED_SPAN_CONTEXT_LIMIT + 2;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let mut processor = crate::observability::otel::OtelEventProcessor::new_openinference( provider.clone(), "test-scope".to_string(), ); let span_count = crate::observability::otel::COMPLETED_SPAN_CONTEXT_LIMIT + 2;🤖 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/tests/unit/observability/openinference_tests.rs` around lines 3210 - 3214, Replace the hardcoded 4096 in the span eviction test’s span_count calculation with the reachable COMPLETED_SPAN_CONTEXT_LIMIT constant, preserving the existing +2 setup so the test continues to exercise eviction when the configured limit changes.crates/core/tests/unit/observability/otel_tests.rs (2)
585-593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the owned runtime is actually used, not just that construction succeeds.
The test only proves
newdoesn't panic without an ambient runtime.shutdown()alone may not drive an export path. Adding aforce_flush()(which must dispatch onto the owned runtime) makes this test actually cover the contract its name claims.♻️ Exercise the owned runtime before shutdown
.expect("gRPC construction should not require an ambient Tokio runtime"); + subscriber.force_flush().unwrap(); subscriber.shutdown().unwrap();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn grpc_config_owns_its_tokio_runtime() { let subscriber = OpenTelemetrySubscriber::new( OpenTelemetryConfig::new(OpenTelemetryType::Full, "http://localhost:4317") .with_transport(OtlpTransport::Grpc) .with_service_name("demo-agent"), ) .expect("gRPC construction should not require an ambient Tokio runtime"); subscriber.force_flush().unwrap(); subscriber.shutdown().unwrap(); }🤖 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/tests/unit/observability/otel_tests.rs` around lines 585 - 593, Update grpc_config_owns_its_tokio_runtime to call subscriber.force_flush() before shutdown, asserting the result succeeds so the test exercises dispatch through the subscriber’s owned Tokio runtime rather than only validating construction.
641-672: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This test can pass while asserting nothing.
status.success()is also true when the filter matches zero tests — libtest exits 0 on "0 passed; 0 failed". The hardcoded pathobservability::otel::tests::direct_config_rejects_process_global_otel_headers(Line 660) is a stringly-typed copy of the module layout; any module rename or relocation of thisinclude!d test file silently turns both child runs into no-ops, and theGlobalHeaderEnvironmentUnsupportedassertion never executes.Capture the child's output and require evidence it actually ran the case.
🛡️ Require proof the child executed the assertion
- let status = std::process::Command::new(std::env::current_exe().unwrap()) + let output = std::process::Command::new(std::env::current_exe().unwrap()) .arg("--exact") .arg("observability::otel::tests::direct_config_rejects_process_global_otel_headers") .env(CHILD_MARKER, variable) .env(variable, "authorization=secret") .env_remove(if variable == "OTEL_EXPORTER_OTLP_HEADERS" { "OTEL_EXPORTER_OTLP_TRACES_HEADERS" } else { "OTEL_EXPORTER_OTLP_HEADERS" }) - .status() + .output() .unwrap(); - assert!(status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(output.status.success(), "child failed for {variable}: {stdout}"); + assert!( + stdout.contains("1 passed"), + "child did not run the filtered test for {variable}: {stdout}" + );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.#[test] fn direct_config_rejects_process_global_otel_headers() { const CHILD_MARKER: &str = "NEMO_RELAY_TEST_GLOBAL_OTEL_HEADER_CHILD"; if let Ok(variable) = std::env::var(CHILD_MARKER) { assert!(matches!( reject_global_header_environment(), Err(OpenTelemetryError::GlobalHeaderEnvironmentUnsupported { variable: rejected }) if rejected == variable )); return; } for variable in [ "OTEL_EXPORTER_OTLP_HEADERS", "OTEL_EXPORTER_OTLP_TRACES_HEADERS", ] { let output = std::process::Command::new(std::env::current_exe().unwrap()) .arg("--exact") .arg("observability::otel::tests::direct_config_rejects_process_global_otel_headers") .env(CHILD_MARKER, variable) .env(variable, "authorization=secret") .env_remove(if variable == "OTEL_EXPORTER_OTLP_HEADERS" { "OTEL_EXPORTER_OTLP_TRACES_HEADERS" } else { "OTEL_EXPORTER_OTLP_HEADERS" }) .output() .unwrap(); let stdout = String::from_utf8_lossy(&output.stdout); assert!(output.status.success(), "child failed for {variable}: {stdout}"); assert!( stdout.contains("1 passed"), "child did not run the filtered test for {variable}: {stdout}" ); } }🤖 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/tests/unit/observability/otel_tests.rs` around lines 641 - 672, Update direct_config_rejects_process_global_otel_headers so each child process captures its output and verifies the targeted test actually ran, such as requiring the libtest summary to report one test passed, before accepting status.success(). Preserve the existing error assertion and environment-variable loop, while ensuring a renamed or relocated test cannot silently pass with zero tests executed.crates/core/tests/unit/observability/plugin_component_tests.rs (1)
166-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Harden the capture server: the
content-lengthunwrap and single-byte read loop make this fan-out suite flake-prone.If the exporter ever sends a chunked body or the client hangs up before the response is written, one of the
unwraps panics in the spawned thread, the channel disconnects, and the three new fan-out tests fail after a 10-secondrecv_timeoutwith a misleading message instead of the real cause.crates/ffi/tests/unit/api/registry_tests.rs::start_otlp_http_collectoralready implements a buffered, tolerant version of exactly this helper — reuse that shape here (buffered reads, explicit assertion message whencontent-lengthis absent, send the body before writing the response) rather than a second byte-at-a-time implementation.🤖 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/tests/unit/observability/plugin_component_tests.rs` around lines 166 - 201, Harden start_otlp_capture_server by matching the buffered, tolerant approach used by start_otlp_http_collector: replace the single-byte header loop with buffered reads, handle chunked or missing content-length without an unconditional unwrap, and provide an explicit assertion message when the header is absent. Send the captured body before writing the response, while preserving the existing endpoint and channel behavior.crates/core/tests/unit/plugin_tests.rs (1)
497-499: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep non-observability lists replacement-based.
alphais not anobservabilitycomponent, so itsconfig.listis replaced by the overlay undermerge_plugin_config_value; this test currently expects the observability-only concatenation behavior and will fail. Expectjson!([9])and qualify the comment accordingly.Proposed fix
- // nested config objects, concatenates config lists, appends overlay-only kinds, preserves + // nested config objects, replaces non-observability config lists, appends overlay-only kinds, preserves ... - json!([9, 1, 2, 3]), - "config lists concatenate with higher-precedence entries first" + json!([9]), + "non-observability config lists are replaced by higher-precedence entries"Also applies to: 555-557
🤖 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/tests/unit/plugin_tests.rs` around lines 497 - 499, Update the assertions for the non-observability component alpha in the merge_plugin_config_value tests to expect its overlay config.list as json!([9]) rather than concatenating lists; adjust the nearby test comment to state that list concatenation applies only to observability components while non-observability lists are replaced.docs/build-plugins/language-binding/code-examples.mdx (1)
183-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Flush Subscriber Delivery Before Clearing the Plugin
The examples promise event logging but clear the plugin without flushing queued native subscriber callbacks. Add the documented flush operation before
plugin.clear()in both examples so terminal events are not lost.As per coding guidelines, “Examples and documentation must use each exporter's documented flush/deregister order before shutdown.”
🤖 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 `@docs/build-plugins/language-binding/code-examples.mdx` around lines 183 - 187, Update both Python and Node.js examples to invoke each exporter’s documented flush operation before calling plugin.clear(). Preserve the existing shutdown sequence otherwise, ensuring queued native subscriber callbacks are delivered before the plugin is cleared.Source: Coding guidelines
docs/resources/troubleshooting/trace-incident-runbook.mdx (1)
152-156: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the
nemo_relay.*attribute check to non-gen_aiendpoints.This section now admits
gen_aiendpoints, but the following bullet (Lines 158-159) still tells readers to expect spans carryingnemo_relay.uuidandnemo_relay.parent_uuid. Thegen_aiprojection deliberately omitsnemo_relay.*attributes, so an operator debugging agen_aiendpoint would treat correct behavior as a failure.📝 Proposed clarification
- The exporter is enabled in the active configuration file or plugin document. -- The backend receives spans with `nemo_relay.uuid` and - `nemo_relay.parent_uuid` attributes. +- For `full` and `openinference` endpoints, the backend receives spans with + `nemo_relay.uuid` and `nemo_relay.parent_uuid` attributes. The `gen_ai` + projection omits `nemo_relay.*` attributes by design.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.For typed OpenTelemetry export, confirm these settings: - Each endpoint has the intended `full`, `gen_ai`, or `openinference` type. - The OpenTelemetry Protocol (OTLP) endpoint, headers, `header_env` variables, credentials, and network egress are available in the target environment. - The exporter is enabled in the active configuration file or plugin document. - For `full` and `openinference` endpoints, the backend receives spans with `nemo_relay.uuid` and `nemo_relay.parent_uuid` attributes. The `gen_ai` projection omits `nemo_relay.*` attributes by design.🤖 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 `@docs/resources/troubleshooting/trace-incident-runbook.mdx` around lines 152 - 156, Update the troubleshooting guidance around the typed OpenTelemetry export settings and the following attribute-check bullet so `nemo_relay.uuid` and `nemo_relay.parent_uuid` are expected only for `full` and `openinference` endpoints, not `gen_ai` endpoints; preserve the existing validation guidance for non-`gen_ai` projections.examples/switchyard/hermes-ollama-plugins.toml (1)
57-59: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the Duplicated TOML Table Headers. The first duplicate makes the TOML invalid; the second creates an empty endpoint that fails endpoint validation.
examples/switchyard/hermes-ollama-plugins.toml#L57-L59: retain exactly one[components.config]declaration.examples/switchyard/hermes-ollama-plugins.toml#L91-L100: retain exactly one[[components.config.opentelemetry.endpoints]]declaration before its endpoint fields.📍 Affects 1 file
examples/switchyard/hermes-ollama-plugins.toml#L57-L59(this comment)examples/switchyard/hermes-ollama-plugins.toml#L91-L100🤖 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 `@examples/switchyard/hermes-ollama-plugins.toml` around lines 57 - 59, Remove the duplicate TOML table headers in examples/switchyard/hermes-ollama-plugins.toml: retain exactly one [components.config] declaration at lines 57-59 and exactly one [[components.config.opentelemetry.endpoints]] declaration before its endpoint fields at lines 91-100.go/nemo_relay/openinference_test.go (1)
22-24: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Shared
NewOtelTestServerhandler can block and hangserver.Close(). Both new tests pass a one-slot channel to the shared helper, whose handler sends unconditionally; a second export (for example a shutdown flush from the deferredsubscriber.Close()) has no reader, so the handler blocks andhttptest.Server.Closewaits on it forever.
go/nemo_relay/openinference_test.go#L22-L24: increase the channel buffer, or change the handler inNewOtelTestServerto aselect/defaultnon-blocking send.go/nemo_relay/otel_test.go#L205-L246: apply the same buffering change to therequestschannel inTestOpenTelemetrySubscriberExportsGenAIAgentProjection.📍 Affects 2 files
go/nemo_relay/openinference_test.go#L22-L24(this comment)go/nemo_relay/otel_test.go#L205-L246🤖 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/openinference_test.go` around lines 22 - 24, Prevent the shared NewOtelTestServer handler from blocking during shutdown by making its request channel send non-blocking or increasing its buffer; update go/nemo_relay/openinference_test.go lines 22-24 and go/nemo_relay/otel_test.go lines 205-246 consistently, including the requests channel in TestOpenTelemetrySubscriberExportsGenAIAgentProjection.
Signed-off-by: Will Killian <wkillian@nvidia.com>
d3d3703 to
f17ad5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 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 @.agents/skills/maintain-observability/SKILL.md:
- Around line 38-42: Add focused validation guidance in the observability skill
for the v3 invariants: verify OpenTelemetry endpoints reject missing or blank
types and destinations, resolve header_env values at activation, and reject
missing, blank, or duplicate headers; also validate that layered ATOF sinks,
ATIF storage, and OpenTelemetry endpoint lists concatenate with
higher-precedence entries first. Name concrete tests or commands for each
scenario before editing.
In `@ATTRIBUTIONS-Rust.md`:
- Around line 2911-2915: Update the attribution generator that emits repository
license blocks to insert blank lines around headings and fenced code blocks, and
add the text language identifier to license fences. Then regenerate
ATTRIBUTIONS-Rust.md so all affected entries consistently satisfy MD022, MD031,
and MD040; do not patch individual blocks manually.
- Around line 3006-3043: Update the three aws-lc-sys - 0.43.0 headings in
ATTRIBUTIONS-Rust.md to include their corresponding license identifiers
(BSD-3-Clause, ISC, and MIT), ensuring each Markdown heading and anchor is
unique while preserving all three separate license text sections.
In `@crates/cli/src/diagnostics/mod.rs`:
- Around line 641-681: Update observability_http_exporter_checks to probe all
configured endpoints concurrently instead of awaiting each probe sequentially.
Preserve endpoint ordering, per-endpoint transport selection and detail
formatting, and confirm an existing futures dependency or use an available tokio
JoinSet-based approach rather than adding an unnecessary dependency.
In `@crates/core/src/observability/otel_genai.rs`:
- Around line 24-30: Update the OpenTelemetry GenAI attribute definitions to
reuse generated semconv constants for GEN_AI_OPERATION_NAME,
GEN_AI_REQUEST_MODEL, GEN_AI_TOOL_NAME, and GEN_AI_USAGE_INPUT_TOKENS wherever
those keys are used. Keep local literals only for the existing missing-key
constants such as GEN_AI_PROVIDER_NAME, GEN_AI_RETRIEVAL_TOP_K,
GEN_AI_USAGE_CACHE_CREATION_INPUT_TOKENS, and
GEN_AI_USAGE_CACHE_READ_INPUT_TOKENS.
In `@crates/core/src/observability/otel.rs`:
- Around line 321-339: Add an explicit comment on the field order in Inner,
documenting that processor must remain before _runtime so the SdkTracerProvider
is dropped before ExporterRuntime joins and tears down its Tokio runtime. Keep
the existing drop behavior unchanged and ensure the comment clearly warns
against reordering these fields.
- Around line 321-339: Tie ExporterRuntime teardown to the SdkTracerProvider
lifetime instead of relying on Inner field-drop order: update the
processor/provider ownership or explicitly drop the provider before signalling
stop and joining the thread in ExporterRuntime::drop. In
crates/core/src/observability/plugin_component.rs lines 940-1021, retain the
OpenTelemetrySubscriber Arcs alongside the fan-out callbacks, or ensure shutdown
deregisters subscribers before executing the registration closure; address both
sites so provider-dependent callbacks cannot outlive the exporter runtime.
- Around line 873-882: Invalidate any prior suppressed parent context when a
scope UUID is reused: add or use a remove_suppressed_parent_context helper that
removes the UUID from both suppressed_parent_contexts and
suppressed_parent_order, and call it from process_start alongside
remove_completed_span_context before recording new suppression. Apply the same
cleanup at the corresponding suppressed-context handling path near the other
referenced location.
In `@crates/core/src/observability/plugin_component.rs`:
- Around line 2024-2050: Update the endpoint iteration in the OpenTelemetry
diagnostics logic so enumeration occurs before filtering non-object entries.
Preserve the existing field validation and diagnostic behavior while ensuring
each reported path uses the original array index, including when invalid entries
precede valid endpoint objects.
In `@crates/core/src/plugin.rs`:
- Around line 1264-1306: Update plugin_config_list_concatenates to remove the
blanket path.len() == 1 condition and allow concatenation only for explicitly
named schema fields. Preserve the existing observability-specific allowlist for
atof.sinks, opentelemetry.endpoints, and atif.storage; all other plugin
configuration arrays must replace rather than concatenate.
In `@crates/core/tests/integration/middleware_tests.rs`:
- Around line 978-981: Update the OpenInference assertion in the integration
test to also verify the emitted openinference.span.kind attribute has the
expected "CHAIN" value, while retaining the existing nemo_relay.mark.orphan
assertion so the projection-specific behavior remains covered.
In `@crates/core/tests/unit/observability/openinference_tests.rs`:
- Around line 3208-3214: Expose COMPLETED_SPAN_CONTEXT_LIMIT as pub(super) in
the observability OTEL implementation, then update
completed_span_context_cache_evicts_oldest_parent_contexts to derive span_count
from that constant with the extra boundary entries, removing the hardcoded 4096
value.
In `@crates/core/tests/unit/observability/otel_tests.rs`:
- Around line 641-672: The child-process verification in
direct_config_rejects_process_global_otel_headers is insufficient because
status.success() does not confirm the test filter matched. Capture the child
command’s output and assert the test harness reports exactly one test executed
for each environment-variable case, while preserving the existing success
assertion and rejection checks.
In `@crates/ffi/tests/unit/api/registry_tests.rs`:
- Around line 14-62: Update start_otlp_http_collector to avoid an unbounded
listener.accept and single-connection assumption: configure the listener for
nonblocking or timed polling, enforce a finite deadline, and continue accepting
and draining additional exporter connections (including both flushes) until the
expected requests are handled or the deadline expires. Ensure the collector
thread always exits so collector.join cannot hang the suite.
In `@crates/node/observability.d.ts`:
- Line 72: Update the transport property in the observability type declaration
to remove the broad string member, leaving only the supported 'http_binary' and
'grpc' literals so invalid transport names are rejected by type checking.
In `@crates/node/src/api/mod.rs`:
- Around line 171-189: Update build_otel_config to validate options.endpoint
before constructing OpenTelemetryConfig: trim it, reject empty or
whitespace-only values with a napi error, and pass the validated endpoint
onward. Preserve the existing type, transport, and default-option handling.
In `@crates/python/src/py_types/observability.rs`:
- Around line 417-423: The Python native configuration currently exposes the
discriminator as otel_type instead of type. In
crates/python/src/py_types/observability.rs lines 417-423, keep the Rust field
named otel_type but export it with the PyO3 name type; in
python/nemo_relay/_native.pyi line 1016, rename the declared attribute to type
while leaving the positional __init__ parameter as otel_type.
In `@docs/about-nemo-relay/release-notes/index.mdx`:
- Around line 87-89: Update the release note describing opentelemetry.endpoints
header_env validation to explicitly state that activation rejects missing
environment variables, blank values, and duplicate headers, making endpoint
activation failures actionable and aligned with the v3 contract.
In `@docs/configure-plugins/observability/configuration.mdx`:
- Around line 136-153: Update the Node.js observability example to use the
public helper names and fields: replace observability.openTelemetryConfig with
observability.otlpConfig, replace observability.openTelemetryEndpoint with the
corresponding OTLP endpoint helper, and preserve the endpoint and service_name
values.
In `@docs/configure-plugins/observability/opentelemetry.mdx`:
- Around line 33-36: Update the “plugins.toml Example” section by adding a
complete lead-in sentence between the heading and the fenced TOML block,
ensuring the code block does not directly follow the heading or complete the
sentence’s grammar.
In `@go/nemo_relay/coverage_gap_test.go`:
- Around line 154-158: Update assertZeroSubscriberConfigs to set
OpenTelemetryConfig.Type to a valid projection such as "full" while leaving
Endpoint empty, then assert that NewOpenTelemetrySubscriber returns the specific
"endpoint is required" error.
🪄 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: 8aad3487-a07e-446b-b975-467f4a688ec1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (118)
.agents/skills/maintain-observability/SKILL.mdATTRIBUTIONS-Rust.mdCargo.tomlREADME.mdabout.tomlcrates/cli/Cargo.tomlcrates/cli/README.mdcrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/probes.rscrates/cli/src/error.rscrates/cli/src/gateway/tls.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/shared/error_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/core/Cargo.tomlcrates/core/README.mdcrates/core/src/lib.rscrates/core/src/observability/manual.rscrates/core/src/observability/mod.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/otel_genai.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin.rscrates/core/tests/integration/atif_storage_tests.rscrates/core/tests/integration/middleware_tests.rscrates/core/tests/unit/observability/exporter_parity_tests.rscrates/core/tests/unit/observability/manual_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugin_tests.rscrates/ffi/Cargo.tomlcrates/ffi/README.mdcrates/ffi/nemo_relay.hcrates/ffi/src/api/mod.rscrates/ffi/src/api/observability.rscrates/ffi/src/types/mod.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/tests/unit/api/core_tests.rscrates/ffi/tests/unit/api/coverage_sweeps_tests.rscrates/ffi/tests/unit/api/plugin_tests.rscrates/ffi/tests/unit/api/registry_tests.rscrates/ffi/tests/unit/types_tests.rscrates/node/Cargo.tomlcrates/node/README.mdcrates/node/observability.d.tscrates/node/observability.jscrates/node/src/api/mod.rscrates/node/tests/observability_plugin_tests.mjscrates/node/tests/openinference_tests.mjscrates/node/tests/otel_tests.mjscrates/pii-redaction/Cargo.tomlcrates/pii-redaction/tests/unit/component_tests.rscrates/python/Cargo.tomlcrates/python/src/py_types/mod.rscrates/python/src/py_types/observability.rscrates/python/tests/coverage/py_types_coverage_tests.rsdeny.tomldocs/about-nemo-relay/agent-runtime-primer.mdxdocs/about-nemo-relay/concepts/codecs.mdxdocs/about-nemo-relay/concepts/events.mdxdocs/about-nemo-relay/concepts/plugins.mdxdocs/about-nemo-relay/concepts/subscribers.mdxdocs/about-nemo-relay/ecosystem.mdxdocs/about-nemo-relay/overview.mdxdocs/about-nemo-relay/release-notes/index.mdxdocs/build-plugins/language-binding/code-examples.mdxdocs/configure-plugins/about.mdxdocs/configure-plugins/observability/about.mdxdocs/configure-plugins/observability/atif.mdxdocs/configure-plugins/observability/atof.mdxdocs/configure-plugins/observability/configuration.mdxdocs/configure-plugins/observability/openinference.mdxdocs/configure-plugins/observability/opentelemetry.mdxdocs/configure-plugins/pii-redaction/configuration.mdxdocs/configure-plugins/plugin-configuration-files.mdxdocs/configure-plugins/switchyard/configuration.mdxdocs/contribute/runtime-contract-docs.mdxdocs/getting-started/configuration.mdxdocs/integrate-into-frameworks/provider-response-codecs.mdxdocs/nemo-relay-cli/claude-code.mdxdocs/reference/migration-guides.mdxdocs/resources/glossary.mdxdocs/resources/troubleshooting/trace-incident-runbook.mdxdocs/supported-integrations/openclaw-plugin.mdxexamples/switchyard/hermes-ollama-plugins.tomlgo/nemo_relay/README.mdgo/nemo_relay/coverage_gap_test.gogo/nemo_relay/nemo_relay.gogo/nemo_relay/observability_plugin.gogo/nemo_relay/observability_plugin_test.gogo/nemo_relay/openinference_test.gogo/nemo_relay/otel_test.gogo/nemo_relay/top_level_coverage_test.gointegrations/coding-agents/README.mdintegrations/openclaw/README.mdintegrations/openclaw/index.tsintegrations/openclaw/openclaw.plugin.jsonintegrations/openclaw/src/config.tsintegrations/openclaw/src/health.tsintegrations/openclaw/test/config.test.tsintegrations/openclaw/test/live-smoke.test.tspython/nemo_relay/README.mdpython/nemo_relay/__init__.pypython/nemo_relay/__init__.pyipython/nemo_relay/_native.pyipython/nemo_relay/observability.pypython/nemo_relay/observability.pyipython/tests/integrations/deepagents_tests/test_deepagents_integration.pypython/tests/test_observability_plugin.pypython/tests/test_types.pyscripts/licensing/attributions_lockfile_md.py
💤 Files with no reviewable changes (6)
- crates/core/src/observability/manual.rs
- crates/cli/tests/coverage/shared/error_tests.rs
- python/nemo_relay/init.pyi
- crates/ffi/tests/unit/types_tests.rs
- crates/python/src/py_types/mod.rs
- crates/core/tests/unit/observability/manual_tests.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (20)
.agents/skills/maintain-observability/SKILL.md (1)
38-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add focused validation for the new v3 invariants.
The checklist now requires endpoint/header validation and precedence-preserving list concatenation, but the validation guidance does not name checks for those paths. Add focused tests or commands for missing, blank, and duplicate
header_envheaders and layered ordering.Based on learnings, observability changes should define focused validation before editing.
🤖 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 @.agents/skills/maintain-observability/SKILL.md around lines 38 - 42, Add focused validation guidance in the observability skill for the v3 invariants: verify OpenTelemetry endpoints reject missing or blank types and destinations, resolve header_env values at activation, and reject missing, blank, or duplicate headers; also validate that layered ATOF sinks, ATIF storage, and OpenTelemetry endpoint lists concatenate with higher-precedence entries first. Name concrete tests or commands for each scenario before editing.Source: Learnings
ATTRIBUTIONS-Rust.md (2)
2911-2915: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Regenerate the attribution blocks with valid Markdown formatting.
The changed blocks repeatedly violate MD022, MD031, and MD040: headings and fences lack required blank lines, and license fences have no language identifier. Fix the generator to emit
textfences and surrounding blank lines, then regenerate this file rather than patching individual entries.Also applies to: 2926-2930, 3006-3010, 7512-7516, 9292-9296, 12263-12267, 15926-15930, 23407-23411, 23695-23699, 26116-26119, 28621-28625, 29457-29461, 29985-29989, 34266-34270, 36993-36997, 38680-38684, 39234-39238, 42311-42315, 43415-43419, 49057-49061, 53343-53347, 54904-54908, 55409-55413, 55765-55769
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 2911-2911: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 2914-2914: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above(MD022, blanks-around-headings)
[warning] 2914-2914: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 2915-2915: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 2915-2915: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@ATTRIBUTIONS-Rust.md` around lines 2911 - 2915, Update the attribution generator that emits repository license blocks to insert blank lines around headings and fenced code blocks, and add the text language identifier to license fences. Then regenerate ATTRIBUTIONS-Rust.md so all affected entries consistently satisfy MD022, MD031, and MD040; do not patch individual blocks manually.Sources: Coding guidelines, Linters/SAST tools
3006-3043: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make repeated
aws-lc-sysheadings unique.The three
aws-lc-sys - 0.43.0sections produce duplicate Markdown headings and anchors. Include the license identifier in each heading, or otherwise generate unique suffixes, while retaining the separate license texts.🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 3006-3006: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 3006-3006: Multiple headings with the same content
(MD024, no-duplicate-heading)
[warning] 3009-3009: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above(MD022, blanks-around-headings)
[warning] 3009-3009: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 3010-3010: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 3010-3010: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 3024-3024: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 3024-3024: Multiple headings with the same content
(MD024, no-duplicate-heading)
[warning] 3027-3027: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above(MD022, blanks-around-headings)
[warning] 3027-3027: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 3028-3028: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 3028-3028: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 3039-3039: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 3039-3039: Multiple headings with the same content
(MD024, no-duplicate-heading)
[warning] 3042-3042: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above(MD022, blanks-around-headings)
[warning] 3042-3042: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 3043-3043: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 3043-3043: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@ATTRIBUTIONS-Rust.md` around lines 3006 - 3043, Update the three aws-lc-sys - 0.43.0 headings in ATTRIBUTIONS-Rust.md to include their corresponding license identifiers (BSD-3-Clause, ISC, and MIT), ensuring each Markdown heading and anchor is unique while preserving all three separate license text sections.Source: Linters/SAST tools
crates/cli/src/diagnostics/mod.rs (1)
641-681: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Sequential endpoint probing scales linearly with configured endpoints.
Each endpoint's connectivity check is awaited one at a time inside the loop. Since version 3 explicitly supports repeated heterogeneous endpoints, a
doctorrun with several slow/unreachable endpoints will take N × per-probe-timeout to complete, even though these probes are independent.⚡ Proposed fix to probe endpoints concurrently
- let mut checks = Vec::with_capacity(endpoints.len()); - for (index, endpoint) in endpoints.iter().enumerate() { + let probes = endpoints.iter().enumerate().map(|(index, endpoint)| async move { let endpoint_type = endpoint .get("type") .and_then(Value::as_str) .unwrap_or("unknown"); let label = "OpenTelemetry endpoint"; let transport = endpoint .get("transport") .and_then(Value::as_str) .unwrap_or("http_binary"); - checks.push(match endpoint.get("endpoint").and_then(Value::as_str) { + match endpoint.get("endpoint").and_then(Value::as_str) { Some(url) => { let mut check = if transport == "grpc" { probe_tcp_named(label, url).await } else { probe_http_named(label, url).await }; check.details = format!("endpoints[{index}] ({endpoint_type}): {}", check.details); check } None => Check { name: label, status: Status::Fail, details: format!("endpoints[{index}] ({endpoint_type}): endpoint is required"), }, - }); - } - checks + } + }); + futures::future::join_all(probes).await }Please confirm the
futurescrate (or atokio::task::JoinSet-based alternative) is already available incrates/cli's dependency tree before adoptingjoin_all.🤖 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/cli/src/diagnostics/mod.rs` around lines 641 - 681, Update observability_http_exporter_checks to probe all configured endpoints concurrently instead of awaiting each probe sequentially. Preserve endpoint ordering, per-endpoint transport selection and detail formatting, and confirm an existing futures dependency or use an available tokio JoinSet-based approach rather than adding an unnecessary dependency.crates/core/src/observability/otel_genai.rs (1)
24-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm which gen_ai.* constants the pinned semconv crate exposes. cargo add --dry-run opentelemetry-semantic-conventions@0.32.1 2>/dev/null || true curl -s "https://docs.rs/opentelemetry-semantic-conventions/0.32.1/opentelemetry_semantic_conventions/attribute/index.html" \ | grep -o 'GEN_AI_[A-Z_]*' | sort -u | head -50Repository: NVIDIA/NeMo-Relay
Length of output: 2049
🌐 Web query:
opentelemetry-semantic-conventions 0.32.1 gen_ai constants docs.rs GEN_AI_OPERATION_NAME GEN_AI_REQUEST_MODEL GEN_AI_TOOL_NAME GEN_AI_USAGE_INPUT_TOKENS💡 Result:
In the OpenTelemetry semantic conventions, Generative AI (GenAI) attributes are defined to standardize telemetry data for AI-related operations [1][2]. The constants you inquired about correspond to the following string values, which are used to represent specific attributes in instrumentation [3][4]: - GEN_AI_OPERATION_NAME: "gen_ai.operation.name" — Represents the name of the operation being performed (e.g., chat, completion, embeddings) [3][5][6]. - GEN_AI_REQUEST_MODEL: "gen_ai.request.model" — Represents the name of the GenAI model to which the request is made [4][7]. - GEN_AI_TOOL_NAME: "gen_ai.tool.name" — This attribute is used to identify the name of a tool being executed in the context of GenAI operations [8][5]. - GEN_AI_USAGE_INPUT_TOKENS: "gen_ai.usage.input_tokens" — Used to record the number of input tokens consumed during a GenAI request [7]. These constants are part of the broader set of GenAI semantic conventions, which are currently maintained and evolved to ensure consistency across different AI providers and instrumentation libraries [1][5]. While some earlier versions of these attributes were found in language-specific repositories like opentelemetry-python, they are increasingly centralized within the dedicated OpenTelemetry GenAI semantic conventions registry [8][5]. Top results: [8] https://github.com/open-telemetry/semantic-conventions-genai/blob/main/docs/registry/attributes/gen-ai.md [1] https://github.com/open-telemetry/semantic-conventions-genai/blob/main/docs/gen-ai/gen-ai-spans.md [3] https://docs.rs/opentelemetry-semantic-conventions/latest/opentelemetry_semantic_conventions/attribute/constant.GEN_AI_OPERATION_NAME.html [5] https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-semantic-conventions/src/opentelemetry/semconv/_incubating/attributes/gen_ai_attributes.py [4] https://docs.rs/opentelemetry-semantic-conventions/latest/opentelemetry_semantic_conventions/attribute/constant.GEN_AI_REQUEST_MODEL.html [7] https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation-genai/opentelemetry-instrumentation-openai-agents-v2/src/opentelemetry/instrumentation/openai_agents/span_processor.py
Citations:
- 1: https://github.com/open-telemetry/semantic-conventions-genai/blob/main/docs/gen-ai/gen-ai-spans.md
- 2: https://github.com/open-telemetry/semantic-conventions/blob/e96d8de9/model/gen-ai/registry.yaml
- 3: https://docs.rs/opentelemetry-semantic-conventions/latest/opentelemetry_semantic_conventions/attribute/constant.GEN_AI_OPERATION_NAME.html
- 4: https://docs.rs/opentelemetry-semantic-conventions/latest/opentelemetry_semantic_conventions/attribute/constant.GEN_AI_REQUEST_MODEL.html
- 5: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-semantic-conventions/src/opentelemetry/semconv/_incubating/attributes/gen_ai_attributes.py
- 6: https://github.com/open-telemetry/opentelemetry-python/blob/eed100c3/opentelemetry-semantic-conventions/src/opentelemetry/semconv/_incubating/attributes/gen_ai_attributes.py
- 7: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation-genai/opentelemetry-instrumentation-openai-agents-v2/src/opentelemetry/instrumentation/openai_agents/span_processor.py
- 8: https://github.com/open-telemetry/semantic-conventions-genai/blob/main/docs/registry/attributes/gen-ai.md
🌐 Web query:
site:docs.rs/opentelemetry-semantic-conventions/0.32.1 opentelemetry_semantic_conventions attribute constant GEN_AI_OPERATION_NAME GEN_AI_REQUEST_MODEL GEN_AI_TOOL_NAME GEN_AI_USAGE_INPUT_TOKENS💡 Result:
In
opentelemetry-semantic-conventions0.32.1, these map to:GEN_AI_OPERATION_NAME = "gen_ai.operation.name",GEN_AI_REQUEST_MODEL = "gen_ai.request.model",GEN_AI_TOOL_NAME = "gen_ai.tool.name"(tool utilized by the agent), andGEN_AI_USAGE_INPUT_TOKENS = "gen_ai.usage.input_tokens". The GenAI attributes in this crate are marked deprecated/moved to the OpenTelemetry GenAI semantic conventions repo. (docs.rs)Citations:
Use the generated semconv constants for existing
gen_ai.*keys
opentelemetry-semantic-conventions 0.32.1already exposesGEN_AI_OPERATION_NAME,GEN_AI_REQUEST_MODEL,GEN_AI_TOOL_NAME, andGEN_AI_USAGE_INPUT_TOKENS; keep literals only for the fewgen_ai.*attributes that still lack generated symbols.🤖 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/otel_genai.rs` around lines 24 - 30, Update the OpenTelemetry GenAI attribute definitions to reuse generated semconv constants for GEN_AI_OPERATION_NAME, GEN_AI_REQUEST_MODEL, GEN_AI_TOOL_NAME, and GEN_AI_USAGE_INPUT_TOKENS wherever those keys are used. Keep local literals only for the existing missing-key constants such as GEN_AI_PROVIDER_NAME, GEN_AI_RETRIEVAL_TOP_K, GEN_AI_USAGE_CACHE_CREATION_INPUT_TOKENS, and GEN_AI_USAGE_CACHE_READ_INPUT_TOKENS.crates/core/src/observability/otel.rs (2)
321-339: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Drop order between
Inner.processorand_runtimeis load-bearing but implicit.
ExporterRuntime::dropjoins the exporter thread, which tears down the owned Tokio runtime that the batch span processor's background task lives on. This is only safe becauseInnerdeclaresprocessor(owningSdkTracerProvider) before_runtime, so the provider is dropped first. Reordering those fields would drop the runtime while the batch processor is still alive. Add a comment (or explicitly drop the provider before signalling stop) so the invariant survives refactors.🧷 Minimal guard
struct Inner { + // Declaration order matters: `processor` owns the tracer provider whose batch + // span processor runs on `_runtime`. The provider must drop before the + // runtime thread is joined. processor: Arc<Mutex<OtelEventProcessor>>, subscriber: EventSubscriberFn, _runtime: Option<ExporterRuntime>, }Also applies to: 562-603
🤖 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/otel.rs` around lines 321 - 339, Add an explicit comment on the field order in Inner, documenting that processor must remain before _runtime so the SdkTracerProvider is dropped before ExporterRuntime joins and tears down its Tokio runtime. Keep the existing drop behavior unchanged and ensure the comment clearly warns against reordering these fields.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Exporter Tokio runtime lifetime is tied to
Inner, not to the tracer provider it services.ExporterRuntimejoins the exporter thread (tearing down the runtime that hosts the batch span processor's background task) wheneverInnerdrops, but theSdkTracerProviderlives behindArc<Mutex<OtelEventProcessor>>, which callers can outliveInnerwith. Both sites depend on unwritten ordering rules to stay safe.
crates/core/src/observability/otel.rs#L321-L339: bind the runtime's lifetime to the provider (or drop the provider explicitly before signalling stop) instead of relying onInner's field declaration orderprocessor→subscriber→_runtime.crates/core/src/observability/plugin_component.rs#L940-L1021: the fan-outcallbacksclone only theEventSubscriberFn(holding the processor/provider) while the soleArc<OpenTelemetrySubscriber>owners sit in the shutdown registration closure; either hold the subscriberArcs alongside the callbacks in the registered fan-out, or confirm teardown always deregisters the subscriber before running the shutdown registration.📍 Affects 2 files
crates/core/src/observability/otel.rs#L321-L339(this comment)crates/core/src/observability/plugin_component.rs#L940-L1021🤖 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/otel.rs` around lines 321 - 339, Tie ExporterRuntime teardown to the SdkTracerProvider lifetime instead of relying on Inner field-drop order: update the processor/provider ownership or explicitly drop the provider before signalling stop and joining the thread in ExporterRuntime::drop. In crates/core/src/observability/plugin_component.rs lines 940-1021, retain the OpenTelemetrySubscriber Arcs alongside the fan-out callbacks, or ensure shutdown deregisters subscribers before executing the registration closure; address both sites so provider-dependent callbacks cannot outlive the exporter runtime.
873-882: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Suppressed parent contexts are never invalidated on scope-uuid reuse.
process_startcallsremove_completed_span_context(event.uuid())to drop a stale completed context when a uuid is reused (covered byprocess_start_removes_completed_span_order_entry), but the newsuppressed_parent_contextsmap has no equivalent invalidation. Undergen_ai, a uuid that was once suppressed keeps its old parent context forever (until the 4096-entry eviction), so a later reuse of that uuid can parent descendants to a stale span. Cheap to close alongside the existing removal.🩹 Suggested fix
fn process_start(&mut self, event: &Event) { self.remove_completed_span_context(event.uuid()); + self.remove_suppressed_parent_context(event.uuid()); let parent_context = self.parent_context(event);fn remove_suppressed_parent_context(&mut self, uuid: Uuid) { self.suppressed_parent_contexts.remove(&uuid); self.suppressed_parent_order .retain(|suppressed_uuid| *suppressed_uuid != uuid); }Also applies to: 1102-1116
🤖 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/otel.rs` around lines 873 - 882, Invalidate any prior suppressed parent context when a scope UUID is reused: add or use a remove_suppressed_parent_context helper that removes the UUID from both suppressed_parent_contexts and suppressed_parent_order, and call it from process_start alongside remove_completed_span_context before recording new suppression. Apply the same cleanup at the corresponding suppressed-context handling path near the other referenced location.crates/core/src/observability/plugin_component.rs (1)
2024-2050: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Endpoint indices in diagnostics shift when a non-object entry precedes a valid one.
enumerate()is applied afterfilter_map(Json::as_object), soendpoints: [42, { "header_en": ... }]reportsendpoints[0].header_enfor what is actuallyendpoints[1]. Enumerate first, then filter.🩹 Proposed fix
- for (index, endpoint) in endpoints.iter().filter_map(Json::as_object).enumerate() { + for (index, endpoint) in endpoints + .iter() + .enumerate() + .filter_map(|(index, endpoint)| endpoint.as_object().map(|endpoint| (index, endpoint))) + {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for (index, endpoint) in endpoints .iter() .enumerate() .filter_map(|(index, endpoint)| endpoint.as_object().map(|endpoint| (index, endpoint))) { for field in endpoint .keys() .filter(|field| !ALLOWED.contains(&field.as_str())) { let behavior = if REMOVED.contains(&field.as_str()) { UnsupportedBehavior::Error } else { policy.unknown_field }; push_policy_diag( diagnostics, behavior, if REMOVED.contains(&field.as_str()) { "observability.legacy_opentelemetry_field" } else { "observability.unknown_field" }, Some("opentelemetry".to_string()), Some(format!("endpoints[{index}].{field}")), format!("unknown OpenTelemetry endpoint field {field:?}"), ); } }🤖 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 2024 - 2050, Update the endpoint iteration in the OpenTelemetry diagnostics logic so enumeration occurs before filtering non-object entries. Preserve the existing field validation and diagnostic behavior while ensuring each reported path uses the original array index, including when invalid entries precede valid endpoint objects.crates/core/src/plugin.rs (1)
1264-1306: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find plugin configs with top-level lists whose layering semantics just changed. rg -nP --type=rust -C3 '#\[serde\(default[^)]*\)\]\s*\n\s*pub \w+: Vec<' -U crates/pii-redaction/src crates/core/src --glob '!**/tests/**' | head -80 rg -nP -C3 'sources|patterns|routes|allow(ed)?_list' crates/pii-redaction/src/component.rs | head -40Repository: NVIDIA/NeMo-Relay
Length of output: 2049
🌐 Web query:
"NVIDIA NeMo Relay plugin_config_list_concatenates path.len() == 1 atif sinks opentelemetry endpoints plugin.rs"💡 Result:
In the NVIDIA NeMo Relay project, the references to plugin configuration, OpenTelemetry (OTLP) endpoints, and Rust-based plugin handling are related to recent observability architectural updates, specifically the unification of exporters in version 3 [1]. The code pattern plugin_config_list_concatenates and the validation check path.len == 1 are part of the core observability plugin implementation in crates/core/src/observability/plugin_component.rs and crates/core/src/plugin.rs [2][3][1]. These components were updated to support typed endpoint arrays, transactional configuration construction, and the concatenation of configuration lists based on precedence [1]. Key context regarding these components includes: 1. Version 3 Configuration: The unification process replaced legacy OTLP and OpenInference sections with a structured, typed array format for multiple OpenTelemetry endpoints [1]. 2. Validation and Logic: Logic within the observability plugin ensures that provided configurations are valid. The check path.len == 1 is typically used during path resolution or configuration validation to ensure that specific configuration paths are correctly scoped or uniquely identified during the fan-out process to multiple exporters [3][1]. 3. ATIF/ATOF Integration: The observability framework handles the wiring for ATOF (Async Trace/Telemetry Output Format), ATIF (Async Trace/Telemetry Input Format), OTLP, and OpenInference exporters [3][4]. These exporters are registered and managed through the built-in observability plugin, which keeps sections disabled by default unless explicitly enabled [3]. For developers working with this codebase, the source of truth for these implementations is located in crates/core/src/observability/plugin_component.rs, which handles the multi-endpoint configuration and fan-out logic, and crates/core/src/plugin.rs, which manages the core plugin registration and lifecycle wiring [3][1]. Migration details and examples for updating to this unified configuration format are provided in the official documentation, specifically under docs/configure-plugins/observability/opentelemetry.mdx [2][1].
Citations:
🌐 Web query:
"NVIDIA NeMo Relay plugin_config_list_concatenates top-level list all plugin kinds non-observability plugin config"💡 Result:
In NVIDIA NeMo Relay, plugin configuration is defined by a top-level document containing version, components, and policy [1][2]. The components array is a critical part of this configuration, where each entry specifies a plugin kind, its enabled status, and a local configuration object [3][4][2]. Relay handles component configuration merging across multiple plugins.toml files (system, project, and user) using specific precedence rules [3][4]: - Components are matched by kind [3][4]. - A higher-precedence component with the same kind merges into the lower-precedence component [3][4]. - A component with a different kind is added to the effective configuration [3][4]. Regarding the specific term "plugin_config_list_concatenates," there is no public technical documentation or public API reference in NVIDIA NeMo Relay explicitly identifying a field or internal function by this name. It is possible this term relates to internal implementation details or private APIs not exposed in the standard configuration schema or public developer documentation. The standard schema explicitly defines components as a single array where merging is handled by matching kinds rather than simple concatenation of lists [3][4]. For plugin configuration management, operators should rely on the documented merging behavior where higher-precedence files override or merge into existing components based on their kind [3][4].
Citations:
- 1: https://docs.nvidia.com/nemo/relay/build-plugins/language-binding/about
- 2: https://docs.nvidia.com/nemo/relay/build-plugins/language-binding/validate-configuration
- 3: https://docs.nvidia.com/nemo/relay/configure-plugins/plugin-configuration-files
- 4: https://docs.nvidia.com/nemo/relay/v0.4.0/build-plugins/plugin-configuration-files
🌐 Web query:
"NVIDIA NeMo Relay plugin configuration files merging arrays override concatenate higher precedence same kind"💡 Result:
In NVIDIA NeMo Relay and NeMo-related components using configuration composition (via Hydra/OmegaConf), the handling of arrays (lists) during merging depends on the specific component's implementation, though standard behavior is to replace rather than concatenate [1][2]. 1. NVIDIA NeMo Relay Plugin Configuration For NeMo Relay plugin configuration files (plugins.toml), the default rule is that higher-precedence arrays replace lower-precedence arrays [3]. If you need to modify an array without full replacement, you must repeat the complete array entry in the higher-precedence file [3]. A specific exception exists for the top-level components array, where Relay matches components by kind; a higher-precedence component of the same kind merges into the lower-precedence one, while components of different kinds are added [3]. Additionally, the built-in pricing component allows higher-precedence sources to be placed before lower-precedence sources [3]. 2. General NeMo/Hydra Configuration Most NeMo components that rely on Hydra and OmegaConf follow standard OmegaConf merge semantics [4][5]. In this system: - Dictionaries are recursively deep-merged (later values override earlier ones) [1][2][6]. - Lists (arrays) are replaced entirely by default; they are not concatenated [1][2][7]. - Scalars are overridden by the value with higher precedence [1][2]. If you require array concatenation in a Hydra-based configuration, it is not a native merge behavior [1][7]. You must typically use workarounds such as: - Defining custom OmegaConf resolvers (e.g., a merge resolver) to concatenate lists during configuration resolution [8]. - Explicitly structuring your configuration to use dictionary keys instead of list indices if you need granular control over specific elements [7]. Always prioritize the specific documentation for the NeMo tool you are using, as some modules implement custom merge logic (e.g., recursive deep_merge helpers) that may deviate from strict standard Hydra defaults [9][10].
Citations:
- 1: https://docs.nvidia.com/nemo/evaluator/tutorials/config-composition
- 2: https://docs.nvidia.com/nemo/evaluator/0.3.0/tutorials/config-composition.html
- 3: https://docs.nvidia.com/nemo/relay/configure-plugins/plugin-configuration-files
- 4: https://docs.nvidia.com/nemo-framework/user-guide/latest/nemotoolkit/core/core.html
- 5: https://docs.nvidia.com/nemo-framework/user-guide/24.12/nemotoolkit/core/core.html
- 6: https://hydra.cc/docs/advanced/override_grammar/basic/
- 7: facebookresearch/hydra#403
- 8: https://stackoverflow.com/questions/71527303/hydra-combine-lists-in-config
- 9: https://docs.nvidia.com/nemo/agent-toolkit/latest/api/nat/utils/io/yaml_tools/index.html
- 10: https://github.com/NVIDIA-NeMo/Speech/blob/main/nemo/utils/config_utils.py
Make list concatenation opt-in per field
path.len() == 1makes every plugin-local top-level array inherit concat semantics, while plugin config arrays normally replace unless a field is explicitly special-cased. Keep this allowlist scoped to named schema fields so new top-level lists don’t silently change merge behavior.🤖 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/plugin.rs` around lines 1264 - 1306, Update plugin_config_list_concatenates to remove the blanket path.len() == 1 condition and allow concatenation only for explicitly named schema fields. Preserve the existing observability-specific allowlist for atof.sinks, opentelemetry.endpoints, and atif.storage; all other plugin configuration arrays must replace rather than concatenate.crates/core/tests/integration/middleware_tests.rs (1)
978-981: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Both projections now assert the same attribute; OpenInference-specific coverage is lost.
The OpenInference branch previously asserted
openinference.span.kind. Percrates/core/src/observability/otel.rs(around lines 996-1007), the OpenInference orphan path emits bothopeninference.span.kind = "CHAIN"andnemo_relay.mark.orphan. Keeping the kind assertion preserves the projection-difference guarantee this PR is about.♻️ Suggested addition
assert!(openinference_mark.attributes.iter().any(|attribute| { attribute.key.as_str() == "nemo_relay.mark.orphan" && attribute.value == opentelemetry::Value::Bool(true) })); + assert!(openinference_mark.attributes.iter().any(|attribute| { + attribute.key.as_str() == "openinference.span.kind" + && attribute.value.to_string() == "CHAIN" + }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.assert!(openinference_mark.attributes.iter().any(|attribute| { attribute.key.as_str() == "nemo_relay.mark.orphan" && attribute.value == opentelemetry::Value::Bool(true) })); assert!(openinference_mark.attributes.iter().any(|attribute| { attribute.key.as_str() == "openinference.span.kind" && attribute.value.to_string() == "CHAIN" }));🤖 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/tests/integration/middleware_tests.rs` around lines 978 - 981, Update the OpenInference assertion in the integration test to also verify the emitted openinference.span.kind attribute has the expected "CHAIN" value, while retaining the existing nemo_relay.mark.orphan assertion so the projection-specific behavior remains covered.crates/core/tests/unit/observability/openinference_tests.rs (1)
3208-3214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Don't hardcode the eviction limit in the test.
4096 + 2duplicatesCOMPLETED_SPAN_CONTEXT_LIMIT. If the constant changes, this test keeps passing while no longer exercising the eviction boundary. Make the constantpub(super)and reference it.♻️ Suggested change
- let span_count = 4096 + 2; + let span_count = crate::observability::otel::COMPLETED_SPAN_CONTEXT_LIMIT + 2;In
crates/core/src/observability/otel.rs:-const COMPLETED_SPAN_CONTEXT_LIMIT: usize = 4096; +pub(super) const COMPLETED_SPAN_CONTEXT_LIMIT: usize = 4096;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn completed_span_context_cache_evicts_oldest_parent_contexts() { let (provider, exporter) = make_provider(); let mut processor = crate::observability::otel::OtelEventProcessor::new_openinference( provider.clone(), "test-scope".to_string(), ); let span_count = crate::observability::otel::COMPLETED_SPAN_CONTEXT_LIMIT + 2;🤖 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/tests/unit/observability/openinference_tests.rs` around lines 3208 - 3214, Expose COMPLETED_SPAN_CONTEXT_LIMIT as pub(super) in the observability OTEL implementation, then update completed_span_context_cache_evicts_oldest_parent_contexts to derive span_count from that constant with the extra boundary entries, removing the hardcoded 4096 value.crates/core/tests/unit/observability/otel_tests.rs (1)
641-672: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm how this test file is mounted and whether the filter path is correct. rg -n 'otel_tests|unit/observability|#\[path' crates/core/src/lib.rs crates/core/src/observability/mod.rs | head -50 fd -t f 'otel_tests.rs' crates/coreRepository: NVIDIA/NeMo-Relay
Length of output: 2049
Make the child prove it ran.
status.success()can still be true when the filter matches nothing, so this test can pass without executingdirect_config_rejects_process_global_otel_headers. Capture the child output and assert exactly one test ran.🤖 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/tests/unit/observability/otel_tests.rs` around lines 641 - 672, The child-process verification in direct_config_rejects_process_global_otel_headers is insufficient because status.success() does not confirm the test filter matched. Capture the child command’s output and assert the test harness reports exactly one test executed for each environment-variable case, while preserving the existing success assertion and rejection checks.crates/ffi/tests/unit/api/registry_tests.rs (1)
14-62: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Collector thread can block forever;
join()then hangs the suite.
listener.accept()has no timeout and the thread handles exactly one connection. If the exporter never connects — or connects twice (two flushes are issued at lines 725 and 727) — the thread stays parked inacceptandcollector.join().unwrap()at line 757 never returns. Tests holdingTEST_MUTEXmake that a suite-wide stall rather than a single failure.Bound the accept with a nonblocking/poll loop plus a deadline, and drain additional connections instead of assuming one.
🛡️ Sketch: deadline-bounded accept
fn start_otlp_http_collector() -> (String, Receiver<Vec<u8>>, JoinHandle<()>) { let listener = TcpListener::bind("127.0.0.1:0").unwrap(); let address = listener.local_addr().unwrap(); let (sender, receiver) = mpsc::channel(); let handle = thread::spawn(move || { - let (mut stream, _) = listener.accept().unwrap(); + listener.set_nonblocking(true).unwrap(); + let deadline = std::time::Instant::now() + Duration::from_secs(10); + let mut stream = loop { + match listener.accept() { + Ok((stream, _)) => break stream, + Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => { + if std::time::Instant::now() >= deadline { + return; + } + thread::sleep(Duration::from_millis(10)); + } + Err(error) => panic!("collector accept failed: {error}"), + } + }; + stream.set_nonblocking(false).unwrap();🤖 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/unit/api/registry_tests.rs` around lines 14 - 62, Update start_otlp_http_collector to avoid an unbounded listener.accept and single-connection assumption: configure the listener for nonblocking or timed polling, enforce a finite deadline, and continue accepting and draining additional exporter connections (including both flushes) until the expected requests are handled or the deadline expires. Ensure the collector thread always exits so collector.join cannot hang the suite.crates/node/observability.d.ts (1)
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Drop
| stringfrom thetransportunion.The trailing
| stringcollapses the union tostring, so typos like'grcp'typecheck even though the core rejects them (validate_opentelemetry_sectionflags unsupported transports). It also diverges from the Python stub, which usesLiteral["http_binary", "grpc"].♻️ Proposed narrowing
- transport?: 'http_binary' | 'grpc' | string; + transport?: 'http_binary' | 'grpc';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.transport?: 'http_binary' | 'grpc';🤖 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/node/observability.d.ts` at line 72, Update the transport property in the observability type declaration to remove the broad string member, leaving only the supported 'http_binary' and 'grpc' literals so invalid transport names are rejected by type checking.crates/node/src/api/mod.rs (1)
171-189: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Node binding accepts a blank
endpoint; other bindings reject it.
build_otel_configpassesoptions.endpointstraight through with no emptiness check. FFI (parse_required_otel_endpoint), Python (to_rust_configtrim check), and Go (NewOpenTelemetrySubscriber) all fail fast on a blank endpoint, sonew OpenTelemetrySubscriber({ type: 'full', endpoint: ' ' })silently builds an exporter with a useless destination only in Node.🐛 Proposed parity fix
let otel_type = parse_otel_type(&options.r#type)?; - let endpoint = options.endpoint; + let endpoint = options.endpoint; + if endpoint.trim().is_empty() { + return Err(napi::Error::from_reason( + "endpoint is required and must be nonblank", + )); + } let transport = parse_otel_transport(options.transport)?;As per coding guidelines, "Require every OpenTelemetry endpoint to specify a type and a nonblank destination."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn build_otel_config( options: OpenTelemetryConfig, ) -> napi::Result<nemo_relay::observability::otel::OpenTelemetryConfig> { let otel_type = parse_otel_type(&options.r#type)?; let endpoint = options.endpoint; if endpoint.trim().is_empty() { return Err(napi::Error::from_reason( "endpoint is required and must be nonblank", )); } let transport = parse_otel_transport(options.transport)?; let service_name = options .service_name .unwrap_or_else(|| "unknown_service".to_string()); let instrumentation_scope = options .instrumentation_scope .unwrap_or_else(|| "opentelemetry".to_string()); let timeout_millis = options.timeout_millis.unwrap_or(3_000); let mut config = nemo_relay::observability::otel::OpenTelemetryConfig::new(otel_type, endpoint) .with_transport(transport) .with_service_name(service_name) .with_instrumentation_scope(instrumentation_scope) .with_timeout(std::time::Duration::from_millis(timeout_millis.into()));🤖 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/node/src/api/mod.rs` around lines 171 - 189, Update build_otel_config to validate options.endpoint before constructing OpenTelemetryConfig: trim it, reject empty or whitespace-only values with a napi error, and pass the validated endpoint onward. Preserve the existing type, transport, and default-option handling.Sources: Coding guidelines, Path instructions
crates/python/src/py_types/observability.rs (1)
417-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Python native surface names the discriminator
otel_typewhile Node, Go, the Python dataclass, and the TOML schema all usetype. One root cause: the pyclass field is exported under its Rust name instead of atypealias, and the stub mirrors it.
crates/python/src/py_types/observability.rs#L417-L423: export the field astypevia#[pyo3(get, set, name = "type")](keep the Rust fieldotel_type).python/nemo_relay/_native.pyi#L1016-L1016: rename the declared attribute totype: Literal["full", "gen_ai", "openinference"]to match; the positional__init__parameter can keep itsotel_typename.As per coding guidelines, "keep FFI, binding-native wrappers, and Python, Go, and Node.js config objects and subscriber/exporter methods consistent in logical knobs and semantics."
📍 Affects 2 files
crates/python/src/py_types/observability.rs#L417-L423(this comment)python/nemo_relay/_native.pyi#L1016-L1016🤖 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/python/src/py_types/observability.rs` around lines 417 - 423, The Python native configuration currently exposes the discriminator as otel_type instead of type. In crates/python/src/py_types/observability.rs lines 417-423, keep the Rust field named otel_type but export it with the PyO3 name type; in python/nemo_relay/_native.pyi line 1016, rename the declared attribute to type while leaving the positional __init__ parameter as otel_type.Source: Coding guidelines
docs/about-nemo-relay/release-notes/index.mdx (1)
87-89: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the concrete
header_envrejection cases.The note only says Relay “validates the variable”; the v3 contract rejects missing, blank, and duplicate headers. State those cases explicitly so endpoint activation failures are actionable and consistent with the observability checklist.
🤖 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 `@docs/about-nemo-relay/release-notes/index.mdx` around lines 87 - 89, Update the release note describing opentelemetry.endpoints header_env validation to explicitly state that activation rejects missing environment variables, blank values, and duplicate headers, making endpoint activation failures actionable and aligned with the v3 contract.docs/configure-plugins/observability/configuration.mdx (1)
136-153: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect the Node.js observability surface for exported factory names and accepted field keys. fd -t f 'observability.d.ts' crates/node --exec cat -n fd -t f 'observability.js' crates/node --exec cat -n rg -n --iglob '*observability*' -e 'serviceName' -e 'service_name' -e 'ComponentSpec' crates/node | head -50Repository: NVIDIA/NeMo-Relay
Length of output: 2049
🌐 Web query:
NeMo Relay Node.js observability ComponentSpec openTelemetryConfig openTelemetryEndpoint service_name serviceName💡 Result:
In the NVIDIA NeMo Relay Node.js library, observability is managed through the observability plugin component, which utilizes the ComponentSpec to configure various exporters, including OpenTelemetry [1][2][3]. The ComponentSpec is a function that creates a top-level plugin component for observability [1][4]. When configuring OpenTelemetry via this component, you use the otlpConfig helper to define your settings, which includes the OpenTelemetry endpoint and service identity parameters [1][2]. Key configuration properties for OpenTelemetry within the NeMo Relay observability plugin include: service_name: Defines the service.name resource attribute for the OpenTelemetry exporter [2][5]. serviceName: This corresponds to the service.name attribute when configuring the OpenTelemetrySubscriber directly (as opposed to via the observability plugin) [6]. endpoint: Specifies the OTLP exporter endpoint (e.g., http://localhost:4318/v1/traces) [2][6][5]. Example usage within the Node.js plugin initialization: const observability = require("nemo-relay-node/observability"); //... within plugin.initialize({ components: [...] }) observability.ComponentSpec({ version: 1, opentelemetry: observability.otlpConfig({ enabled: true, endpoint: "http://localhost:4318/v1/traces", service_name: "nemo-relay", //... other attributes }), }), Alternatively, if you are not using the plugin component wrapper and are using the OpenTelemetrySubscriber directly, the configuration interface (OpenTelemetryConfig) uses serviceName as the property name [6]. For detailed configuration, refer to the official NeMo Relay documentation for observability [2][5] and the Node.js library API references [1][6].
Citations:
- 1: https://docs.nvidia.com/nemo/relay/v0.4.0/reference/api/nodejs-library-reference/nemo-relay-node-observability
- 2: https://docs.nvidia.com/nemo/relay/configure-plugins/observability/configuration
- 3: https://docs.nvidia.com/nemo/relay/configure-plugins/observability/about
- 4: https://docs.nvidia.com/nemo/relay/v0.4.0/reference/api/nodejs-library-reference/nemo-relay-node-plugin
- 5: https://docs.nvidia.com/nemo/relay/configure-plugins/observability/opentelemetry
- 6: https://docs.nvidia.com/nemo/relay/v0.4.0/reference/api/nodejs-library-reference/nemo-relay-node
🌐 Web query:
site:docs.nvidia.com/nemo/relay v0.4.0 Node.js observability ComponentSpec otlpConfig openTelemetryConfig openTelemetryEndpoint service_name💡 Result:
In NVIDIA NeMo Relay v0.4.0, ComponentSpec, otlpConfig, and related OpenTelemetry configurations are used to set up observability within Node.js applications [1][2]. ComponentSpec is a function used to wrap observability configuration as a top-level plugin component [1]. The otlpConfig function creates OpenTelemetry Protocol (OTLP) exporter settings for either OpenTelemetry or OpenInference [1][3]. The OtlpConfig interface defines the following relevant fields for OpenTelemetry [1]: - enabled (boolean): Toggles the exporter [1][2]. - transport ('http_binary' | 'grpc' | string): Sets the transport mechanism [1][2]. - endpoint (string): The OTLP endpoint URL [1][2]. - service_name (string): Sets the service.name resource attribute [1][2]. - service_namespace (string): Sets the service.namespace resource attribute [1][2]. - service_version (string): Sets the service.version resource attribute [1][2]. - instrumentation_scope (string): The name of the instrumentation scope [1][2]. - headers (Record<string, string>): Exporter headers [1][2]. - resource_attributes (Record<string, string>): OTLP resource attributes [1][2]. - timeout_millis (number): Export timeout in milliseconds [1][2]. An example usage in a Node.js environment involves initializing the plugin with these components [4]: const observability = require("nemo-relay-node/observability"); //... observability.ComponentSpec({ version: 1, opentelemetry: observability.otlpConfig({ enabled: true, transport: "http_binary", endpoint: "http://localhost:4318/v1/traces", service_name: "agent-service", service_namespace: "nemo", service_version: "1.0.0", instrumentation_scope: "nemo-relay-otel", resource_attributes: { "deployment.environment": "dev", }, headers: { authorization: "Bearer ", }, }), })
Citations:
- 1: https://docs.nvidia.com/nemo/relay/v0.4.0/reference/api/nodejs-library-reference/nemo-relay-node-observability
- 2: https://docs.nvidia.com/nemo/relay/v0.4.0/observability-plugin/opentelemetry
- 3: https://docs.nvidia.com/nemo/relay/reference/api/nodejs-library-reference/nemo-relay-node-observability
- 4: https://docs.nvidia.com/nemo/relay/configure-plugins/observability/opentelemetry
🌐 Web query:
site:docs.nvidia.com/nemo/relay "nemo-relay-node/observability" otlpConfig openTelemetryConfig openTelemetryEndpoint ComponentSpec💡 Result:
In NVIDIA NeMo Relay, the observability system allows you to configure exporters for OpenTelemetry (OTLP) and OpenInference using the observability plugin [1]. ComponentSpec is a helper function used to create a plugin component entry for a plugin configuration document, packaging the plugin kind (observability), component-local configuration, and enablement flags [2]. The otlpConfig function is used to create OTLP exporter settings for either OpenTelemetry or OpenInference [3][4]. It accepts an OtlpConfig object which defines the following parameters [4][5]: - enabled: Boolean to toggle the subscriber (defaults to false). - transport: Specifies the transport protocol, either http_binary or grpc (defaults to http_binary). - endpoint: The OTLP endpoint (e.g., http://localhost:4318/v1/traces). - headers: A dictionary of string-to-string headers for the exporter. - resource_attributes: A dictionary of string-to-string OTLP resource attributes. - service_name, service_namespace, service_version: Metadata fields for the exported telemetry. - instrumentation_scope: The name of the instrumentation scope. - timeout_millis: The export timeout in milliseconds (defaults to 3000). When using the observability plugin, you configure these settings within the opentelemetry or openinference sections of the configuration, typically passed through ComponentSpec during plugin initialization [5][6][7]. Example usage: const observability = require("nemo-relay-node/observability"); //... inside plugin initialization components array: observability.ComponentSpec({ version: 1, opentelemetry: observability.otlpConfig({ enabled: true, endpoint: "http://localhost:4318/v1/traces", service_name: "nemo-relay", //... other attributes }), })
Citations:
- 1: https://docs.nvidia.com/nemo/relay/configure-plugins/observability/about
- 2: https://docs.nvidia.com/nemo/relay/v0.4.0/reference/api/nodejs-library-reference/nemo-relay-node-plugin
- 3: https://docs.nvidia.com/nemo/relay/reference/api/nodejs-library-reference/nemo-relay-node-observability
- 4: https://docs.nvidia.com/nemo/relay/v0.4.0/reference/api/nodejs-library-reference/nemo-relay-node-observability
- 5: https://docs.nvidia.com/nemo/relay/configure-plugins/observability/opentelemetry
- 6: https://docs.nvidia.com/nemo/relay/configure-plugins/observability/configuration
- 7: https://docs.nvidia.com/nemo/relay/v0.4.0/observability-plugin/configuration
Use the Node.js OTLP helper names here. The public Node.js observability API uses
observability.ComponentSpec(...)withobservability.otlpConfig(...)andendpoint/service_namefields;openTelemetryConfigandopenTelemetryEndpointdon’t match that surface.🤖 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 `@docs/configure-plugins/observability/configuration.mdx` around lines 136 - 153, Update the Node.js observability example to use the public helper names and fields: replace observability.openTelemetryConfig with observability.otlpConfig, replace observability.openTelemetryEndpoint with the corresponding OTLP endpoint helper, and preserve the endpoint and service_name values.Source: Coding guidelines
docs/configure-plugins/observability/opentelemetry.mdx (1)
33-36: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a lead-in sentence before the code block.
The heading is immediately followed by the fenced block.
📝 Proposed fix
## `plugins.toml` Example +The following `plugins.toml` fragment enables two typed endpoints: + ```tomlAs per coding guidelines, "Introduce every code block with a complete sentence" and "Do not make a code block complete the grammar of the previous sentence."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.## `plugins.toml` Example The following `plugins.toml` fragment enables two typed endpoints:🤖 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 `@docs/configure-plugins/observability/opentelemetry.mdx` around lines 33 - 36, Update the “plugins.toml Example” section by adding a complete lead-in sentence between the heading and the fenced TOML block, ensuring the code block does not directly follow the heading or complete the sentence’s grammar.Source: Coding guidelines
go/nemo_relay/coverage_gap_test.go (1)
154-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate the endpoint-validation assertion.
An empty config fails first with
type is required, so this test does not verify the endpoint requirement. SetTypeto a valid projection such as"full", leaveEndpointempty, and assert the returned error isendpoint is required.Proposed test adjustment
- if _, err := NewOpenTelemetrySubscriber(OpenTelemetryConfig{}); err == nil { - t.Fatal("expected a required endpoint error") + _, err := NewOpenTelemetrySubscriber(OpenTelemetryConfig{Type: "full"}) + if err == nil || err.Error() != "endpoint is required" { + t.Fatalf("expected endpoint validation error, got %v", err) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func assertZeroSubscriberConfigs(t *testing.T) { t.Helper() _, err := NewOpenTelemetrySubscriber(OpenTelemetryConfig{Type: "full"}) if err == nil || err.Error() != "endpoint is required" { t.Fatalf("expected endpoint validation error, got %v", err) }🤖 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/coverage_gap_test.go` around lines 154 - 158, Update assertZeroSubscriberConfigs to set OpenTelemetryConfig.Type to a valid projection such as "full" while leaving Endpoint empty, then assert that NewOpenTelemetrySubscriber returns the specific "endpoint is required" error.
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 (1)
crates/cli/src/diagnostics/probes.rs (1)
130-180: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMove these Rust tests out of
src.
tcp_testswas added insidecrates/cli/src/diagnostics/probes.rs. Move it undercrates/cli/tests/and exercise the supported diagnostic path or an appropriate test seam.As per coding guidelines, Rust tests must belong in crate
tests/trees and must not be added undersrc.🤖 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/cli/src/diagnostics/probes.rs` around lines 130 - 180, Move the tcp_tests module out of diagnostics/probes.rs into the crate’s tests tree, preserving coverage for probe_tcp_named and grpc_endpoint_port through the supported diagnostic API or an appropriate public test seam. Remove the in-src test module and adjust imports or visibility only as needed for the relocated tests to compile.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/cli/src/diagnostics/probes.rs`:
- Around line 130-180: Move the tcp_tests module out of diagnostics/probes.rs
into the crate’s tests tree, preserving coverage for probe_tcp_named and
grpc_endpoint_port through the supported diagnostic API or an appropriate public
test seam. Remove the in-src test module and adjust imports or visibility only
as needed for the relocated tests to compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c1d0f1b8-9581-4aff-8bd7-4346e1991087
📒 Files selected for processing (2)
crates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (10)
**/*.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/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/cli/src/diagnostics/probes.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/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
**/*.{rs,py,go,js,ts}: When observability configuration or lifecycle is exposed, keep FFI, binding-native wrappers, and Python, Go, and Node.js APIs aligned in logical knobs and semantics.
For observability configuration version 3, use oneopentelemetrysection with typed endpoints; OpenInference must not have a standalone public configuration surface.
Every OpenTelemetry endpoint must specify a type and a nonblank destination.
Resolveheader_envvalues when activating an endpoint and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Files:
crates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/shared/doctor_tests.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA/NeMo-Relay
Timestamp: 2026-07-27T02:36:31.655Z
Learning: Keep changes scoped, surface assumptions, and define focused validation before editing; use `karpathy-guidelines` alongside this guidance.
Learnt from: CR
Repo: NVIDIA/NeMo-Relay
Timestamp: 2026-07-27T02:36:31.655Z
Learning: Run affected Rust crate tests and `just test-rust` when event fields change; run `just test-python`, `just test-go`, and `just test-node` when binding-native configuration or lifecycle changes; update docs and examples in the same branch.
🔇 Additional comments (2)
crates/cli/src/diagnostics/probes.rs (1)
76-117: LGTM!Also applies to: 119-127
crates/cli/tests/coverage/shared/doctor_tests.rs (1)
11-11: LGTM!Also applies to: 920-927, 936-937, 951-1019, 1205-1205, 1290-1290
Signed-off-by: Will Killian <wkillian@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/core/tests/unit/observability/otel_tests.rs`:
- Around line 1300-1358: Update the usage mapping in end_attributes, covered by
gen_ai_projection_emits_normalized_response_attributes, so
gen_ai.usage.input_tokens uses the sum of Usage.prompt_tokens,
Usage.cache_read_tokens, and Usage.cache_write_tokens rather than prompt_tokens
alone. Preserve the existing output_tokens and cache-specific attributes, and
update the test expectation to the resulting total of 21.
🪄 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: 0baf7c45-8fd9-433a-b3de-8f56318d0b26
📒 Files selected for processing (1)
crates/core/tests/unit/observability/otel_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (16)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/core/tests/unit/observability/otel_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/tests/unit/observability/otel_tests.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation
Files:
crates/core/tests/unit/observability/otel_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/tests/unit/observability/otel_tests.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/core/tests/unit/observability/otel_tests.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/core/tests/unit/observability/otel_tests.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/core/tests/unit/observability/otel_tests.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/core/tests/unit/observability/otel_tests.rs
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/core/tests/unit/observability/otel_tests.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
**/*.{rs,py,go,js,ts}: Keep FFI, binding-native wrappers, and Python, Go, and Node.js configuration objects and subscriber/exporter methods aligned in exposed configuration and lifecycle semantics.
Every OpenTelemetry endpoint must specify a type and a nonblank destination.
Resolveheader_envvalues at activation and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Run affected Rust crate tests andjust test-rustwhen event fields change; runjust test-python,just test-go, andjust test-nodewhen binding-native configuration or lifecycle changes.
Files:
crates/core/tests/unit/observability/otel_tests.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/core/tests/unit/observability/otel_tests.rs
**/*.{rs,md,mdx}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
For observability configuration version 3, use one
opentelemetrysection containing typed endpoints; do not expose OpenInference as a standalone public surface.
Files:
crates/core/tests/unit/observability/otel_tests.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
OpenTelemetry and OpenInference dependencies must be unconditional rather than Cargo feature-gated.
Files:
crates/core/tests/unit/observability/otel_tests.rs
**/*.{rs,py,go,js,ts,md,mdx}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Examples and documentation must use each exporter's documented flush/deregister order before shutdown.
Files:
crates/core/tests/unit/observability/otel_tests.rs
crates/{core,adaptive}/**/*.rs
⚙️ CodeRabbit configuration file
crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/core/tests/unit/observability/otel_tests.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/core/tests/unit/observability/otel_tests.rs
🔇 Additional comments (1)
crates/core/tests/unit/observability/otel_tests.rs (1)
18-19: LGTM!
| #[test] | ||
| fn gen_ai_projection_emits_normalized_response_attributes() { | ||
| let event = make_scope_event_with_profile( | ||
| ScopeCategory::End, | ||
| Uuid::now_v7(), | ||
| None, | ||
| "chat", | ||
| ScopeType::Llm, | ||
| Some(json!({"answer": "ok"})), | ||
| Some( | ||
| CategoryProfile::builder() | ||
| .annotated_response(std::sync::Arc::new(AnnotatedLlmResponse { | ||
| id: Some("response-1".to_string()), | ||
| model: Some("model-1".to_string()), | ||
| finish_reason: Some(FinishReason::ToolUse), | ||
| usage: Some(Usage { | ||
| prompt_tokens: Some(13), | ||
| completion_tokens: Some(8), | ||
| total_tokens: Some(21), | ||
| cache_read_tokens: Some(5), | ||
| cache_write_tokens: Some(3), | ||
| cost: None, | ||
| }), | ||
| ..empty_annotated_response() | ||
| })) | ||
| .build(), | ||
| ), | ||
| ); | ||
|
|
||
| let attributes = attr_map(&crate::observability::otel_genai::end_attributes(&event)); | ||
| assert_eq!( | ||
| attributes.get("gen_ai.response.id"), | ||
| Some(&"response-1".to_string()) | ||
| ); | ||
| assert_eq!( | ||
| attributes.get("gen_ai.response.model"), | ||
| Some(&"model-1".to_string()) | ||
| ); | ||
| assert_eq!( | ||
| attributes.get("gen_ai.response.finish_reasons"), | ||
| Some(&"[\"tool_calls\"]".to_string()) | ||
| ); | ||
| assert_eq!( | ||
| attributes.get("gen_ai.usage.input_tokens"), | ||
| Some(&"13".to_string()) | ||
| ); | ||
| assert_eq!( | ||
| attributes.get("gen_ai.usage.output_tokens"), | ||
| Some(&"8".to_string()) | ||
| ); | ||
| assert_eq!( | ||
| attributes.get("gen_ai.usage.cache_read.input_tokens"), | ||
| Some(&"5".to_string()) | ||
| ); | ||
| assert_eq!( | ||
| attributes.get("gen_ai.usage.cache_creation.input_tokens"), | ||
| Some(&"3".to_string()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the usage/response attribute projection logic for GenAI to confirm input_tokens/finish_reasons semantics.
rg -n -C6 'gen_ai\.usage\.input_tokens|gen_ai\.response\.finish_reasons' crates/core/src/observability/otel_genai.rsRepository: NVIDIA/NeMo-Relay
Length of output: 1737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the GenAI projection logic and the test attribute serialization helper.
sed -n '1,220p' crates/core/src/observability/otel_genai.rs
printf '\n==== attr_map / string_array references ====\n'
rg -n -C4 'fn attr_map|attr_map\(|string_array\(' crates/core/tests/unit/observability/otel_tests.rs crates/core/src/observability -g '!**/target/**'Repository: NVIDIA/NeMo-Relay
Length of output: 44873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace where response usage is constructed and whether cache tokens are folded into prompt_tokens.
rg -n -C5 'prompt_tokens|cache_read_tokens|cache_write_tokens|normalized_llm_response|AnnotatedLlmResponse|Usage \{' crates/core/src crates/core/tests -g '!**/target/**'
printf '\n==== relevant response normalization code ====\n'
sed -n '1,260p' crates/core/src/codec/response.rs
printf '\n==== llm normalization references ====\n'
rg -n -C4 'normalized_llm_response|annotated_response|Usage' crates/core/src -g '!**/target/**'Repository: NVIDIA/NeMo-Relay
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how LLM response usage is normalized and whether cache tokens contribute to prompt_tokens.
rg -n -C6 'normalized_llm_response|prompt_tokens|cache_read_tokens|cache_write_tokens|finish_reasons' crates/core/src crates/core/tests -g '!**/target/**'
printf '\n==== response.rs ====\n'
sed -n '1,320p' crates/core/src/codec/response.rs
printf '\n==== any Anthropic/OpenAI response normalization ====\n'
rg -n -C4 'Anthropic|OpenAI|cache_read|cache_write|prompt_tokens' crates/core/src/codec crates/core/src -g '!**/target/**'Repository: NVIDIA/NeMo-Relay
Length of output: 50374
🌐 Web query:
OpenTelemetry GenAI semantic conventions input_tokens cache_read_input_tokens cache_creation_input_tokens total input_tokens spec
💡 Result:
The OpenTelemetry semantic conventions for Generative AI (GenAI) define specific attributes to track token usage, including those related to prompt caching [1][2]. These conventions are maintained in the OpenTelemetry GenAI semantic conventions repository [3][4]. Key attributes related to input token usage are: 1. gen_ai.usage.input_tokens: The total number of tokens used in the GenAI input (prompt) [1][2]. This value SHOULD include all types of input tokens, including cached tokens [1][2]. Instrumentations are encouraged to make a best effort to populate this total by summing different token types parsed from the provider output if a total is not explicitly provided [1][2]. 2. gen_ai.usage.cache_read.input_tokens: The number of input tokens served from a provider-managed cache [1][2]. This value SHOULD be included in the total gen_ai.usage.input_tokens [1][2]. 3. gen_ai.usage.cache_creation.input_tokens: The number of input tokens written to a provider-managed cache [1][2]. This value SHOULD also be included in the total gen_ai.usage.input_tokens [1][2]. These attributes are categorized as Recommended [1][2]. Because different AI providers report token usage in varied ways (e.g., some include cached tokens in their total input_tokens while others report them separately, such as Anthropic), instrumentations may need to perform calculations to ensure the gen_ai.usage.input_tokens attribute represents the accurate total, for example: gen_ai.usage.input_tokens = input_tokens + cache_read_input_tokens + cache_creation_input_tokens (where specific provider values exclude cache tokens) [5][6][7].
Citations:
- 1: https://github.com/open-telemetry/semantic-conventions-genai/blob/main/docs/gen-ai/gen-ai-spans.md
- 2: https://github.com/open-telemetry/semantic-conventions-genai/blob/main/docs/registry/attributes/gen-ai.md
- 3: https://github.com/open-telemetry/semantic-conventions-genai
- 4: https://opentelemetry.io/docs/specs/semconv/registry/attributes/gen-ai/
- 5: https://github.com/open-telemetry/semantic-conventions/blob/main/model/gen-ai/spans.yaml
- 6: https://github.com/open-telemetry/semantic-conventions/blob/v1.41.0/model/gen-ai/spans.yaml
- 7: genai: define cached tokens attributes open-telemetry/semantic-conventions#3163
Include cache tokens in gen_ai.usage.input_tokens. finish_reasons is already emitted as an array-valued OTel attribute; the remaining issue is that usage.prompt_tokens is forwarded directly here, so this test exports 13 instead of the GenAI total 21 when cache tokens are present. Sum prompt_tokens + cache_read_tokens + cache_write_tokens before setting gen_ai.usage.input_tokens.
🤖 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/tests/unit/observability/otel_tests.rs` around lines 1300 - 1358,
Update the usage mapping in end_attributes, covered by
gen_ai_projection_emits_normalized_response_attributes, so
gen_ai.usage.input_tokens uses the sum of Usage.prompt_tokens,
Usage.cache_read_tokens, and Usage.cache_write_tokens rather than prompt_tokens
alone. Preserve the existing output_tokens and cache-specific attributes, and
update the test expectation to the resulting total of 21.
1 participant
Warning
BREAKING CHANGE — observability configuration and exporter APIs: Observability configuration version 3 replaces the standalone
opentelemetryandopeninferenceexporter shapes with typedopentelemetry.endpoints. Direct Rust, Python, Node.js, Go, and C callers must replaceOpenInferenceConfig/OpenInferenceSubscriberwithOpenTelemetryConfig/OpenTelemetrySubscriberand selecttype = "openinference". Version-2 OTLP component shapes are rejected under version 3. Projection mark, alias, semantic-selector, attribute-mapping, exclusion, and content-capture controls are removed in favor of fixedfull,gen_ai, andopeninferencebehavior.Overview
Unifies Relay's OTLP exporters behind typed OpenTelemetry endpoints and adds a standards-only GenAI semantic projection while preserving the complete Relay and OpenInference projections as explicit endpoint types.
Details
OpenTelemetryTypediscriminator withfull,gen_ai, andopeninferenceprojections and one independently managedOpenTelemetrySubscriberAPI.opentelemetry.endpoints, per-endpointheader_env, heterogeneous and repeated endpoint fan-out, atomic construction, and aggregate flush/shutdown handling.nemo_relay.*or opt-in content attributes; unsupported scopes are omitted while preserving descendant ancestry.object_storeto 0.14.1, removes the resolveddeny.tomladvisory exceptions, and refreshes generated Rust attribution data.Migration examples and the version-2 to version-3 mapping are documented in
docs/reference/migration-guides.mdxanddocs/configure-plugins/observability/opentelemetry.mdx.Where should the reviewer start?
Start with
crates/core/src/observability/otel.rsfor the unified subscriber and endpoint construction,crates/core/src/observability/plugin_component.rsfor version-3 multi-endpoint configuration and fan-out, andcrates/core/src/observability/otel_genai.rsfor the GenAI projection contract. Binding migrations are anchored bycrates/ffi/src/api/observability.rs.Validation performed on commit
7fb9fbc4after rebasing onto42c7655f:The first local Go invocation inherited a user-level version-2 observability file; the isolated CI-equivalent rerun passed. Documentation validation completed with zero errors; the external redirect comparison was skipped after FDR returned 403.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
full,gen_ai, andopeninference.header_env, and resource attributes.Breaking Changes
Documentation
Closes RELAY-503