-
Notifications
You must be signed in to change notification settings - Fork 33
Report packet-capture statistics by host #179
Conversation
apidump/summary.go
Outdated
s.printPortHighlights(top) | ||
s.printHostHighlights(top) |
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.
Can we clarify to the user that these highlights do not describe disjoint sets of traffic?
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.
I added section labels to that effect in dd88251.
apidump/summary.go
Outdated
if left.HTTPRequests != right.HTTPRequests { | ||
return left.HTTPRequests > right.HTTPRequests | ||
} else if left.HTTPResponses != right.HTTPResponses { | ||
return left.HTTPResponses > right.HTTPResponses |
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.
Should we sort by the sum instead?
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.
Makes sense. Changed in bf4d2b3.
trace/stats.go
Outdated
@@ -131,11 +172,39 @@ func (s *PacketCounter) Summary(n int) *PacketCountSummary { | |||
s.mutex.RLock() | |||
defer s.mutex.RUnlock() | |||
|
|||
topByPort, byPortOverflow := s.byPort.TopN(n, func(c *PacketCounts) int { return c.TCPPackets }) | |||
topByInterface, byInterfaceOverflow := s.byInterface.TopN(n, func(c *PacketCounts) int { return c.TCPPackets }) | |||
topByHost, byHostOverflow := s.byHost.TopN(n, func(c *PacketCounts) int { return c.TCPPackets }) |
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.
Aren't the TCP counts in byHost
all zero, according to the XXX
comment on line 37?
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.
Yes, good point. Fixed in bf4d2b3.
trace/stats.go
Outdated
limit: limit, | ||
m: make(map[T]*PacketCounts), | ||
|
||
// Accumulate across all interfaces and ports. |
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.
And hosts too?
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.
Fixed in bf4d2b3.
@@ -27,40 +28,71 @@ func (d *PacketCountDiscard) Update(_ PacketCounts) { | |||
// In the future, this could put counters on a pipe and do the increments | |||
// in a separate goroutine, but we would *still* need a mutex to read the | |||
// totals out. | |||
// TODO: limit maximum size | |||
type PacketCounter struct { |
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.
Documentation should be updated.
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.
Updated in bf4d2b3.
This PR adds per-host packet capture statistics for TLS handshakes and HTTP requests, which readily have the host available. Per-host stats are uploaded with telemetry data and summarized as CLI output alongside per-port statistics.
This also adds hard limits on the number of ports, interfaces, and hosts that the CLI tracks for telemetry data. The CLI is a long-running process, and our telemetry data monotonically increases. We should consider capturing telemetry data in rolling windows in the future. For now, I chose a limit of 10,000 each for ports, interfaces, and hosts; if we assume hosts and interfaces are ~10 bytes, then that maxes out at ~2Mb. These are most useful in the first 60 seconds of running the CLI, so 10K should be more than enough.
Depends on postmanlabs/observability-shared-libs#174.
Here's what the CLI packet capture summary looks like with hosts: