Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/insert-grader-results-otel.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

58 changes: 57 additions & 1 deletion actions/setup/js/send_otlp_span.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,52 @@ function buildExperimentAttributes(assignments) {
return attrs;
}

/**
* Build summary attributes and per-result events from valid deterministic grader output.
* Free-form grader messages, details, and errors are intentionally excluded because
* custom graders may derive them from trace content containing sensitive values.
*
* @param {any} graderOutput
* @param {number} eventTimeMs
* @returns {{attributes: Array<{key: string, value: object}>, events: Array<{timeUnixNano: string, name: string, attributes: Array<{key: string, value: object}>}>}}
*/
function buildGraderTelemetry(graderOutput, eventTimeMs) {

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.

actions/setup/js/send_otlp_span.cjs:720: yagni: buildGraderTelemetry is a one-call-site helper that just reshapes a small object into attrs/events. Inline this logic in sendJobConclusionSpan and drop the extra indirection.

if (!graderOutput || typeof graderOutput !== "object" || !Array.isArray(graderOutput.results) || graderOutput.results.length === 0) {
return { attributes: [], events: [] };
}

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.

[/grill-with-docs] The graderOutput.results array is filtered for valid entries, but the original graderOutput.results.length (including null/invalid entries) could theoretically differ from results.length used for gh-aw.graders.count — which would make the count inconsistent with the number of events emitted.

💡 What this means

If graderOutput.results contains [null, { id: 'quality', ... }], gh-aw.graders.count will be 1 (post-filter), which is correct. However, the filtering happens silently — an upstream bug that produces null results won't surface anywhere. Consider adding a gh-aw.graders.skipped attribute or at least documenting that count reflects valid results only, not total entries.

This is a minor observability gap, not a bug, but worth a comment in the JSDoc.

@copilot please address this.

const results = graderOutput.results.filter(result => result && typeof result === "object" && typeof result.id === "string" && result.id);
if (results.length === 0) {
return { attributes: [], events: [] };
}

const countByStatus = status => results.filter(result => result.status === status).length;
const attributes = [

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.

Potential metric invariant breakgh-aw.graders.count reflects all valid results, but the four per-status counters (passed, failed, errors, unavailable) only cover the currently-known set of status values. If a new status is introduced (e.g., "skipped", "timeout"), count will exceed passed + failed + errors + unavailable, silently producing inconsistent dashboards/alerts.

Consider either:

  • adding a catch-all counter (e.g., gh-aw.graders.other) for unrecognised statuses, or
  • asserting in tests that count === passed + failed + errors + unavailable for known inputs so future status additions are surfaced immediately.

@copilot please address this.

buildAttr("gh-aw.graders.count", results.length),
buildAttr("gh-aw.graders.passed", countByStatus("pass")),
buildAttr("gh-aw.graders.failed", countByStatus("fail")),
buildAttr("gh-aw.graders.errors", countByStatus("error")),
buildAttr("gh-aw.graders.unavailable", countByStatus("unavailable")),
buildAttr("gh-aw.graders.other", results.length - countByStatus("pass") - countByStatus("fail") - countByStatus("error") - countByStatus("unavailable")),
];
const timeUnixNano = toNanoString(eventTimeMs);
const events = results.map(result => {
const resultAttributes = [buildAttr("gh-aw.grader.id", result.id)];
if (typeof result.name === "string" && result.name) resultAttributes.push(buildAttr("gh-aw.grader.name", result.name));
if (typeof result.status === "string" && result.status) resultAttributes.push(buildAttr("gh-aw.grader.status", result.status));
if (typeof result.source === "string" && result.source) resultAttributes.push(buildAttr("gh-aw.grader.source", result.source));
if (typeof result.unit === "string" && result.unit) resultAttributes.push(buildAttr("gh-aw.grader.unit", result.unit));
if (typeof result.value === "number" && Number.isFinite(result.value)) resultAttributes.push(buildDoubleAttr("gh-aw.grader.value", result.value));
if (typeof result.passed === "boolean") resultAttributes.push(buildAttr("gh-aw.grader.passed", result.passed));
if (typeof result.severity === "string" && result.severity) resultAttributes.push(buildAttr("gh-aw.grader.severity", result.severity));
if (typeof result.baselineValue === "number" && Number.isFinite(result.baselineValue)) resultAttributes.push(buildDoubleAttr("gh-aw.grader.baseline_value", result.baselineValue));
if (typeof result.deltaFromBaseline === "number" && Number.isFinite(result.deltaFromBaseline)) resultAttributes.push(buildDoubleAttr("gh-aw.grader.delta_from_baseline", result.deltaFromBaseline));
return { timeUnixNano, name: "grader.result", attributes: resultAttributes };
});

return { attributes, events };
}

