Extract shared AnalysisBase from DomainAnalysis/FirewallAnalysis#46063
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Extracts shared domain counters and merging logic into AnalysisBase, reducing duplication between access and firewall analysis.
Changes:
- Adds shared metrics and domain-union logic.
- Embeds
AnalysisBasein both analysis types. - Updates consumers and tests for renamed fields and nested literals.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/domain_buckets.go |
Adds AnalysisBase and domain merging. |
pkg/cli/access_log.go |
Migrates access analysis to the shared base. |
pkg/cli/firewall_log.go |
Migrates firewall analysis to the shared base. |
pkg/cli/logs_report_firewall.go |
Updates access counter references. |
pkg/cli/logs_usage_activity.go |
Builds nested shared-base values. |
pkg/cli/access_log_test.go |
Updates access-analysis tests. |
pkg/cli/audit_agent_example_test.go |
Updates example firewall fixtures. |
pkg/cli/audit_agent_output_test.go |
Updates agent-output fixtures. |
pkg/cli/audit_cross_run_test.go |
Updates cross-run fixtures. |
pkg/cli/audit_diff_test.go |
Updates firewall-diff fixtures. |
pkg/cli/audit_report_test.go |
Updates audit-report fixtures. |
pkg/cli/audit_test.go |
Updates audit fixtures. |
pkg/cli/firewall_log_integration_test.go |
Updates integration fixtures. |
pkg/cli/firewall_log_test.go |
Updates firewall merge tests. |
pkg/cli/log_aggregation_test.go |
Updates shared aggregation tests. |
pkg/cli/logs_episode_test.go |
Updates episode fixtures. |
pkg/cli/logs_report_test.go |
Updates report aggregation fixtures. |
pkg/cli/logs_summary_test.go |
Updates summary serialization fixture. |
pkg/cli/logs_usage_activity_test.go |
Updates usage-summary fixture. |
pkg/cli/observability_insights_test.go |
Updates observability fixtures. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Medium
| TotalRequests int `json:"total_requests"` | ||
| AllowedCount int `json:"allowed_count"` | ||
| BlockedCount int `json:"blocked_count"` | ||
| AnalysisBase |
| AnalysisBase: AnalysisBase{TotalRequests: 10, AllowedRequests: 6, BlockedRequests: 4}, | ||
| } | ||
|
|
||
| analysis2 := &DomainAnalysis{ | ||
| TotalRequests: 5, | ||
| AllowedCount: 3, | ||
| BlockedCount: 2, | ||
| AnalysisBase: AnalysisBase{TotalRequests: 5, AllowedRequests: 3, BlockedRequests: 2}, |
|
@copilot please run the
|
…und-trip tests
- Add MarshalJSON/UnmarshalJSON to DomainAnalysis to preserve the original
allowed_count/blocked_count JSON keys from before the AnalysisBase refactor,
maintaining backward-compat for cached RunSummary.access_analysis JSON and
AccessLogSummary.by_workflow consumers.
- Expand TestDomainAnalysisAddMetrics to cover domain-list merging/dedup
(the new behavior delivered by addBaseMetrics for DomainAnalysis).
- Add TestDomainAnalysisJSONWireNames to assert the stable wire names and
full round-trip fidelity.
- Fix interface{} → any (modernize lint).
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…nAnalysisAddMetrics Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Both review threads addressed in commits
Local fmt, lint, and targeted tests are all green. Pre-existing failures ( |
|
🎉 This pull request is included in a new release. Release: |
DomainAnalysisandFirewallAnalysisduplicatedDomainBuckets+TotalRequests+ allowed/blocked counts with parallelAddMetricsimplementations, creating drift risk.Changes
domain_buckets.go— NewAnalysisBasestruct embeddingDomainBuckets,TotalRequests,AllowedRequests,BlockedRequests; singleaddBaseMetricsimplementation;mergeDomainListhelper for deduped domain unionaccess_log.go—DomainAnalysisembedsAnalysisBase; renamesAllowedCount/BlockedCount→AllowedRequests/BlockedRequests;AddMetricsreduced to one callfirewall_log.go—FirewallAnalysisembedsAnalysisBaseinstead of three separate fields;AddMetricsonly handlesRequestsByDomainmerginglogs_report_firewall.go,logs_usage_activity.go— Field reference updatesDomain-list merging (previously only in
FirewallAnalysis.AddMetrics) is now handled once inaddBaseMetrics, so both types get it automatically.