feat: export OTLP traces to a local file - #1042
SandyChapman wants to merge 7 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
willkill07
left a comment
There was a problem hiding this comment.
I feel like this needs a proper proposal rather than instructing Claude to implement the functionality loosely based on need.
I understand this is in draft form, but it isn't conducive to any significant review time without a corresponding proposal.
Ultimately, we need a configuration shape for the new functionality without breaking existing plugin configuration files.
Relay's OpenTelemetry plugin could only ship spans to a collector. ATIF and ATOF both write to disk, so a consumer that treats a trajectory as an artifact rather than as telemetry -- an evaluation harness, an offline replay, any environment with no collector to export to -- had no OTLP option at all. `[[components.config.opentelemetry.file_sinks]]` writes the same `ExportTraceServiceRequest` an endpoint would receive. The default `json_lines` format follows the OpenTelemetry Protocol File Exporter specification: one OTLP/JSON record per line. `proto` writes each record length-delimited, matching the Collector file exporter's layout, for consumers that would rather not pay JSON's size and parse cost. A file sink is modelled as a destination rather than as a third `OtlpTransport`: it has no endpoint, headers, or timeout, and endpoint validation does not apply to it. Everything above the exporter -- projection, id generation, batching, resource attributes -- is shared, so the two destination kinds cannot drift on what a span means. `SpanExporter::export` returns `impl Future` and the trait is therefore not dyn-compatible, so dispatch is a concrete enum rather than a boxed trait object. Records are flushed before each export returns, so a run that dies between batches still leaves a readable prefix. Output is created with the same owner-only permissions and directory confinement as the ATOF and ATIF sinks, because a trajectory carries prompt and response content. Signed-off-by: Sandy Chapman <schapman@nvidia.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
`nemo-relay configure` lists file sinks beside trace endpoints, and the Python, Node, Go, and FFI surfaces each gain the destination so a programmatically built subscriber can write a trace without a collector. The Node suite caught a second validation site: `validate_opentelemetry_section` runs before activation and still required an endpoint, so a file-sink-only section was rejected before `register_opentelemetry` ever saw it. It now accepts a file sink as a destination and reports a malformed one per index, matching how it already reports endpoints. Headers and `header_env` are refused on a Python file sink rather than silently dropped: a caller who sets them has misunderstood the destination. Signed-off-by: Sandy Chapman <schapman@nvidia.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Signed-off-by: Sandy Chapman <schapman@nvidia.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
`opentelemetry-proto`'s `with-serde` feature, which provides the OTLP/JSON serialization the file exporter's default format needs, adds `const-hex` to the graph and its dev dependencies to the lock. Signed-off-by: Sandy Chapman <schapman@nvidia.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
`OpenTelemetryConfig` carried `endpoint`, `transport`, `headers`, `header_env`, and `timeout` alongside an optional file sink, so a config holding both was representable and the endpoint half was dropped without a word. `nemo-relay plugins edit` can modify a configuration programmatically, so that combination was reachable from a supported surface, and a config that reads as though it exports to a collector while it writes a file gives no signal that anything is wrong. Those five fields move into `TraceDestination::Otlp`, leaving the two destinations mutually exclusive by construction. Endpoint and header validation moves with them, which removes the conditional that gated it on the absence of a file sink. The builders stay infallible, so misuse is recorded and refused at construction rather than silently dropped: `endpoint, headers, transport do not apply to a file sink destination`, deduplicated and sorted. Each binding enforces the same rule at its own boundary. Python turns the three endpoint-only attributes into setters that raise, and Node rejects them alongside `outputDirectory` -- which it had been applying to file sinks all along, since it passed `timeoutMillis` and the header map through unconditionally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
dad6459 to
a7621e8
Compare
The FFI entry point existed but nothing declared or called it from Go, so a Go consumer could configure a file sink through `plugins.toml` and had no way to build one programmatically. `NewOpenTelemetryFileSinkSubscriber` closes that, taking a config with no endpoint, transport, headers, or timeout, since none of them apply to a file. Coverage measures `crates/ffi/src` through the Go suite, so the entry point was also reported as entirely unexercised despite its Rust-side test. Also covers the two reachable error paths in the exporter: a parent path that is a file, so the output directory cannot be created, and a poisoned writer lock, which every entry point reports instead of unwrapping into a second panic. Signed-off-by: Sandy Chapman <schapman@nvidia.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`TraceExporter` existed to dispatch between the two exporters because `SpanExporter::export` returns `impl Future`, which makes the trait not dyn-compatible. A boxed trait object is still impossible, but the enum was not the only way out: `DiagnosticBatchSpanProcessor` is already generic over its exporter, so the provider construction takes the exporter as a type parameter and each destination hands over its concrete one. Thirty lines of hand-written dispatch go away. The builders matched on `endpoint_settings_mut()`, which turned a two-variant enum back into an `Option` and matched on that. They now match the enum directly, and the accessor has no callers left. The comments claimed more than they were worth. `TraceDestination` explained that an enum is mutually exclusive and then restated its own variants; the exporter module opened with motivation that belongs in a design discussion. What is left is the part a reader cannot recover from the code: why an empty batch writes nothing, why the JSON is compact, why a flush after shutdown succeeds, and why the output is owner-only. Signed-off-by: Sandy Chapman <schapman@nvidia.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
willkill07
left a comment
There was a problem hiding this comment.
Just leaving a round of comments
| `output_directory`, the same as the ATOF and ATIF file sinks, because a | ||
| trajectory carries prompt and response content. |
| Endpoint validation does not apply: a file sink has no endpoint, transport, | ||
| headers, or request timeout, and configuring those fields on one is an error. |
There was a problem hiding this comment.
This is obvious simply through the configuration, is it not?
| A file sink writes the same OTLP `ExportTraceServiceRequest` an endpoint would | ||
| receive to a local file. Use one when a trajectory is an artifact rather than | ||
| telemetry: an evaluation harness that scores the trace it just produced, an | ||
| offline replay, or any environment with no collector to export to. File sinks |
There was a problem hiding this comment.
I really don't like Claude-isms in documentation. Can this please be rephrased?
| `json_lines` is the serialization described by the OpenTelemetry Protocol File | ||
| Exporter specification: one OTLP/JSON-encoded `ExportTraceServiceRequest` per | ||
| line, with `traceId` and `spanId` hex-encoded. `proto` writes each request | ||
| length-delimited -- a big-endian `u32` byte count followed by that many bytes of | ||
| encoded request -- matching the OpenTelemetry Collector file exporter's | ||
| `format: proto` layout. |
There was a problem hiding this comment.
Can we just concisely describe the two output formats (and when to use each) without diving into the details of its output. If it does match the specified OpenTelemetry spec, then we should directly reference it.
| Two file sinks cannot write the same path: appending sinks would interleave | ||
| their records and overwriting sinks would race, so the configuration is | ||
| rejected at activation. |
There was a problem hiding this comment.
This should be moved above the table.
| /// Set by [`PyOpenTelemetryConfig::file_sink`]; when present the spans are | ||
| /// written to this file instead of exported to `endpoint`. | ||
| pub(crate) file_sink: Option<PyOtlpFileSink>, |
There was a problem hiding this comment.
If we are doing this for the file sink, we should probably hoist out the endpoint-specific options as their own struct to reduce future confusion.
Then it could be an enum instead of two Options
| headers: HashMap<String, String>, | ||
| header_env: HashMap<String, String>, | ||
| header_file: HeaderFiles, | ||
| destination: TraceDestination, |
There was a problem hiding this comment.
For other language bindings we have a clear split between:
- OpenTelemetry endpoint
- OpenTelemetry file sink
Shouldn't we match that here?
To be clear: the plugin config is good! But the configuration options for a file sink are very distinct from the options for the endpoint, and we currently need a lot of extra logic (like inapplicable_options) gives a code smell.
Overview
Relay's OpenTelemetry plugin can only ship spans to a collector. ATIF and ATOF both write to disk, so a consumer that treats a trajectory as an artifact rather than as telemetry has no OTLP option at all. This adds
[[components.config.opentelemetry.file_sinks]], which writes the sameExportTraceServiceRequestan endpoint would receive to a local file.The argument for a sink here rather than a collector is that it removes a routable address and an egress rule for isolated sandboxes. Additionally, some users using Fabric+Relay for evaluation, may not have provision the infrastructure required to collect traces.
Note for reviewers: a companion change is needed in NeMo-Fabric
This change is necessary but not sufficient for the evaluation use case.
Callers that reach Relay through Fabric use
nemo_fabric.RelayOpenTelemetryConfig, which has two fields (enabled,endpoints) and a hand-written validator in its ownmodels.py:Checked against
nemo-fabric0.3.0b1:extra="allow", sofile_sinkssurvives serialization and reaches Relay untouched.Fabric needs
file_sinkson the model and the validator relaxed to accept a file sink as a destination, which is the same shape as thevalidate_opentelemetry_sectionchange in the second commit here.Details
The destination is a sum type.
endpoint,transport,headers,header_env, andtimeoutlive insideTraceDestination::Otlp, so a config holding both an endpoint and a file sink cannot be built:Endpoint and header validation moved into that variant, which removed the conditional that gated it on the absence of a file sink. Configuration mirrors the existing ATOF file sink (
output_directory,filename,mode) instead of overloadingendpointwith a path.No silent ignore. The builders stay infallible, so an endpoint-only option set on a file sink is recorded and refused at construction:
endpoint, headers, transport do not apply to a file sink destination, deduplicated and sorted. Each binding enforces the same rule at its own boundary. Python turns the three endpoint-only attributes into setters that raise; Node rejects them alongsideoutputDirectory, which it had been applying to file sinks all along, since it passedtimeoutMillisand the header map through unconditionally.Everything above the exporter (projection, id generation, batching, resource attributes, shutdown) is shared, so a span means the same thing whichever destination it goes to.
SpanExporter::exportreturnsimpl Future, so the trait is not dyn-compatible and dispatch is a concrete enum.Conversion is upstream.
group_spans_by_resource_and_scopeand theSpanData→ protobufFromimpls come fromopentelemetry-proto, the crateopentelemetry-otlpalready uses to build its wire payload. Bothopentelemetry-protoandprostwere already incrates/core/Cargo.tomlunder[dev-dependencies]; they move to[dependencies]with thetraceandwith-serdefeatures. No new crates and no version changes.Durability and permissions. Each export is flushed before it is reported as delivered, so a run that exits between batches leaves a readable prefix rather than an empty file. Output is created with the same owner-only permissions and directory confinement as the ATOF and ATIF sinks, because a trajectory carries prompt and response content. Two sinks writing one path are rejected at activation.
Surfaces.
nemo-relay configurelists file sinks beside trace endpoints; Python, Node, Go, and the FFI each gain the destination.Where should the reviewer start?
crates/core/src/observability/otel_file.rs— the exporter is ~200 lines and the whole design is visible there. Thencrates/core/tests/unit/observability/otel_file_tests.rs: the round-trips decode withprostandserde_jsondirectly rather than through this module's own encoder, so a writer and reader that agree only with each other cannot pass.json_lines_encode_span_identifiers_as_hexis the OTLP/JSON conformance check.endpoint_options_on_a_file_sink_are_refused_by_nameandseveral_refused_options_are_reported_togethercover the exclusivity rule.The design decision worth arguing about is
file_sinksas a separate array versus atransport = "file"variant onendpoints. I chose the former becauseendpointis required and URL-validated, and conditional validation on a field that sometimes holds a path seemed worse than a second array.Testing
cargo test --workspace— 1660 core lib tests pass. 46 new tests: exporter round-trip and framing, config validation, destination exclusivity, editor schema, and per-binding coverage.native_plugin_integrationneedsjust build-test-plugin-fixtures, which I could not run locally.cargo clippy --workspace --all-targetsandcargo fmt --allclean.node --test crates/node/tests/observability_plugin_tests.mjs— 10/10. One test drives a file-sink-only section through the plugin host and asserts the trace file appears.pytest python/tests/test_types.py— 63/63,ruffclean,tyclean (9 pre-existing unused-ignore warnings).go testonotel_test.gopasses;gofmtandgo vetclean.cargo deny(not installed) and the fixture-dependent native plugin integration tests.Breaking changes
None.
file_sinksdefaults to empty and every existing configuration behaves identically.Related Issues:
Relates to #1089