// ---------------------------------------------------------------------------
// Custom OTLP attributes (GH_AW_OTLP_ATTRIBUTES)
// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -1991,6 +2037,8 @@ function readAgentRuntimeMetrics() {
* - `/tmp/gh-aw/agent_usage.json` – per-type token breakdown written by parse_token_usage.cjs;
* provides `input_tokens`, `output_tokens`,
* `cache_read_tokens`, and `cache_write_tokens` counters
* - `/tmp/gh-aw/agent/graders/grader_results.json` – deterministic grader
* summary attributes and per-result span events
*
* @param {string} spanName - OTLP span name (e.g. `"gh-aw.job.conclusion"`)
* @param {{ startMs?: number }} [options]
Expand Down Expand Up @@ -2327,6 +2375,8 @@ async function sendJobConclusionSpan(spanName, options = {}) {
}
}

const graderTelemetry = jobName === "agent" ? buildGraderTelemetry(readJSONIfExists("/tmp/gh-aw/agent/graders/grader_results.json"), endMs) : { attributes: [], events: [] };

const resourceAttributes = buildGitHubActionsResourceAttributes({
repository,
runId,
Expand Down Expand Up @@ -2385,7 +2435,7 @@ async function sendJobConclusionSpan(spanName, options = {}) {
});
};

const spanEvents = buildSpanEvents(endMs);
const spanEvents = [...buildSpanEvents(endMs), ...graderTelemetry.events];

// Prefer the timestamp written at the very beginning of the Execute Agent CLI step
// (captures true step start on the host, before the AWF container launches) so the
Expand Down Expand Up @@ -2480,6 +2530,11 @@ async function sendJobConclusionSpan(spanName, options = {}) {
}
}

// Grader results are run-level outcomes. They belong only on the agent job's
// conclusion span, rather than its dedicated child span or downstream jobs
// which may have downloaded the agent artifact.
attributes.push(...graderTelemetry.attributes);

