Skip to content

Refactor: Extract shared domain aggregation logic in logs_report.go #3588

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 2 commits into from
Nov 11, 2025
Merged

Refactor: Extract shared domain aggregation logic in logs_report.go #3588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
887e5e2
Initial plan
Copilot Nov 11, 2025
111cfc5
Refactor: Extract shared domain aggregation logic
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
172 changes: 95 additions & 77 deletions pkg/cli/logs_report.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,52 +368,87 @@ func buildMCPFailuresSummary(processedRuns []ProcessedRun) []MCPFailureSummary {
return result
}

// buildAccessLogSummary aggregates access log data across all runs
func buildAccessLogSummary(processedRuns []ProcessedRun) *AccessLogSummary {
allAllowedDomains := make(map[string]bool)
allDeniedDomains := make(map[string]bool)
byWorkflow := make(map[string]*DomainAnalysis)
totalRequests := 0
allowedCount := 0
deniedCount := 0
// domainAggregation holds the result of aggregating domain statistics
type domainAggregation struct {
allAllowedDomains map[string]bool
allDeniedDomains map[string]bool
totalRequests int
allowedCount int
deniedCount int
}

// aggregateDomainStats aggregates domain statistics across runs
// This is a shared helper for both access log and firewall log summaries
func aggregateDomainStats(processedRuns []ProcessedRun, getAnalysis func(*ProcessedRun) (allowedDomains, deniedDomains []string, totalRequests, allowedCount, deniedCount int, exists bool)) *domainAggregation {
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

[nitpick] The callback function signature could benefit from named return values to improve readability, especially given the multiple int return values. Consider:

func aggregateDomainStats(processedRuns []ProcessedRun, 
    getAnalysis func(*ProcessedRun) (
        allowedDomains, deniedDomains []string, 
        totalRequests, allowedCount, deniedCount int, 
        exists bool,
    )) *domainAggregation

This makes it clearer what each return value represents when reading callback implementations.

Suggested change
func aggregateDomainStats(processedRuns []ProcessedRun, getAnalysis func(*ProcessedRun) (allowedDomains, deniedDomains []string, totalRequests, allowedCount, deniedCount int, exists bool)) *domainAggregation {
func aggregateDomainStats(
processedRuns []ProcessedRun,
getAnalysis func(*ProcessedRun) (
allowedDomains []string,
deniedDomains []string,
totalRequests int,
allowedCount int,
deniedCount int,
exists bool,
),
) *domainAggregation {

Copilot uses AI. Check for mistakes.
agg := &domainAggregation{
allAllowedDomains: make(map[string]bool),
allDeniedDomains: make(map[string]bool),
}

for _, pr := range processedRuns {
if pr.AccessAnalysis != nil {
totalRequests += pr.AccessAnalysis.TotalRequests
allowedCount += pr.AccessAnalysis.AllowedCount
deniedCount += pr.AccessAnalysis.DeniedCount
byWorkflow[pr.Run.WorkflowName] = pr.AccessAnalysis

for _, domain := range pr.AccessAnalysis.AllowedDomains {
allAllowedDomains[domain] = true
}
for _, domain := range pr.AccessAnalysis.DeniedDomains {
allDeniedDomains[domain] = true
}
allowedDomains, deniedDomains, totalRequests, allowedCount, deniedCount, exists := getAnalysis(&pr)
if !exists {
continue
}

agg.totalRequests += totalRequests
agg.allowedCount += allowedCount
agg.deniedCount += deniedCount

for _, domain := range allowedDomains {
agg.allAllowedDomains[domain] = true
}
for _, domain := range deniedDomains {
agg.allDeniedDomains[domain] = true
}
}

if totalRequests == 0 {
return nil
return agg
}

// convertDomainsToSortedSlices converts domain maps to sorted slices
func convertDomainsToSortedSlices(allowedMap, deniedMap map[string]bool) (allowed, denied []string) {
for domain := range allowedMap {
allowed = append(allowed, domain)
}
sort.Strings(allowed)

// Convert maps to slices
var allowedDomains []string
for domain := range allAllowedDomains {
allowedDomains = append(allowedDomains, domain)
for domain := range deniedMap {
denied = append(denied, domain)
}
sort.Strings(allowedDomains)
sort.Strings(denied)

return allowed, denied
}

// buildAccessLogSummary aggregates access log data across all runs
func buildAccessLogSummary(processedRuns []ProcessedRun) *AccessLogSummary {
byWorkflow := make(map[string]*DomainAnalysis)

// Use shared aggregation helper
agg := aggregateDomainStats(processedRuns, func(pr *ProcessedRun) ([]string, []string, int, int, int, bool) {
if pr.AccessAnalysis == nil {
return nil, nil, 0, 0, 0, false
}
byWorkflow[pr.Run.WorkflowName] = pr.AccessAnalysis
return pr.AccessAnalysis.AllowedDomains,
pr.AccessAnalysis.DeniedDomains,
pr.AccessAnalysis.TotalRequests,
pr.AccessAnalysis.AllowedCount,
pr.AccessAnalysis.DeniedCount,
true
})

var deniedDomains []string
for domain := range allDeniedDomains {
deniedDomains = append(deniedDomains, domain)
if agg.totalRequests == 0 {
return nil
}
sort.Strings(deniedDomains)

allowedDomains, deniedDomains := convertDomainsToSortedSlices(agg.allAllowedDomains, agg.allDeniedDomains)

return &AccessLogSummary{
TotalRequests: totalRequests,
AllowedCount: allowedCount,
DeniedCount: deniedCount,
TotalRequests: agg.totalRequests,
AllowedCount: agg.allowedCount,
DeniedCount: agg.deniedCount,
AllowedDomains: allowedDomains,
DeniedDomains: deniedDomains,
ByWorkflow: byWorkflow,
Expand All @@ -422,59 +457,42 @@ func buildAccessLogSummary(processedRuns []ProcessedRun) *AccessLogSummary {

// buildFirewallLogSummary aggregates firewall log data across all runs
func buildFirewallLogSummary(processedRuns []ProcessedRun) *FirewallLogSummary {
allAllowedDomains := make(map[string]bool)
allDeniedDomains := make(map[string]bool)
allRequestsByDomain := make(map[string]DomainRequestStats)
byWorkflow := make(map[string]*FirewallAnalysis)
totalRequests := 0
allowedRequests := 0
deniedRequests := 0

for _, pr := range processedRuns {
if pr.FirewallAnalysis != nil {
totalRequests += pr.FirewallAnalysis.TotalRequests
allowedRequests += pr.FirewallAnalysis.AllowedRequests
deniedRequests += pr.FirewallAnalysis.DeniedRequests
byWorkflow[pr.Run.WorkflowName] = pr.FirewallAnalysis

for _, domain := range pr.FirewallAnalysis.AllowedDomains {
allAllowedDomains[domain] = true
}
for _, domain := range pr.FirewallAnalysis.DeniedDomains {
allDeniedDomains[domain] = true
}

// Aggregate request stats by domain
for domain, stats := range pr.FirewallAnalysis.RequestsByDomain {
existing := allRequestsByDomain[domain]
existing.Allowed += stats.Allowed
existing.Denied += stats.Denied
allRequestsByDomain[domain] = existing
}
// Use shared aggregation helper
agg := aggregateDomainStats(processedRuns, func(pr *ProcessedRun) ([]string, []string, int, int, int, bool) {
if pr.FirewallAnalysis == nil {
return nil, nil, 0, 0, 0, false
}
byWorkflow[pr.Run.WorkflowName] = pr.FirewallAnalysis

// Aggregate request stats by domain (firewall-specific)
for domain, stats := range pr.FirewallAnalysis.RequestsByDomain {
existing := allRequestsByDomain[domain]
existing.Allowed += stats.Allowed
existing.Denied += stats.Denied
allRequestsByDomain[domain] = existing
}
}

if totalRequests == 0 {
return nil
}
return pr.FirewallAnalysis.AllowedDomains,
pr.FirewallAnalysis.DeniedDomains,
pr.FirewallAnalysis.TotalRequests,
pr.FirewallAnalysis.AllowedRequests,
pr.FirewallAnalysis.DeniedRequests,
true
})

// Convert maps to slices
var allowedDomains []string
for domain := range allAllowedDomains {
allowedDomains = append(allowedDomains, domain)
if agg.totalRequests == 0 {
return nil
}
sort.Strings(allowedDomains)

var deniedDomains []string
for domain := range allDeniedDomains {
deniedDomains = append(deniedDomains, domain)
}
sort.Strings(deniedDomains)
allowedDomains, deniedDomains := convertDomainsToSortedSlices(agg.allAllowedDomains, agg.allDeniedDomains)

return &FirewallLogSummary{
TotalRequests: totalRequests,
AllowedRequests: allowedRequests,
DeniedRequests: deniedRequests,
TotalRequests: agg.totalRequests,
AllowedRequests: agg.allowedCount,
DeniedRequests: agg.deniedCount,
AllowedDomains: allowedDomains,
DeniedDomains: deniedDomains,
RequestsByDomain: allRequestsByDomain,
Expand Down
Loading
Loading