feat(#6458): export eval measurement scores via OTLP - #6459
Conversation
Wire MeasureAndExport to emit gen_ai.evaluation.result span events on the same TraceID when OTEL_EXPORTER_OTLP_* is set, matching ADR 0087 / 0050. Local JSONL stays source of truth; remote export is fail-open. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
PR Summary by QodoExport eval measurement scores as OTLP GenAI evaluation span events
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Site previewPreview: https://ac85078b-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · Commit: |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Bound post-hoc export retries/budget, share fullsend resource identity, batch scores, skip empty span IDs, keep Ok status for all labels, omit score.value on skip, and sync docs that still said OTLP was planned. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Review squad follow-upAddressed verified findings from the review pass:
Deferred (documented / intentional): separate remote-export ledger for OTLP retry after local success — remote remains best-effort once; Assessments stay a MLflow-side consumer, not core. |
|
🤖 Finished Review · ✅ Success · Started 7:52 PM UTC · Completed 8:07 PM UTC Commit: |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsLow
Next steps:
Previous run (3)ReviewFindingsLow
Previous run (4)ReviewFindingsLow
Previous run (5)ReviewFindingsLow
Previous run (6)ReviewFindingsLow
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsLow
Previous run (9)ReviewFindingsLow
Info
Previous run (10)ReviewFindingsLow
Previous run (11)ReviewFindingsLow
Previous run (12)ReviewFindingsMedium
Low
|
fullsend-ai-review
Bot
added
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review (MEDIUM+)
Four inline comments on unique issues. Fail-open OTLP path looks solid (empty IDs skipped, export bounded, batch processor). The correctness gap to fix is resource identity.
Already posted (skipped): ADR 0087 Decision rewrite on docs/ADRs/0087-eval-measurements-online-trace-scoring.md — existing thread already covers it. Preferred fix remains a 0050-style annotation rather than editing the Decision.
Align score resource service.version with CLI Version(), no-op OTLP when inbound TRACEPARENT is unsampled, apply shared span limits and truncate evaluation explanations, cite GenAI semconv and clarify vendor UI mapping, and restore ADR 0087 Decision with an Implemented annotation. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 11:33 AM UTC · Completed 11:53 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $6.68 |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Additional review pass (8 findings, all verified against PR head 911b9bf and cross-checked against existing threads for duplicates).
Scope TRACEPARENT suppression per TraceID via W3C propagator, export already-persisted scores on mid-loop persist failure, clear transient export latch on success, hermetic OTEL in Measure tests, and refresh the GenAI semconv citation. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
# Conflicts: # pkg/behaviourtest/drivers/install/ensure.go
Signed-off-by: Adam Scerra <ascerra@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 5:35 PM UTC · Completed 5:52 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.72 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
ready-for-merge
Signed-off-by: Adam Scerra <ascerra@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 5:54 PM UTC · Completed 6:14 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.68 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Review · Commit: |
Signed-off-by: Adam Scerra <ascerra@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 6:23 PM UTC · Completed 6:42 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.02 |
| // eval-measurements.jsonl. Idempotent per ledger. | ||
| func MeasureFile(telemetryPath, registryPath, outDir string) ([]EvaluationResult, error) { | ||
| r, _, err := MeasureAndExport(context.Background(), telemetryPath, registryPath, outDir) | ||
| r, _, err := MeasureAndExport(context.Background(), telemetryPath, registryPath, outDir, "") |
There was a problem hiding this comment.
[low] api-shape-consistency
MeasureFile passes empty serviceVersion to MeasureAndExport. Inside BuildResource, empty string defaults to unknown. Callers using MeasureFile will emit OTLP score spans with service.version=unknown if OTLP is configured.
Suggested fix: Add a serviceVersion parameter to MeasureFile or document the unknown default prominently in the godoc.
| sdktrace.WithResource(telemetry.BuildResource(serviceVersion)), | ||
| sdktrace.WithSampler(sdktrace.AlwaysSample()), | ||
| sdktrace.WithRawSpanLimits(telemetry.SpanLimits()), | ||
| sdktrace.WithSpanProcessor(sdktrace.NewBatchSpanProcessor(capExp, sdktrace.WithMaxQueueSize(len(exportable)))), |
There was a problem hiding this comment.
[low] edge-case
BatchSpanProcessor queue size is set to exactly len(exportable). A test validates 2049 spans work, but the exact-size match leaves no margin for future SDK internal changes.
| if len(errs) == 0 { | ||
| return nil | ||
| } | ||
| // Transport failure: nothing is known to have landed — do not claim N/M |
There was a problem hiding this comment.
[low] error-handling
When BatchSpanProcessor exports in multiple batches and an early batch succeeds but a later batch fails, the error message says otlp export failed for all N scores, which is conservative by design but could mislead debugging in partial-success scenarios.
| return out | ||
| } | ||
|
|
||
| func clearOTLPEnv(t *testing.T) { |
There was a problem hiding this comment.
[low] naming-convention
clearOTLPEnv is duplicated identically in export_otlp_test.go and cli/evalmeasure_test.go. The CLI copy cross-references the pattern. Reasonable Go testing pattern given the separate packages.
| // but some traces were still recovered. Callers should warn; MeasureAndExport | ||
| // still scores those traces and returns a nil error. | ||
| Incomplete string | ||
| // RemoteExportWarning is set when portable OTLP score export failed |
There was a problem hiding this comment.
[low] struct-placement
RemoteExportWarning is added to ParseStats but is set by the OTLP export path, not parsing. The struct already serves as the MeasureAndExport return carrier.
|
/fs-fix |
|
🤖 Finished Fix · ❌ Failure (post-script /home/runner/work/fullsend/fullsend/.fullsend/.fullsend-cache/resources/sha256/8bf60853c6b5877033b9ac827f88f835047874e2e318066bd436305316d0a38e/scripts/post-fix.sh failed: exit status 1) · Started 9:12 PM UTC · Completed 9:24 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.82 |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/fullsend/actions/runs/33683583135 Details: |
|
/fs-fix do not use a sign-off by trailer |
|
🤖 Finished Fix · ❌ Failure (post-script /home/runner/work/fullsend/fullsend/.fullsend/.fullsend-cache/resources/sha256/21644e0238d0b83db763fa138c412322223b0ba4a919a542b839e11b6a3d62e7/scripts/post-fix.sh failed: exit status 1) · Started 10:41 AM UTC · Completed 10:51 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.66 |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/fullsend/actions/runs/33745590114 Details: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review of the OTLP score-export path at e947baae (read the code at head, then verified the primary finding by executing a probe test against this branch with a stub exporter). Two medium findings below. I have not re-raised the acknowledged low-severity threads.
The suite is green at head (internal/evalmeasure, internal/telemetry, internal/cli), which is part of finding 1: the defect is invisible to the current tests.
| func (c *capturingExporter) ExportSpans(ctx context.Context, spans []sdktrace.ReadOnlySpan) error { | ||
| err := c.base.ExportSpans(ctx, spans) | ||
| c.mu.Lock() | ||
| c.err = err |
There was a problem hiding this comment.
[MEDIUM] capturingExporter drops a failed batch's error when a later batch succeeds — no CLI warning, and the ledger has already foreclosed the retry
c.err = err overwrites rather than accumulates. With more than 512 exportable rows the BatchSpanProcessor splits deterministically (SDK default MaxExportBatchSize is 512; line 141 sets WithMaxQueueSize(len(exportable)) but not WithMaxExportBatchSize). If an earlier batch fails and the last one succeeds, capExp.err is nil at line 167, ForceFlush also returns nil (its explicit exportSpans finds the batch already drained by the async processor), and ExportOTLPScores returns nil.
Executed at this head, stub exporter failing only its first call, 600 pass rows:
ExportSpans calls=2 batch sizes=[512 88]
ExportOTLPScores err = <nil> <- 512 scores never landed
Control, 3 rows / single batch, same stub: otlp export failed for all 3 scores: simulated transport failure.... So the reporting path itself works; it is specifically the multi-batch case that loses the error. go test ./internal/evalmeasure/ ./internal/telemetry/ ./internal/cli/ is green at this head, so no existing test catches it.
Consequence chain in run.go: RecordScored (run.go:89) ledgers each row before exportScored() runs (run.go:98), and the ledger records "scored", not "exported". With ExportOTLPScores returning nil, stats.RemoteExportWarning stays empty, the CLI prints no warning (internal/cli/evalmeasure.go:130), and on a re-run AlreadyScored skips those rows — so the 512 scores are permanently absent from the backend. The only artifact is a bare otel.Handle line on stderr from the async drain, with no context and no effect on exit status, while the run reports success. That is a miss against the stated contract that OTLP failures warn via the CLI.
The bot raised the overwrite as [low] at line 223 (thread still unresolved) and dismissed it on two grounds; neither holds:
-
"The overwrite is intentional (tested by
TestCapturingExporter_ClearsErrorOnLaterSuccess)" — the rationale at lines 212-215 is "avoids false 'remote export failed' after data actually landed", but that scenario cannot arise here. Retries live inside a singleExportSpanscall (otlptracehttp.RetryConfigviaNewOTLPExporterBounded); the processor never redelivers a failed batch. sdk/trace v1.44.0exportSpanssays so outright: "A new batch is always created after exporting, even if the batch failed to be exported. It is up to the exporter to implement any type of retry logic." SuccessiveExportSpanscalls therefore carry disjoint span sets, so a later success never implies the earlier spans landed.TestCapturingExporter_ClearsErrorOnLaterSuccess(export_otlp_test.go:366) passesnilspans on both calls, which is exactly what hides that distinction — it locks the behaviour in rather than guarding it. -
"practical batch sizes (1-5 scores) make multi-batch splits implausible" — contradicted by this PR.
WithMaxQueueSize(len(exportable))exists, per its own comment at lines 132-136, because "the SDK's default queue is 2,048 and drops spans once full", andTestExportOTLPScores_ExportsMoreThanDefaultQueueSizeasserts 2,049 rows. A singleMeasureAndExporttoday is usually well under 512, so this is latent rather than everyday — but it is latent at exactly the scale this PR designs and tests for, and the failure mode is silent permanent loss.
Suggested fix: accumulate in the latch instead of overwriting — keep the errors.Joined batch errors plus a failed-span count, and feed that into the accounting at lines 177-181 so a partial cross-batch failure reports as partial rather than as success. Then invert TestCapturingExporter_ClearsErrorOnLaterSuccess, and add an ExportOTLPScores-level test with a stub that fails only its first call at >512 rows. Adding WithMaxExportBatchSize(len(exportable)) beside the queue size removes the split for a materialized set and is worth doing, but on its own it leaves the latch semantics wrong.
| fmt.Fprintf(os.Stderr, "FAIL: no gen_ai.evaluation.result events\n") | ||
| os.Exit(1) | ||
| } | ||
| fmt.Fprintf(os.Stderr, "PASS: %d score(s), %d OTLP event(s)\n", len(results), len(events)) |
There was a problem hiding this comment.
[MEDIUM] prove-otlp-scores prints PASS while RemoteExportWarning is non-empty, so the PR's own proof tool cannot detect a partial export failure
The three exit gates are len(results) == 0 (124), nReqs == 0 (128) and len(events) == 0 (132). stats.RemoteExportWarning is placed in the JSON report at line 115 but never gates the outcome, and the event count is printed beside the score count without being compared to it. A run where export failed for part of the batch — one malformed-ID row among good ones, or the multi-batch case in my other comment on export_otlp.go — still exits 0 with PASS.
That matters because this is the tool the PR body cites as evidence ("Local httptest OTLP sink proof against Review artifact"). It is a hack/ tool with no CI or Makefile wiring (grepped at head: no references outside its own directory), so the severity here is about the evidence value of the test plan rather than runtime behaviour — but as written, PASS does not mean what the test plan uses it to mean.
Suggested fix: fail when stats.RemoteExportWarning != "", and reconcile len(events) against the number of eligible results — results minus the rows ExportOTLPScores intentionally omits (suppressed-TraceID rows, and label: skip rows with no parent SpanID) — instead of only requiring at least one event.
2 participants
Summary
eval-measurements.jsonl, newly scored rows also emitgen_ai.evaluation.resultspan events on the same TraceID whenOTEL_EXPORTER_OTLP_*is set (same path as ADR 0050 agent traces).run-telemetry.jsonl.MLFLOW_*/ Assessments) in core — MLflow Assessments UI can be a separate consumer of the OTLP event.Closes #6458
Test plan
go test ./internal/evalmeasure/ ./internal/telemetry/84d470ba…)tr-84d470ba2451ffeccfe09022d9b2aebdeval-measureposts scores when OTEL is setMade with Cursor