Repository navigation
Precompute usage-artifact activity aggregates and include GitHub API rate limit data for usage-only reporting #40504
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| package cli | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| ) | ||
|
|
||
| type usageActivitySummary struct { | ||
| Schema string `json:"schema,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. [/grill-with-docs] The 💡 Suggested guardAdd a schema version check after parsing: const usageActivitySummarySchema = "usage-activity-summary/v1"
// in loadUsageActivitySummary, after json.Unmarshal:
if summary.Schema != "" && summary.Schema != usageActivitySummarySchema {
return nil, fmt.Errorf("unsupported usage activity summary schema %q (expected %q)", summary.Schema, usageActivitySummarySchema)
}Since the schema is producer-controlled by this repo's conclusion job, a hard error is safe here — if the constant falls out of sync it will surface immediately in tests rather than silently producing wrong activity counts. |
||
| Firewall *usageActivityFirewall `json:"firewall,omitempty"` | ||
| Session *usageActivitySession `json:"session,omitempty"` | ||
| Gateway *usageActivityGateway `json:"gateway,omitempty"` | ||
| } | ||
|
|
||
| type usageActivityFirewall struct { | ||
| TotalRequests int `json:"total_requests"` | ||
| AllowedRequests int `json:"allowed_requests"` | ||
| BlockedRequests int `json:"blocked_requests"` | ||
| } | ||
|
|
||
| type usageActivitySession struct { | ||
| TotalEvents int `json:"total_events"` | ||
| SessionStarts int `json:"session_starts"` | ||
| SessionShutdowns int `json:"session_shutdowns"` | ||
| Turns int `json:"turns"` | ||
| AssistantMessages int `json:"assistant_messages"` | ||
| ReasoningEvents int `json:"reasoning_events"` | ||
| ToolExecutionStarts int `json:"tool_execution_starts"` | ||
| ToolExecutionCompletes int `json:"tool_execution_completes"` | ||
| FailedToolExecutions int `json:"failed_tool_executions"` | ||
| } | ||
|
|
||
| type usageActivityGateway struct { | ||
| TotalCalls int `json:"total_calls"` | ||
| FailedCalls int `json:"failed_calls"` | ||
| Servers []usageActivityGatewayServer `json:"servers,omitempty"` | ||
| } | ||
|
|
||
| type usageActivityGatewayServer struct { | ||
| ServerName string `json:"server_name"` | ||
| ToolCallCount int `json:"tool_call_count"` | ||
| FailedCalls int `json:"failed_calls"` | ||
| } | ||
|
|
||
| func loadUsageActivitySummary(runDir string) (*usageActivitySummary, error) { | ||
| candidates := []string{ | ||
| filepath.Join(runDir, "usage", "activity", "summary.json"), | ||
| filepath.Join(runDir, "activity", "summary.json"), | ||
| } | ||
| for _, candidate := range candidates { | ||
| cleanPath := filepath.Clean(candidate) | ||
| raw, err := os.ReadFile(cleanPath) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| continue | ||
| } | ||
| return nil, fmt.Errorf("read usage activity summary %s: %w", cleanPath, err) | ||
| } | ||
| var summary usageActivitySummary | ||
| if err := json.Unmarshal(raw, &summary); err != nil { | ||
| return nil, fmt.Errorf("parse usage activity summary %s: %w", cleanPath, err) | ||
|
Contributor
There was a problem hiding this comment. JSON parse failure on the primary candidate short-circuits the fallback path: if 💡 Suggested fixFall through on unmarshal failure the same way var summary usageActivitySummary
if err := json.Unmarshal(raw, &summary); err != nil {
continue // try next candidate
}
return &summary, nilOr collect the error and return it only after exhausting all candidates: var lastErr error
for _, candidate := range candidates {
raw, err := os.ReadFile(cleanPath)
if err != nil {
if os.IsNotExist(err) { continue }
return nil, err
}
var s usageActivitySummary
if err := json.Unmarshal(raw, &s); err != nil {
lastErr = err
continue
}
return &s, nil
}
return nil, lastErrA partially-written JSON file from an interrupted artifact upload should behave the same as a missing file for the purposes of the fallback chain. |
||
| } | ||
| return &summary, nil | ||
|
Comment on lines
+64
to
+73
|
||
| } | ||
| return nil, nil | ||
| } | ||
|
|
||
| func applyUsageActivitySummaryToResult(summary *usageActivitySummary, result *DownloadResult) { | ||
| if summary == nil || result == nil { | ||
| return | ||
| } | ||
|
|
||
| // Preserve previously parsed turn counts (from full session artifacts/events.jsonl) | ||
| // and only backfill when they are missing. | ||
| if summary.Session != nil && result.Run.Turns == 0 && summary.Session.Turns > 0 { | ||
|
Contributor
There was a problem hiding this comment. [/grill-with-docs] The condition 💡 Options to considerThe simplest fix is to use a sentinel that means "no full-artifact data available" instead of relying on the zero value. For example:
|
||
| result.Run.Turns = summary.Session.Turns | ||
| } | ||
|
|
||
| if summary.Firewall != nil && result.FirewallAnalysis == nil { | ||
| result.FirewallAnalysis = &FirewallAnalysis{ | ||
| TotalRequests: summary.Firewall.TotalRequests, | ||
| AllowedRequests: summary.Firewall.AllowedRequests, | ||
| BlockedRequests: summary.Firewall.BlockedRequests, | ||
| RequestsByDomain: map[string]DomainRequestStats{}, | ||
| } | ||
| } | ||
|
|
||
| if summary.Gateway != nil && result.MCPToolUsage == nil { | ||
| servers := make([]MCPServerStats, 0, len(summary.Gateway.Servers)) | ||
| for _, server := range summary.Gateway.Servers { | ||
| servers = append(servers, MCPServerStats{ | ||
| ServerName: server.ServerName, | ||
| // Keep both RequestCount and ToolCallCount aligned because MCPServerStats | ||
| // distinguishes overall request volume (RequestCount) from tool-invocation | ||
| // volume (ToolCallCount). In usage-aggregate mode we only have per-server | ||
| // tool-call counts, so both fields are populated from that single source. | ||
| RequestCount: server.ToolCallCount, | ||
|
Contributor
There was a problem hiding this comment. [/improve-codebase-architecture] 💡 SuggestionEither:
The existing comment explains the intent, but it's easy to miss. A zero value with a named sentinel (or a |
||
| ToolCallCount: server.ToolCallCount, | ||
| ErrorCount: server.FailedCalls, | ||
| }) | ||
| } | ||
| result.MCPToolUsage = &MCPToolUsageData{ | ||
| Servers: servers, | ||
| } | ||
| } | ||
|
Comment on lines
+112
to
+117
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| package cli | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestLoadUsageActivitySummary(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| runDir := t.TempDir() | ||
| summaryPath := filepath.Join(runDir, "usage", "activity", "summary.json") | ||
| require.NoError(t, os.MkdirAll(filepath.Dir(summaryPath), 0o755)) | ||
| require.NoError(t, os.WriteFile(summaryPath, []byte(`{ | ||
| "schema":"usage-activity-summary/v1", | ||
| "firewall":{"total_requests":10,"allowed_requests":8,"blocked_requests":2}, | ||
| "session":{"turns":7}, | ||
| "gateway":{"total_calls":5,"failed_calls":1} | ||
| }`), 0o644)) | ||
|
|
||
| summary, err := loadUsageActivitySummary(runDir) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, summary) | ||
| require.NotNil(t, summary.Firewall) | ||
| assert.Equal(t, 10, summary.Firewall.TotalRequests) | ||
| require.NotNil(t, summary.Session) | ||
| assert.Equal(t, 7, summary.Session.Turns) | ||
| require.NotNil(t, summary.Gateway) | ||
| assert.Equal(t, 5, summary.Gateway.TotalCalls) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. [/tdd] 💡 Suggested additionsfunc TestLoadUsageActivitySummary_FallbackPath(t *testing.T) {
t.Parallel()
runDir := t.TempDir()
// Write to the legacy fallback location.
fallbackPath := filepath.Join(runDir, "activity", "summary.json")
require.NoError(t, os.MkdirAll(filepath.Dir(fallbackPath), 0o755))
require.NoError(t, os.WriteFile(fallbackPath, []byte(`{"session":{"turns":3}}`), 0o644))
summary, err := loadUsageActivitySummary(runDir)
require.NoError(t, err)
require.NotNil(t, summary)
assert.Equal(t, 3, summary.Session.Turns)
}
func TestLoadUsageActivitySummary_NoFile(t *testing.T) {
t.Parallel()
summary, err := loadUsageActivitySummary(t.TempDir())
require.NoError(t, err)
assert.Nil(t, summary)
}
func TestLoadUsageActivitySummary_MalformedJSON(t *testing.T) {
t.Parallel()
runDir := t.TempDir()
p := filepath.Join(runDir, "usage", "activity", "summary.json")
require.NoError(t, os.MkdirAll(filepath.Dir(p), 0o755))
require.NoError(t, os.WriteFile(p, []byte(`{not valid json`), 0o644))
_, err := loadUsageActivitySummary(runDir)
require.Error(t, err)
} |
||
|
|
||
| func TestApplyUsageActivitySummaryToResult(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| result := DownloadResult{} | ||
| summary := &usageActivitySummary{ | ||
| Session: &usageActivitySession{Turns: 4}, | ||
| Firewall: &usageActivityFirewall{ | ||
| TotalRequests: 12, | ||
| AllowedRequests: 9, | ||
| BlockedRequests: 3, | ||
| }, | ||
| Gateway: &usageActivityGateway{ | ||
| TotalCalls: 6, | ||
| FailedCalls: 2, | ||
| Servers: []usageActivityGatewayServer{ | ||
| {ServerName: "github", ToolCallCount: 5, FailedCalls: 2}, | ||
| {ServerName: "playwright", ToolCallCount: 1, FailedCalls: 0}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| applyUsageActivitySummaryToResult(summary, &result) | ||
|
|
||
| assert.Equal(t, 4, result.Run.Turns) | ||
| require.NotNil(t, result.FirewallAnalysis) | ||
| assert.Equal(t, 12, result.FirewallAnalysis.TotalRequests) | ||
| assert.Equal(t, 3, result.FirewallAnalysis.BlockedRequests) | ||
| require.NotNil(t, result.MCPToolUsage) | ||
| require.Len(t, result.MCPToolUsage.Servers, 2) | ||
| assert.Equal(t, "github", result.MCPToolUsage.Servers[0].ServerName) | ||
| assert.Equal(t, 5, result.MCPToolUsage.Servers[0].ToolCallCount) | ||
| assert.Equal(t, 2, result.MCPToolUsage.Servers[0].ErrorCount) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The three backfill-precedence branches in 💡 Suggested test casesfunc TestApplyUsageActivitySummary_DoesNotOverwriteExistingData(t *testing.T) {
t.Parallel()
// Turns: a non-zero value must not be replaced.
result := DownloadResult{}
result.Run.Turns = 9
applyUsageActivitySummaryToResult(
&usageActivitySummary{Session: &usageActivitySession{Turns: 4}},
&result,
)
assert.Equal(t, 9, result.Run.Turns, "existing turns must not be overwritten")
// FirewallAnalysis: a non-nil pointer must not be replaced.
existing := &FirewallAnalysis{TotalRequests: 100}
result2 := DownloadResult{FirewallAnalysis: existing}
applyUsageActivitySummaryToResult(
&usageActivitySummary{Firewall: &usageActivityFirewall{TotalRequests: 5}},
&result2,
)
assert.Same(t, existing, result2.FirewallAnalysis)
// MCPToolUsage: a non-nil pointer must not be replaced.
existingMCP := &MCPToolUsageData{}
result3 := DownloadResult{MCPToolUsage: existingMCP}
applyUsageActivitySummaryToResult(
&usageActivitySummary{Gateway: &usageActivityGateway{TotalCalls: 3}},
&result3,
)
assert.Same(t, existingMCP, result3.MCPToolUsage)
}This precedence contract is the core invariant of the backfill design: if a future change reorders full-artifact parsing and the backfill call, these tests will catch the regression immediately. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/zoom-out] Non-
ENOENTerrors fromloadUsageActivitySummary(e.g., permission denied, storage corruption) are only surfaced when--verboseis active. A systematic read failure affecting every run would be completely invisible in normal operation — not counted in any metric, not printed, not aggregated.💡 Suggestion
Consider emitting this warning unconditionally (or at least incrementing a skipped-run counter), since a non-ENOENT IO error is genuinely unexpected and should not require
--verboseto diagnose. The warning is already low-noise (one line per run) and distinguishes the missing-file case (whichloadUsageActivitySummaryreturnsnil, nilfor silently) from the real-error case.