Skip to content

feat(evaluator): publish the OTLP trace to Intake when a trial has one - #1725

Open
SandyChapman wants to merge 1 commit into
evaluator-trace-format-selector/schapmanfrom
publish-otlp-to-intake/schapman
Open

feat(evaluator): publish the OTLP trace to Intake when a trial has one#1725
SandyChapman wants to merge 1 commit into
evaluator-trace-format-selector/schapmanfrom
publish-otlp-to-intake/schapman

Conversation

@SandyChapman

Copy link
Copy Markdown
Contributor

Summary

A trial's OTLP trace is now what gets published to Intake, carrying the span tree, per-call detail and timing that ATIF's turn shape flattens. Runners whose agent emits no OTLP keep publishing ATIF exactly as before — this adds a path rather than replacing one.

Stacked on #1717. Review that one first; this PR's diff is only the publish work.

Related Issue

AALGO-569 (Linear), "PR 4" in its sequencing table. Not a GitHub issue, so no Fixes keyword.

Changes

  • mapping.otlp_ingest_for_trial reads the trial's OTLP trace as a typed ExportTraceServiceRequest, stamps identity and trial totals, and returns the serialized payload plus its root span id. Returns None — falling back to ATIF — when the trial has no readable OTLP trace or no single scorable root span.
  • publish.py sends that payload via intake.ingest.otlp.v1.traces.create(body=…) (landed in fix(intake): declare the OTLP trace-ingest protobuf body #1680) and scores against the locally-read span id.
  • values/otlp.py gains set_span_attributes, set_root_span_attributes, set_root_span_error, fill_missing_start_times, and root_span_id.
  • OTLPTraceHandle.export_request() returns, now that it has a caller.

Design calls

Identity goes on span attributes, not resource attributes. Intake merges the layers as {**resource, **span}, so an agent recording its own gen_ai.conversation.id would win from the resource layer and take the session id with it. That id is part of the key Intake's ReplacingMergeTree replaces on, so losing it turns a re-publish into duplicate rows.

_resolve_root_span_id stays, for the ATIF path only. ATIF span ids are minted inside Intake via stable_id(workspace, session_id, *identity, "trajectory", prefix="span") and cannot be known locally without replicating that scheme. Deleting it outright would have broken score attachment for exactly the ATIF-only runners this PR keeps supporting. Note this corrects AALGO-569, which justifies removing the round trip with "OTLP span IDs are producer-chosen, so Evaluator already knows the id" — untrue for Harbor, where the agent under test writes the trace. The real reason is that the root span id is readable from the payload we are about to publish.

Four things the OTLP path must carry that the agent's spans do not, each found by review rather than by a failing test:

  1. Trial token and cost totals, which ATIF sent as final_metrics. On the root span alone, so a rollup summing across a trace cannot count them once per span.
  2. The trial's error, as span status — Intake reads status, not attributes, to decide a span failed. Recorded only as exception.type, a failed trial read as successful.
  3. A start time for any span lacking one. Intake stores such a span against its own ingest clock (_nanos_to_datetime(...) or ingested_at), and start time is in the replace key, so re-publish would insert instead of replacing.
  4. A check of the per-span error list ingest returns with its 200. A dropped span is otherwise invisible — including when the dropped span is the one about to be scored.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with documentation updates
  • Documentation only
  • Contributor tooling or automation
  • CI, build, or test infrastructure

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Documentation updated for user-visible behavior
  • Documentation not applicable — justification:

publish_to_intake is an internal publish path with no user-facing configuration or CLI surface; which encoding a trial publishes is a property of what the runner produced, not something a user selects. The user-facing trace-format documentation was updated in #1717.

Verification

  • Pull request title follows the repository's Conventional Commit format
  • Every commit includes an appropriate Signed-off-by: trailer
  • uv run pre-commit run -a passes, or any blocked checks are identified below
  • Targeted tests pass, or tests are marked not applicable above
  • No secrets, API keys, or credentials are included

Targeted validation:

Command Result
pytest plugins/nemo-evaluator/tests/integration/test_publish_to_intake.py (real ClickHouse + platform) 6 passed
pytest packages/nemo_evaluator_sdk/tests plugins/nemo-evaluator/tests 2706 passed, 51 skipped
tools/lint/lint-python-types.sh (CI's type gate) All checks passed
uv run ruff check packages plugins All checks passed
uv run ruff format --check packages plugins 2192 files already formatted

The integration test was run, not just written

The idempotency claim is the one unit doubles cannot reach, so it is covered end to end against real ClickHouse: publish twice, assert one trace and one span. To confirm the assertion has teeth, session_id_for was mutated to append a per-call uuid — the exact "unstable session id" failure the ReplacingMergeTree key is vulnerable to — and the test went red. Code restored and re-verified.

The four ATIF integration tests pass alongside the two new OTLP ones, so the fallback path is unregressed.

pre-commit run -a — one hook blocked, unrelated to this change

uv-lock requires uv 0.9.14; this shell has 0.9.30. No dependency changed in this PR, and the sibling uv-lock-check (drift) passed, confirming uv.lock is untouched. Every other hook passed, including ty, ruff, copyright headers, config-reference docs, helm-docs, and the plugin/nmp-common boundary check.

Known gap, not addressed here

Intake's ATIF ingest endpoint calls validate_evaluation_context; its OTLP endpoint has no equivalent. So a nonexistent evaluation name now publishes successfully where the ATIF path would have failed. This PR is the first evaluator caller to exercise that endpoint and so the first to expose the gap, but the fix belongs in Intake alongside its ATIF sibling — a client-side check here would duplicate a server responsibility and need removing later. Worth its own ticket.

A trial's OTLP trace is now what gets published, carrying the span tree,
per-call detail and timing that ATIF's turn shape flattens. Runners whose
agent emits no OTLP keep publishing ATIF unchanged, so this adds a path
rather than replacing one.

Evaluation identity is stamped on every span before serializing, not on
resource attributes: Intake merges the layers as `{**resource, **span}`,
so an agent recording its own `gen_ai.conversation.id` would win from the
resource layer and take the session id with it — and the session id is
part of the key Intake's spans table replaces on, so losing it turns a
re-publish into duplicate rows.

The score's target span is read off the payload rather than queried back
after ingest, so the OTLP path drops that round trip. `_resolve_root_span_id`
stays for the ATIF fallback, where span ids are minted inside Intake from
its own identity scheme and cannot be known locally.

Four things the OTLP path has to carry that the agent's spans do not:

* trial token and cost totals, which ATIF sent as `final_metrics`, on the
  root span alone so a rollup summing a trace cannot count them per span;
* the trial's error, as span status rather than only as an attribute,
  which is what Intake reads to decide a span failed;
* a start time for any span lacking one, because Intake otherwise stores
  it against its own ingest clock and re-publish stops replacing;
* a check of the per-span error list ingest returns with its 200, since a
  dropped span is otherwise invisible — including the one being scored.

Signed-off-by: Sandy Chapman <schapman@nvidia.com>
@github-actions github-actions Bot added the feat label Sep 2, 2026
@SandyChapman
SandyChapman marked this pull request as ready for review September 2, 2026 17:44
@SandyChapman
SandyChapman requested review from a team as code owners September 2, 2026 17:44
"""The trial-level totals and error that ATIF conveyed outside the step list."""
totals: dict[str, str | int | float] = {}
for field, attribute in _MEASUREMENT_ATTRIBUTES.items():
value = getattr(measurements, field)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I'd use here getattr(measurements, field). Looks concise but what if _MEASUREMENT_ATTRIBUTES and TrialMeasurements diverge in the future?

else None
)

def build_body(steps: Sequence[AtifStepParam] | None) -> AtifCreateParams:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function is quite long. Can we put atif and oltp related logic into separate functions and invoke them here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants