Skip to content

Refactor: Extract generic aggregation helper to eliminate duplicate code in logs report #3591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Nov 11, 2025
Merged

Refactor: Extract generic aggregation helper to eliminate duplicate code in logs report #3591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e6f13b5
Initial plan
Copilot Nov 11, 2025
830c33f
Refactor: Eliminate duplicate code in logs report aggregation functions
Copilot Nov 11, 2025
8f3bd4a
Merge branch 'main' into copilot/refactor-duplicate-code-helper-funct…
Copilot Nov 11, 2025
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
136 changes: 89 additions & 47 deletions pkg/cli/logs_report.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,37 +292,75 @@ func addUniqueWorkflow(workflows []string, workflow string) []string {
return append(workflows, workflow)
}

// buildMissingToolsSummary aggregates missing tools across all runs
func buildMissingToolsSummary(processedRuns []ProcessedRun) []MissingToolSummary {
toolSummary := make(map[string]*MissingToolSummary)

// aggregateSummaryItems is a generic helper that aggregates items from processed runs into summaries
// It handles the common pattern of grouping by key, counting occurrences, tracking unique workflows, and collecting run IDs
func aggregateSummaryItems[TItem any, TSummary any](
processedRuns []ProcessedRun,
getItems func(ProcessedRun) []TItem,
getKey func(TItem) string,
createSummary func(TItem) *TSummary,
updateSummary func(*TSummary, TItem),
finalizeSummary func(*TSummary),
) []TSummary {
summaryMap := make(map[string]*TSummary)

// Aggregate items from all runs
for _, pr := range processedRuns {
for _, tool := range pr.MissingTools {
if summary, exists := toolSummary[tool.Tool]; exists {
summary.Count++
summary.Workflows = addUniqueWorkflow(summary.Workflows, tool.WorkflowName)
summary.RunIDs = append(summary.RunIDs, tool.RunID)
for _, item := range getItems(pr) {
key := getKey(item)
if summary, exists := summaryMap[key]; exists {
updateSummary(summary, item)
} else {
toolSummary[tool.Tool] = &MissingToolSummary{
Tool: tool.Tool,
Count: 1,
Workflows: []string{tool.WorkflowName},
FirstReason: tool.Reason,
RunIDs: []int64{tool.RunID},
}
summaryMap[key] = createSummary(item)
}
}
}

var result []MissingToolSummary
for _, summary := range toolSummary {
// Populate WorkflowsDisplay and FirstReasonDisplay fields for console rendering (truncation handled by maxlen tag)
summary.WorkflowsDisplay = strings.Join(summary.Workflows, ", ")
summary.FirstReasonDisplay = summary.FirstReason

// Convert map to slice and finalize each summary
var result []TSummary
for _, summary := range summaryMap {
finalizeSummary(summary)
result = append(result, *summary)
}

return result
}

// buildMissingToolsSummary aggregates missing tools across all runs
func buildMissingToolsSummary(processedRuns []ProcessedRun) []MissingToolSummary {
result := aggregateSummaryItems(
processedRuns,
// getItems: extract missing tools from each run
func(pr ProcessedRun) []MissingToolReport {
return pr.MissingTools
},
// getKey: use tool name as the aggregation key
func(tool MissingToolReport) string {
return tool.Tool
},
// createSummary: create new summary for first occurrence
func(tool MissingToolReport) *MissingToolSummary {
return &MissingToolSummary{
Tool: tool.Tool,
Count: 1,
Workflows: []string{tool.WorkflowName},
FirstReason: tool.Reason,
RunIDs: []int64{tool.RunID},
}
},
// updateSummary: update existing summary with new occurrence
func(summary *MissingToolSummary, tool MissingToolReport) {
summary.Count++
summary.Workflows = addUniqueWorkflow(summary.Workflows, tool.WorkflowName)
summary.RunIDs = append(summary.RunIDs, tool.RunID)
},
// finalizeSummary: populate display fields for console rendering
func(summary *MissingToolSummary) {
summary.WorkflowsDisplay = strings.Join(summary.Workflows, ", ")
summary.FirstReasonDisplay = summary.FirstReason
},
)

// Sort by count descending
sort.Slice(result, func(i, j int) bool {
return result[i].Count > result[j].Count
Expand All @@ -333,32 +371,36 @@ func buildMissingToolsSummary(processedRuns []ProcessedRun) []MissingToolSummary

// buildMCPFailuresSummary aggregates MCP failures across all runs
func buildMCPFailuresSummary(processedRuns []ProcessedRun) []MCPFailureSummary {
failureSummary := make(map[string]*MCPFailureSummary)

for _, pr := range processedRuns {
for _, failure := range pr.MCPFailures {
if summary, exists := failureSummary[failure.ServerName]; exists {
summary.Count++
summary.Workflows = addUniqueWorkflow(summary.Workflows, failure.WorkflowName)
summary.RunIDs = append(summary.RunIDs, failure.RunID)
} else {
failureSummary[failure.ServerName] = &MCPFailureSummary{
ServerName: failure.ServerName,
Count: 1,
Workflows: []string{failure.WorkflowName},
RunIDs: []int64{failure.RunID},
}
result := aggregateSummaryItems(
processedRuns,
// getItems: extract MCP failures from each run
func(pr ProcessedRun) []MCPFailureReport {
return pr.MCPFailures
},
// getKey: use server name as the aggregation key
func(failure MCPFailureReport) string {
return failure.ServerName
},
// createSummary: create new summary for first occurrence
func(failure MCPFailureReport) *MCPFailureSummary {
return &MCPFailureSummary{
ServerName: failure.ServerName,
Count: 1,
Workflows: []string{failure.WorkflowName},
RunIDs: []int64{failure.RunID},
}
}
}

var result []MCPFailureSummary
for _, summary := range failureSummary {
// Populate WorkflowsDisplay field for console rendering (truncation handled by maxlen tag)
summary.WorkflowsDisplay = strings.Join(summary.Workflows, ", ")

result = append(result, *summary)
}
},
// updateSummary: update existing summary with new occurrence
func(summary *MCPFailureSummary, failure MCPFailureReport) {
summary.Count++
summary.Workflows = addUniqueWorkflow(summary.Workflows, failure.WorkflowName)
summary.RunIDs = append(summary.RunIDs, failure.RunID)
},
// finalizeSummary: populate display fields for console rendering
func(summary *MCPFailureSummary) {
summary.WorkflowsDisplay = strings.Join(summary.Workflows, ", ")
},
)

// Sort by count descending
sort.Slice(result, func(i, j int) bool {
Expand Down
94 changes: 94 additions & 0 deletions pkg/cli/logs_report_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,100 @@ func TestBuildMCPFailuresSummaryDeduplication(t *testing.T) {
}
}

// TestAggregateSummaryItems tests the generic aggregation helper function
func TestAggregateSummaryItems(t *testing.T) {
// Test with MissingToolReport data using the generic helper
processedRuns := []ProcessedRun{
{
Run: WorkflowRun{
WorkflowName: "workflow-a",
},
MissingTools: []MissingToolReport{
{
Tool: "docker",
Reason: "Container operations needed",
WorkflowName: "workflow-a",
RunID: 1001,
},
},
},
{
Run: WorkflowRun{
WorkflowName: "workflow-b",
},
MissingTools: []MissingToolReport{
{
Tool: "docker",
Reason: "Container build needed",
WorkflowName: "workflow-b",
RunID: 1002,
},
},
},
}

// Use the generic aggregation helper directly
result := aggregateSummaryItems(

Copilot AI Nov 11, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] The test calls aggregateSummaryItems directly, but since this is a generic helper function that's not exported (starts with lowercase a), it won't be accessible outside the package. This test will only work because it's in the same package (cli).

While this works for now, consider whether aggregateSummaryItems should be:

  1. Exported (renamed to AggregateSummaryItems) if it's intended to be reusable across packages
  2. Kept private if it's only for internal use within this package (current approach)

The current approach is fine for internal testing, but the naming and visibility should be intentional.

Copilot uses AI. Check for mistakes.

Copilot AI Nov 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The function is intentionally kept private (unexported) as it's an internal helper specifically designed for use within the cli package. The current approach ensures encapsulation - the generic aggregation pattern is not exposed as a public API since it's implementation-specific to the logs report functionality. Exporting it would create an unnecessary public interface that we'd need to maintain for backward compatibility.

processedRuns,
func(pr ProcessedRun) []MissingToolReport {
return pr.MissingTools
},
func(tool MissingToolReport) string {
return tool.Tool
},
func(tool MissingToolReport) *MissingToolSummary {
return &MissingToolSummary{
Tool: tool.Tool,
Count: 1,
Workflows: []string{tool.WorkflowName},
FirstReason: tool.Reason,
RunIDs: []int64{tool.RunID},
}
},
func(summary *MissingToolSummary, tool MissingToolReport) {
summary.Count++
summary.Workflows = addUniqueWorkflow(summary.Workflows, tool.WorkflowName)
summary.RunIDs = append(summary.RunIDs, tool.RunID)
},
func(summary *MissingToolSummary) {
summary.WorkflowsDisplay = "test-display"
},
)

// Verify aggregation worked correctly
if len(result) != 1 {
t.Errorf("Expected 1 aggregated summary, got %d", len(result))
return
}

summary := result[0]

// Verify count aggregation
if summary.Count != 2 {
t.Errorf("Expected count = 2, got %d", summary.Count)
}

// Verify workflow deduplication
if len(summary.Workflows) != 2 {
t.Errorf("Expected 2 unique workflows, got %d", len(summary.Workflows))
}

// Verify run IDs collected
if len(summary.RunIDs) != 2 {
t.Errorf("Expected 2 run IDs, got %d", len(summary.RunIDs))
}

// Verify first reason preserved
if summary.FirstReason != "Container operations needed" {
t.Errorf("Expected FirstReason = 'Container operations needed', got '%s'", summary.FirstReason)
}

// Verify finalize was called
if summary.WorkflowsDisplay != "test-display" {
t.Errorf("Expected WorkflowsDisplay = 'test-display', got '%s'", summary.WorkflowsDisplay)
}
}

// TestAggregateDomainStats tests the shared domain aggregation helper
func TestAggregateDomainStats(t *testing.T) {
t.Run("aggregates domains correctly", func(t *testing.T) {
Expand Down
Loading