// Only attach token-usage attributes to jobs that actually executed model usage.
// Most downstream jobs (conclusion, safe_outputs) may have agent_usage.json on
// disk via artifact download but must NOT emit token data — otherwise every
Expand Down Expand Up @@ -2564,6 +2619,7 @@ module.exports = {
OTEL_JSONL_PATH,
appendToOTLPJSONL,
buildExperimentAttributes,
buildGraderTelemetry,
parseOTLPCustomAttributes,
buildCustomOTLPAttributes,
};
119 changes: 119 additions & 0 deletions actions/setup/js/send_otlp_span.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ const {
buildCurrentWorkflowCallId,
buildEpisodeAttributesFromContext,
buildExperimentAttributes,
buildGraderTelemetry,
hasProxyConfigured,
resolveEngineId,
parseOTLPCustomAttributes,
Expand Down Expand Up @@ -2644,6 +2645,30 @@ describe("sendJobConclusionSpan", () => {
expect(span.spanId).toMatch(/^[0-9a-f]{16}$/);
});

it("emits graders only on the agent job conclusion span", async () => {
const mockFetch = vi.fn().mockResolvedValue({ ok: true, status: 200, statusText: "OK" });
vi.stubGlobal("fetch", mockFetch);
process.env.GH_AW_OTLP_ENDPOINTS = JSON.stringify([{ url: "https://traces.example.com" }]);
process.env.INPUT_JOB_NAME = "agent";
const readFileSpy = vi.spyOn(fs, "readFileSync").mockImplementation(filePath => {
if (filePath === "/tmp/gh-aw/agent/graders/grader_results.json") {
return JSON.stringify({ results: [{ id: "quality", status: "pass", value: 0.9 }] });
}
throw Object.assign(new Error("ENOENT"), { code: "ENOENT" });
});

await sendJobConclusionSpan("gh-aw.agent.conclusion");
process.env.INPUT_JOB_NAME = "conclusion";
await sendJobConclusionSpan("gh-aw.conclusion.conclusion");
readFileSpy.mockRestore();

const spans = mockFetch.mock.calls.map(([, request]) => JSON.parse(request.body).resourceSpans[0].scopeSpans[0].spans[0]);
expect(spans[0].attributes).toContainEqual(buildAttr("gh-aw.graders.count", 1));
expect(spans[0].events).toContainEqual(expect.objectContaining({ name: "grader.result" }));
expect(spans[1].attributes.map(attribute => attribute.key)).not.toContain("gh-aw.graders.count");
expect((spans[1].events ?? []).map(event => event.name)).not.toContain("grader.result");
});

it("emits live episode attributes on conclusion spans from aw_info workflow_call context", async () => {
const mockFetch = vi.fn().mockResolvedValue({ ok: true, status: 200, statusText: "OK" });
vi.stubGlobal("fetch", mockFetch);
Expand Down Expand Up @@ -2692,6 +2717,9 @@ describe("sendJobConclusionSpan", () => {
if (filePath === "/tmp/gh-aw/agent_output.json") {
return JSON.stringify({ items: [{ type: "issue" }, { type: "pull_request" }] });
}
if (filePath === "/tmp/gh-aw/agent/graders/grader_results.json") {
return JSON.stringify({ results: [{ id: "quality", status: "pass" }] });
}
throw Object.assign(new Error("ENOENT"), { code: "ENOENT" });
});

Expand All @@ -2716,6 +2744,9 @@ describe("sendJobConclusionSpan", () => {
expect(conclusionSpan.parentSpanId).toBe("abcdef1234567890");
expect(agentSpan.attributes).toContainEqual({ key: "gh-aw.output.item_count", value: { intValue: 2 } });
expect(conclusionSpan.attributes).toContainEqual({ key: "gh-aw.output.item_count", value: { intValue: 2 } });
expect(agentSpan.attributes.map(attribute => attribute.key)).not.toContain("gh-aw.graders.count");
expect(conclusionSpan.attributes).toContainEqual(buildAttr("gh-aw.graders.count", 1));
expect(conclusionSpan.events).toContainEqual(expect.objectContaining({ name: "grader.result" }));
const agentKeys = agentSpan.attributes.map(a => a.key);
const conclusionKeys = conclusionSpan.attributes.map(a => a.key);
expect(agentKeys).not.toContain("gh-aw.max_ai_credits");
Expand Down Expand Up @@ -6449,6 +6480,94 @@ describe("sendJobConclusionSpan", () => {
});
});

// ---------------------------------------------------------------------------
// buildGraderTelemetry
// ---------------------------------------------------------------------------

describe("buildGraderTelemetry", () => {
it("builds summary attributes and one event per grader result", () => {
const telemetry = buildGraderTelemetry(
{
results: [
{
id: "quality",
name: "Quality",
value: 0.75,
unit: "ratio",
passed: true,
status: "pass",
source: "builtin",
severity: "info",
baselineValue: 0.5,
deltaFromBaseline: 0.25,
},
{ id: "reliability", name: "Reliability", value: null, passed: false, status: "fail", source: "inline" },
{ id: "broken", status: "error", source: "inline" },
{ id: "missing", status: "unavailable", source: "builtin" },
],
},
1700000000000
);

expect(telemetry.attributes).toEqual([
buildAttr("gh-aw.graders.count", 4),
buildAttr("gh-aw.graders.passed", 1),
buildAttr("gh-aw.graders.failed", 1),
buildAttr("gh-aw.graders.errors", 1),
buildAttr("gh-aw.graders.unavailable", 1),
buildAttr("gh-aw.graders.other", 0),
]);
expect(telemetry.events).toHaveLength(4);
expect(telemetry.events[0]).toEqual({
timeUnixNano: toNanoString(1700000000000),
name: "grader.result",
attributes: [
buildAttr("gh-aw.grader.id", "quality"),
buildAttr("gh-aw.grader.name", "Quality"),
buildAttr("gh-aw.grader.status", "pass"),
buildAttr("gh-aw.grader.source", "builtin"),
buildAttr("gh-aw.grader.unit", "ratio"),
buildDoubleAttr("gh-aw.grader.value", 0.75),
buildAttr("gh-aw.grader.passed", true),
buildAttr("gh-aw.grader.severity", "info"),
buildDoubleAttr("gh-aw.grader.baseline_value", 0.5),
buildDoubleAttr("gh-aw.grader.delta_from_baseline", 0.25),
],
});
});

it("omits free-form and non-finite grader values", () => {
const telemetry = buildGraderTelemetry(
{
results: [
{
id: "custom",
status: "error",
value: Number.NaN,
message: "sensitive message",
details: "sensitive details",
error: "sensitive error",
},
],
},
1
);

const eventKeys = telemetry.events[0].attributes.map(attribute => attribute.key);
expect(eventKeys).toEqual(["gh-aw.grader.id", "gh-aw.grader.status"]);
expect(JSON.stringify(telemetry)).not.toContain("sensitive");
});

it.each([null, {}, { results: [] }, { results: [null, {}, { id: "" }] }])("returns empty telemetry for output without valid results", output => {
expect(buildGraderTelemetry(output, 1)).toEqual({ attributes: [], events: [] });
});

it("counts unrecognized statuses as other", () => {
const telemetry = buildGraderTelemetry({ results: [{ id: "skipped", status: "skipped" }] }, 1);
expect(telemetry.attributes).toContainEqual(buildAttr("gh-aw.graders.other", 1));
});
});

// ---------------------------------------------------------------------------
// parseOTLPEndpoints

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.

[/tdd] No call-site integration test covers sendJobConclusionSpan wiring grader attributes/events into the span — only the unit function is exercised.

💡 Suggested integration test to add in the `sendJobConclusionSpan` describe block

The existing sendJobConclusionSpan block already mocks readJSONIfExists. Adding one case would close the gap:

it("includes grader attributes and events when grader_results.json exists", async () => {
  mockReadJSONIfExists.mockImplementation((path) => {
    if (path.includes("grader_results.json"))
      return { results: [{ id: "quality", status: "pass", value: 0.9 }] };
    return null;
  });
  // assert span attributes contain gh-aw.graders.count
  // assert span events contain a grader.result event
});

A regression at the assembly step (e.g., reordering attributes/events in sendJobConclusionSpan) wouldn't be caught by the unit tests alone.

@copilot please address this.

// ---------------------------------------------------------------------------
Expand Down
3 changes: 1 addition & 2 deletions actions/setup/js/trace_graders.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -524,8 +524,7 @@ function normalizeResult(id, rawResult, meta) {
if (typeof rawResult === "object" && rawResult !== null && !Array.isArray(rawResult)) {
// Object result from custom script
value = rawResult.value;
if (rawResult.unit) base.unit = String(rawResult.unit);
if (rawResult.severity) base.severity = String(rawResult.severity);
if (typeof rawResult.severity === "string" && ["error", "warning", "info", "note"].includes(rawResult.severity)) base.severity = rawResult.severity;
if (rawResult.details) base.details = String(rawResult.details);
if (rawResult.message) base.message = String(rawResult.message);
if (typeof rawResult.passed === "boolean") base.passed = rawResult.passed;
Expand Down
11 changes: 8 additions & 3 deletions actions/setup/js/trace_graders.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -415,14 +415,19 @@ describe("trace_graders", () => {
expect(r.status).toBe("fail");
});

it("handles object results from custom scripts", () => {
it("uses manifest metadata for object results from custom scripts", () => {
const r = normalizeResult("test", { value: 42, unit: "ms", severity: "warning", details: "too slow" }, { ...meta, source: "inline" });
expect(r.value).toBe(42);
expect(r.unit).toBe("ms");
expect(r.unit).toBe("count");
expect(r.severity).toBe("warning");
expect(r.details).toBe("too slow");
});

it("omits an unrecognized custom severity", () => {
const r = normalizeResult("test", { value: 42, severity: "sensitive trace content" }, { ...meta, source: "inline" });
expect(r.severity).toBeUndefined();
});

it("handles null result as unavailable", () => {
const r = normalizeResult("test", null, meta);
expect(r.status).toBe("unavailable");
Expand Down Expand Up @@ -572,7 +577,7 @@ describe("trace_graders", () => {
}
`;
const trace = makeTrace({ toolCalls: [{ name: "a" }, { name: "b" }] });
const result = runCustomGrader("test", script, trace, meta);
const result = runCustomGrader("test", script, trace, { ...meta, unit: "count" });
expect(result.value).toBe(2);
expect(result.unit).toBe("count");
});
Expand Down
Loading