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://48da3ef3-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}
| Fullsend does not pick an observability product for scores. The portable | ||
| contract is a local JSONL artifact next to telemetry; remote export reuses | ||
| the same OpenTelemetry (`OTEL_EXPORTER_OTLP_*`) configuration as agent | ||
| traces when implemented. | ||
| traces. |
There was a problem hiding this comment.
2. Guide not in admin/user 📜 Skill insight ⌂ Architecture
A modified guide exists under docs/guides/infrastructure/, but guides are required to live under either docs/guides/admin/ or docs/guides/user/. This breaks the required documentation directory structure.
Agent Prompt
## Issue description
The file `docs/guides/infrastructure/eval-measurements.md` is a documentation guide, but it is not placed under `docs/guides/admin/` or `docs/guides/user/` as required.
## Issue Context
Choose the correct audience (likely `admin/` for infrastructure/ops content) and move/rename the file accordingly, then update inbound links (e.g., from `docs/architecture.md` and glossary entries) to the new location.
## Fix Focus Areas
- docs/guides/infrastructure/eval-measurements.md[1-60]
- docs/architecture.md[321-325]
- docs/glossary.md[89-93]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
Previous runReviewFindingsLow
Previous run (2)ReviewFindingsLow
Info
Previous run (3)ReviewFindingsLow
Previous run (4)ReviewFindingsLow
Previous run (5)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).
| require.NoError(t, os.Mkdir(meas, 0o755)) | ||
| }) | ||
| results, _, err := MeasureAndExport(ctx, telem, filepath.Join("testdata", "sample-registry.yaml"), out) | ||
| results, _, err := MeasureAndExport(ctx, telem, filepath.Join("testdata", "sample-registry.yaml"), out, "") |
There was a problem hiding this comment.
[HIGH] — Tests here (and in internal/cli/evalmeasure_test.go) can export fixture scores to a real OTLP backend
MeasureAndExport/MeasureFile trigger ExportOTLPScores whenever OTEL_EXPORTER_OTLP_* is set in the process env. Neither run_test.go nor evalmeasure_test.go reference the clearOTLPEnv/pinOTELEnv hermeticity helpers already used in export_otlp_test.go and telemetry_test.go (the latter explicitly documents "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT may be set by org vars" in CI). Both files call MeasureAndExport/MeasureFile repeatedly — e.g. TestMeasureAndExport_KeepsFirstWhenSecondPersistFails right here produces a real scored result. If ambient OTEL env vars are present, fixture trace IDs (aaaaaaaa...) can leak to a real collector and retries can add flakiness.
Suggestion: Apply the existing clearOTLPEnv/pinOTELEnv pattern to every Measure*-calling test in these two files.
There was a problem hiding this comment.
apply this suggestion
There was a problem hiding this comment.
Fixed in 807faa1.
clearOTLPEnv is now applied in every MeasureFile/MeasureAndExport test in run_test.go, and the same helper was added for CLI evalmeasure_test.go paths that invoke measure. Ambient OTEL_EXPORTER_OTLP_* from CI org vars can no longer leak fixture TraceIDs to a real collector.
| } | ||
| } | ||
| if len(all) > 0 { | ||
| if err := ExportOTLPScores(ctx, all, serviceVersion); err != nil { |
There was a problem hiding this comment.
[HIGH] — Mid-loop persist error after a successful row permanently skips OTLP export for that row
The loop above (appending to all, then AppendMeasurements/RecordScored) persists+ledgers each result as it goes, but ExportOTLPScores is only called once here, after the whole loop completes. If AppendMeasurements or RecordScored fails on a later trace, the function returns early from inside the loop and never reaches this call, so an earlier trace that was already durably persisted+ledgered never gets exported. TestMeasureAndExport_KeepsFirstWhenSecondPersistFails exercises exactly this scenario (asserts results has the first trace and an error) but doesn't check/require OTLP export of that first row — confirming the gap is untested.
Suggestion: Export all (fail-open, consistent with the rest of the design) before returning on the mid-loop error path, or explicitly document that a partial-batch failure silently drops remote export for already-ledgered rows.
There was a problem hiding this comment.
apply this suggestion
There was a problem hiding this comment.
Fixed in 807faa1.
MeasureAndExport now calls fail-open ExportOTLPScores for the already-persisted prefix before returning on mid-loop persist/ledger errors (same helper as the success path). TestMeasureAndExport_KeepsFirstWhenSecondPersistFails asserts the first row actually hits a local OTLP sink before the early return.
|
|
||
| // newScoreOTLPExporter is a test seam. Production uses a retry-bounded | ||
| // exporter so Simple/Batch export cannot retry forever (unlike live agent | ||
| // Setup, which may leave MaxElapsedTime at the SDK default). |
There was a problem hiding this comment.
[MEDIUM] — Comment misdescribes agent Setup's OTLP exporter as capped at the SDK's default MaxElapsedTime
This comment says the bounded score exporter exists "unlike live agent Setup, which may leave MaxElapsedTime at the SDK default." In telemetry.go, NewOTLPExporter's RetryConfig sets InitialInterval/MaxInterval but never MaxElapsedTime, leaving it at the Go zero value (0). In the vendored otlptracehttp source, WithRetry replaces the whole RetryConfig with no merge against retry.DefaultConfig (MaxElapsedTime=1m), and the retry loop's give-up check is if maxElapsedTime != 0 && ... — so a zero value means retries never give up on elapsed time. Agent Setup's exporter is actually less bounded than the SDK's own 1-minute default, not equal to it as this comment implies.
Suggestion: Fix the comment to say agent Setup's exporter has no MaxElapsedTime cap at all (bounded only by ctx cancellation at shutdown), or explicitly set MaxElapsedTime to the SDK default in NewOTLPExporter if that was the real intent.
There was a problem hiding this comment.
explicitly set MaxElapsedTime to the SDK default in NewOTLPExporter if that was the real intent.
There was a problem hiding this comment.
Fixed in 807faa1.
Comment corrected: agent NewOTLPExporter leaves MaxElapsedTime at 0, which otlptracehttp treats as “never give up on elapsed time” (bounded only by shutdown ctx). Score export still uses the tighter NewOTLPExporterBounded path on purpose.
| // | ||
| // Attribute names follow OpenTelemetry GenAI semantic conventions | ||
| // (semantic-conventions-genai, evaluation events — pin consulted for this | ||
| // ship: https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-events/ |
There was a problem hiding this comment.
[MEDIUM] — Cited semconv gen-ai-events URL is a dead/moved page, not the current spec
This const-block comment cites https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-events/ as the "pin consulted for this ship" (the same URL is repeated in docs/guides/infrastructure/eval-measurements.md). A live fetch of that URL now serves: "GenAI semantic conventions have moved to the OpenTelemetry GenAI semantic conventions repository. This page has moved and is no longer maintained in this repository." This is the URL added in response to a prior review comment about premature-decision/instability, so the citation itself is already stale even after that fix.
Suggestion: Update the citation in both export_otlp.go and eval-measurements.md to the current source (github.com/open-telemetry/semantic-conventions-genai), and note the event's low-stability/reference-implementation status there.
There was a problem hiding this comment.
Fixed in 807faa1.
Citation updated in export_otlp.go and docs/guides/infrastructure/eval-measurements.md to the current GenAI repo pin: https://github.com/open-telemetry/semantic-conventions-genai/blob/main/reference/reports/gen-ai-evaluation-result-event.md (noted as low-stability / reference).
| if !telemetry.OTLPEnabled() { | ||
| return nil | ||
| } | ||
| if inboundTRACEPARENTUnsampled() { |
There was a problem hiding this comment.
[MEDIUM] — Job-level TRACEPARENT-unsampled gate doesn't account for multiple TraceIDs in one telemetry file
ParseTelemetryFile groups spans by TraceID into a map, so a single run-telemetry.jsonl can hold multiple distinct traces — realistic since telemetry.go opens the file with os.O_APPEND, meaning repeated fullsend run invocations sharing an output dir append rather than truncate. MeasureAndExport accumulates every scored row across all traces into one all slice and passes it to a single ExportOTLPScores call, which applies one process-global inboundTRACEPARENTUnsampled() check here based only on the current process's own TRACEPARENT env var, not per-row/per-TraceID sampled status. A file holding traces from more than one prior invocation with different sampled decisions gets one ambient gate applied to all of them, which can either wrongly suppress export of an actually-sampled trace or export a score span onto a TraceID whose agent spans were never sent remotely (an orphan) — the exact failure mode this check exists to prevent.
Suggestion: Gate suppression per-TraceID rather than one job-level env check, or explicitly document/enforce a 1:1 telemetry-file-to-sampled-decision assumption if that is guaranteed elsewhere.
There was a problem hiding this comment.
Fixed in 807faa1 (narrowed, with managed-path context).
Managed fullsend run does not append two runs into one run-telemetry.jsonl — each run gets a unique output/fs-<slug>-<hash>/. The gate is still TraceID-scoped for defense in depth: when inbound TRACEPARENT is valid+unsampled, we suppress only scores whose trace_id equals that parent TraceID; other TraceIDs in the batch still export. Guide documents the 1:1 runDir assumption. Parsing now uses propagation.TraceContext{}.Extract (same as agent run).
| err := c.base.ExportSpans(ctx, spans) | ||
| if err != nil { | ||
| c.mu.Lock() | ||
| if c.err == nil { |
There was a problem hiding this comment.
[MEDIUM] — capturingExporter latches the first ExportSpans error even after a later flush succeeds
ExportSpans stores the first non-nil error in c.err (if c.err == nil { c.err = err }) and never clears it on a subsequent successful call. Combined with the errors.Join after ForceFlush, a transient failure on an early batch followed by a successful later batch still results in stats.RemoteExportWarning being set. Since the ledger already marks these rows as scored (no retry path), the operator gets a false "remote export failed" signal for data that actually landed.
Suggestion: Only report export failure if ForceFlush/Shutdown ultimately still report failure, or clear/overwrite the latched error on a later successful ExportSpans call.
There was a problem hiding this comment.
Fixed in 807faa1.
capturingExporter now stores the latest ExportSpans error (nil on success), so a later successful batch clears a prior transient failure. Added TestCapturingExporter_ClearsErrorOnLaterSuccess.
| if stats.SkippedSpans > 0 { | ||
| printer.StepWarn(fmt.Sprintf("%s: skipped %d unreadable span(s) inside otherwise-valid telemetry line(s)", p, stats.SkippedSpans)) | ||
| } | ||
| if stats.RemoteExportWarning != "" { |
There was a problem hiding this comment.
[MEDIUM] — CLI RemoteExportWarning branch has no dedicated test
No test under internal/cli references RemoteExportWarning or the "OTLP score export failed" message, even though this is new production control flow introduced by this PR. Coverage exists only one layer down at TestMeasureAndExport_OTLPFailOpen in the evalmeasure package.
Suggestion: Add a runEvalMeasure-level test pointed at a closed/unreachable OTLP endpoint (with OTEL env pinned) asserting the CLI-level StepWarn fires.
There was a problem hiding this comment.
Fixed in 807faa1.
Added TestRunEvalMeasure_OTLPFailWarns: pins OTEL to a closed port and asserts the CLI StepWarn path emits OTLP score export failed while local JSONL is still written.
| // inboundTRACEPARENTUnsampled reports whether TRACEPARENT is present and | ||
| // carries the W3C sampled flag cleared (…-00). Empty/malformed TRACEPARENT | ||
| // is treated as "no inbound parent" (export proceeds). | ||
| func inboundTRACEPARENTUnsampled() bool { |
There was a problem hiding this comment.
[MEDIUM] — Hand-rolled TRACEPARENT parsing can diverge from the agent's W3C propagator
inboundTRACEPARENTUnsampled manually splits the TRACEPARENT env var on - and checks the flags byte, while the agent run path (internal/cli/run.go) uses propagation.TraceContext{}.Extract for the same env var. These are two independent implementations of W3C traceparent parsing; they can silently diverge on edge cases (all-zero IDs, malformed flag fields, future header versions), causing score-export sampling suppression to drift from the agent's own sampling decision over time.
Suggestion: Reuse propagation.TraceContext{}.Extract (the same propagator the agent path uses) instead of a bespoke string-split parser.
There was a problem hiding this comment.
Fixed in 807faa1.
Replaced the hand-rolled TRACEPARENT string split with propagation.TraceContext{}.Extract + SpanContextFromContext (same propagator path as resolveTraceIdentity in run.go), then apply TraceID-scoped unsampled suppression from that context.
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>
|
🤖 Finished Review · ✅ Success · Started 7:56 PM UTC · Completed 8:11 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.36 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep: 4 findings (all MEDIUM), inline below.
Fold Shutdown into the shared export budget and surface its errors, honor OTEL attribute value limits for evaluation explanations, report partial export as N/M in warnings, and stop prove-otlp-scores from wiping ledger state beside live telemetry by default. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 5:17 PM UTC · Completed 5:35 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.00 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep: 3 findings (all MEDIUM), inline below. Verified against head 8278aeef.
| if len(errs) == 0 { | ||
| return nil | ||
| } | ||
| exported := attempted - failed |
There was a problem hiding this comment.
[MEDIUM] The new "N/M scores exported" warning reports total success for the dominant (transport) failure mode
failed (line 136) is incremented only inside the per-row loop and only when exportOneScore returns an error, which it can do exclusively on trace_id/span_id parse failures (lines 212-219). The two transport-level failure signals — tp.ForceFlush(ctx) (line 140) and the captured capExp.err (lines 143-148) — are appended to errs but never adjust attempted or failed. So exported := attempted - failed (line 152) still counts every row whose span was merely constructed, not delivered.
With the collector unreachable and zero spans landing, the operator-facing warning reads 1/1 scores exported; 0 failed: <flush err>; otlp export: <err> — it declares total success and total failure in the same sentence. That makes both the round-3 fix (commit 8278aee, reply on thread 3855447436: "Partial/total failures now report as N/M scores exported; K failed") and the doc comment at lines 77-78 ("Partial ID failures report N/M scores exported so operators can tell partial from total failure") untrue for the most common failure.
The test gap is visible: TestExportOTLPScores_PartialIDFailureReportsCounts (export_otlp_test.go:391) asserts the count text only against a healthy httptest sink, while the two closed-port tests that do exercise transport failure — TestMeasureAndExport_OTLPFailOpen (export_otlp_test.go:265) and TestRunEvalMeasure_OTLPFailWarns (internal/cli/evalmeasure_test.go:543) — assert nothing about the count text.
Suggestion: Treat a transport error as invalidating the per-row tally: when flushErr != nil or expErr != nil, either report 0/%d scores exported (nothing is known to have landed) or drop the N/M prefix entirely for that branch and emit a distinct message such as otlp export failed for all %d scores. Reserve N/M for the pure ID-failure path. Add an assertion on the warning text in the closed-port test so the two failure classes cannot silently converge again.
There was a problem hiding this comment.
Fixed in c530e9d.
Transport failures (ForceFlush / capturingExporter err) now emit a distinct otlp export failed for all N scores: … warning instead of claiming N/M scores exported from locally constructed spans. The N/M form is reserved for pure ID-parse failures. Closed-port tests (TestMeasureAndExport_OTLPFailOpen, TestRunEvalMeasure_OTLPFailWarns) assert the new transport wording.
| require.NotEmpty(t, stats.RemoteExportWarning, "expected OTLP failure warning with local JSONL kept") | ||
| } | ||
|
|
||
| func TestExportOTLPScores_UnsampledTRACEPARENTNoop(t *testing.T) { |
There was a problem hiding this comment.
[MEDIUM] No positive test for a sampled (-01) TRACEPARENT, so silently dropping the IsSampled clause would go undetected on the common path
clearOTLPEnv (line 77) sets TRACEPARENT to "" for every test, so all tests except the two TRACEPARENT ones exit inboundUnsampledTRACEPARENT at the empty-string guard (export_otlp.go:162) and never reach the sc.IsSampled() check at export_otlp.go:170. The only two tests that set TRACEPARENT — TestExportOTLPScores_UnsampledTRACEPARENTNoop (line 284) and TestExportOTLPScores_UnsampledTRACEPARENTOtherTraceIDExports (line 298) — both use the -00 (unsampled) flag.
To be precise: a full inversion of the clause would be caught, because the -00 Noop test would then see spans in the sink and fail its assert.Empty. The genuinely uncovered mutation is dropping or weakening the sc.IsSampled() clause: suppression would then fire for a sampled -01 parent too, silently killing score export for the normal dispatched-pipeline case (where fullsend run adopts a live sampled TRACEPARENT), while both existing -00 tests stay green. That is the highest-traffic production path for this feature and it currently has no positive assertion.
Suggestion: Add TestExportOTLPScores_SampledTRACEPARENTExports: set TRACEPARENT to 00-84d470ba2451ffeccfe09022d9b2aebd-77f8c0902eaeedcb-01 with a score row on that same TraceID, and assert sink.allSpans() is non-empty. This pins the common path against the clause being weakened, mirroring the existing agent-tracing test pattern in internal/cli/telemetry_run_test.go.
There was a problem hiding this comment.
Fixed in c530e9d.
Added TestExportOTLPScores_SampledTRACEPARENTExports with TRACEPARENT …-01 on the same TraceID as the score row, asserting the sink is non-empty so dropping/weakening sc.IsSampled() would fail the common path.
| })) | ||
| defer srv.Close() | ||
|
|
||
| _ = os.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", srv.URL) |
There was a problem hiding this comment.
[MEDIUM] hack/prove-otlp-scores leaves TRACEPARENT ambient, so it can report FAIL while the export path works
The tool deliberately normalizes the OTLP environment before measuring — os.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", srv.URL) (line 90), os.Unsetenv("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT") (line 91), os.Unsetenv("OTEL_SDK_DISABLED") (line 92) — but never clears TRACEPARENT/TRACESTATE. ExportOTLPScores reads those directly from the process env via inboundUnsampledTRACEPARENT (export_otlp.go:161-167).
The suppression gate is now TraceID-scoped, so the false negative needs the ambient TRACEPARENT to be both unsampled and to carry the same TraceID as the run being scored — which is exactly this tool's natural usage: do a fullsend run under an unsampled inbound parent, then prove against that run dir from the same shell. In that case every score row is skipped at export_otlp.go:125, reqs stays empty, and the tool prints FAIL: no OTLP requests received and exits 1 (lines 124-126) even though the export path is behaving exactly as designed. That is a false negative in the one tool whose entire purpose is to prove the path works — and which the PR body cites as dogfood evidence.
Suggestion: Add os.Unsetenv("TRACEPARENT") and os.Unsetenv("TRACESTATE") alongside the existing unsets at lines 91-92, so the tool controls the full set of inputs that gate export. Failing that, print the resolved suppression decision before the FAIL line so an operator can tell suppression apart from a broken export path.
There was a problem hiding this comment.
Fixed in c530e9d.
hack/prove-otlp-scores now Unsetenvs TRACEPARENT and TRACESTATE alongside the existing OTEL unsets, so an ambient unsampled parent from a prior fullsend run in the same shell cannot suppress every score and produce a false FAIL.
Distinguish transport failures from partial ID failures in warnings, add a sampled TRACEPARENT positive export test, clear ambient TRACEPARENT in prove-otlp-scores, and replace hand-rolled hexOf with encoding/hex. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 6:44 PM UTC · Completed 7:02 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.35 |
|
Risk Assessment: moderate (2/5) DetailsModerate risk: large change size (16 files, 1242 lines) offset by no protected paths, no security-sensitive files, strong test ratio, and well-aligned issue scope. Previous runRisk Assessment: moderate (2/5) DetailsModerate risk. Large additive change (1230 lines, 16 files) drives elevated change-size signal, but no protected paths or security-sensitive files are touched. Feature is gated behind an environment variable (OTEL_EXPORTER_OTLP_ENDPOINT), making it opt-in and safely rollbackable. PR aligns well with issue #6458 acceptance criteria. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep: 1 finding (MEDIUM), inline below. Verified against head c530e9da.
Repoint citations from the support-matrix report to gen-ai-events.md and document that fullsend emits a span event while the convention specifies a log record. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 8:38 PM UTC · Completed 8:57 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.88 |
| } | ||
|
|
||
| func (c *capturingExporter) ExportSpans(ctx context.Context, spans []sdktrace.ReadOnlySpan) error { | ||
| err := c.base.ExportSpans(ctx, spans) |
There was a problem hiding this comment.
[low] error-handling
capturingExporter.ExportSpans overwrites c.err on every call. If the BatchSpanProcessor calls ExportSpans multiple times, only the last call's error is retained. The overwrite is intentional (tested by TestCapturingExporter_ClearsErrorOnLaterSuccess) and practical batch sizes (1-5 scores) make multi-batch splits implausible.
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(ctx, otlpExportBudget) | ||
| defer cancel() |
There was a problem hiding this comment.
[low] edge-case
ExportOTLPScores shares the otlpExportBudget (15s) context between ForceFlush and the deferred Shutdown. If ForceFlush consumes most of the deadline, Shutdown may receive an expired context. Harmless since ForceFlush already flushed spans, and the error is additive (errors.Join, not replacing).
|
|
||
| // clearOTLPEnv keeps Measure*/eval-measure tests hermetic when CI injects | ||
| // OTEL_EXPORTER_OTLP_* org vars (same pattern as evalmeasure/export_otlp_test). | ||
| func clearOTLPEnv(t *testing.T) { |
There was a problem hiding this comment.
[low] naming-conventions
clearOTLPEnv in this file clears 6 env vars while the identically-named function in internal/evalmeasure/export_otlp_test.go clears 8 (also blanking OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT and OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT). No cli test exercises attribute truncation, so the risk is theoretical.
fullsend-ai-review
Bot
added
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep — 1 new finding, inline. Everything else on this PR was already covered by an existing thread.
| // so event explanations honor the same OTEL_* override as agent span attrs. | ||
| // SpanLimits already maps an unset env to MaxSpanAttrValueLen; a configured | ||
| // negative sentinel means unlimited (truncateRunes no-ops). | ||
| func eventAttrValueLenLimit() int { |
There was a problem hiding this comment.
[MEDIUM] Explanation truncation silently becomes unbounded when Level 3 content capture is on
eventAttrValueLenLimit() (lines 298-300) returns telemetry.SpanLimits().AttributeValueLengthLimit, and that value is fed to truncateRunes(r.Explanation, ...) at the gen_ai.evaluation.explanation call site (line 264). Verified against the head SHA by reading all three files:
telemetry.SpanLimits()→spanLimits()(telemetry.go:159-173).sdktrace.NewSpanLimits()leavesAttributeValueLengthLimitnegative; the function assignsMaxSpanAttrValueLen(8192) only when content capture is off. WhenContentCaptureEnabled()is true it deliberatelyreturn limitswith the negative sentinel untouched, so JSON content span attrs are not cut mid-string.ContentCaptureEnabled()(content.go:29-36) is true whenOTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENTistrue,span_only, orspan_and_event.truncateRunes(s, max)(lines 281-283) returnssunchanged whenmax < 0.
So whenever that env var holds an affirmative value in the eval-measure process env and no OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT / OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT is set, the explanation goes onto the OTLP wire with no size cap at all. Because SDK span limits do not apply to event attributes (the code's own comment at lines 260-263 says so), nothing else bounds it.
This contradicts three statements in this PR's own code:
- The call-site comment claiming the explanation is bounded "using the same effective limit
SpanLimits()applies for span attrs". eventAttrValueLenLimit's doc, which attributes the unlimited case to "a configured negative sentinel" — but here the negative comes from the content-capture branch, not from any operator configuration.telemetry.MaxSpanAttrValueLen's doc block, which states the convention that when Level 3 lifts the provider cap, "free-text values that relied on this cap are bounded at their call sites instead". The explanation is exactly such a free-text call site, and its call-site bound reuses the lifted value — circular, so the documented safety net does not exist here.
This is a follow-on defect in the fix for the resolved thread on OTEL_* limit honoring (which asked for a fallback "when that resolves to the SDK's own unset/negative sentinel" — the fallback was not implemented, and the content-capture trigger was never discussed there). Latent today because EM-001 explanations are short, but any future scorer with longer explanations rides unbounded whenever Level 3 capture is enabled.
Suggestion: Do not reuse the content-capture-aware SpanLimits() for the event-attribute bound. Either (a) resolve the limit as operatorAttrValueLimit() when set, else telemetry.MaxSpanAttrValueLen (an explicit operator -1 still meaning unlimited), or (b) fall back to MaxSpanAttrValueLen in eventAttrValueLenLimit() whenever SpanLimits().AttributeValueLengthLimit is negative and no operator limit is configured. Then fix the eventAttrValueLenLimit doc to say the unlimited case requires an operator-configured -1, and add a test that sets OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT=true with a long explanation and asserts it is still truncated.
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