Skip to content

Create a grpc StatsHandler which exports ingress and egress bytes metrics with cloud provider and region dimensions#11556

Merged
vanja-p merged 31 commits intomasterfrom
vanja-egressstats
Mar 13, 2026
Merged

Create a grpc StatsHandler which exports ingress and egress bytes metrics with cloud provider and region dimensions#11556
vanja-p merged 31 commits intomasterfrom
vanja-egressstats

Conversation

@vanja-p
Copy link
Contributor

@vanja-p vanja-p commented Mar 12, 2026 •
edited
Loading

Based on #11508

I'll consolidate with https://github.com/buildbuddy-io/buildbuddy-internal/pull/6809 in a later PR.

iain-macdonald and others added 18 commits March 10, 2026 17:35
@iain-macdonald
Add GCP IP ranges and fetcher
d7691cd
@iain-macdonald
Add AWS IP ranges and fetcher
d5fab03
@iain-macdonald
Support adding additional stats handlers to gRPC servers
9753b3a
@iain-macdonald
Add a metric tracking gRPC server egress
bd2134a
@iain-macdonald
Remove header rows from the CSVs
e18d719
@iain-macdonald
WIP: add RPC interceptors that classify egress bytes by group ID, and…
e3c783f 
… destination cloud and region
@iain-macdonald
lru
1c4c019
@iain-macdonald
Remove outtrailer
7fd45aa
@iain-macdonald
Add benchmark
f76516c
@iain-macdonald
TODO
18347b4
@vanja-p
add azure, github, macstadium, metal IP ranges
a3a6143
@vanja-p
make sure github takes precedence over azure
9ab6316
@vanja-p
add a few more tests
f37422c
@vanja-p
modernize benchmark
0c0e725
@vanja-p
add stats handler benchmarks
7a61139
@vanja-p
fix benchmarks
35bdb0f
@vanja-p
add benchmark for 10 payloads
5b3131c
@vanja-p
optimize record
6f36325 
goos: linux
goarch: amd64
pkg: github.com/buildbuddy-io/buildbuddy/enterprise/server/util/egress
cpu: AMD Ryzen 9 7900X 12-Core Processor
                                             │  /tmp/base  │             /tmp/after             │
                                             │   sec/op    │   sec/op     vs base               │
StatsHandler/cached_hit-24                     180.2n ± 5%   156.9n ± 7%  -12.93% (p=0.001 n=7)
StatsHandler/cached_hit_multiple_payloads-24   631.1n ± 2%   199.8n ± 2%  -68.34% (p=0.001 n=7)
StatsHandler/varying_ips-24                    110.0µ ± 1%   109.7µ ± 2%        ~ (p=0.165 n=7)
geomean                                        2.322µ        1.509µ       -34.98%

                                             │ /tmp/base  │            /tmp/after             │
                                             │    B/op    │    B/op     vs base               │
StatsHandler/cached_hit-24                     184.0 ± 0%   136.0 ± 0%  -26.09% (p=0.001 n=7)
StatsHandler/cached_hit_multiple_payloads-24   184.0 ± 0%   136.0 ± 0%  -26.09% (p=0.001 n=7)
StatsHandler/varying_ips-24                    439.0 ± 0%   391.0 ± 0%  -10.93% (p=0.001 n=7)
geomean                                        245.9        193.4       -21.35%

                                             │ /tmp/base  │            /tmp/after             │
                                             │ allocs/op  │ allocs/op   vs base               │
StatsHandler/cached_hit-24                     5.000 ± 0%   4.000 ± 0%  -20.00% (p=0.001 n=7)
StatsHandler/cached_hit_multiple_payloads-24   5.000 ± 0%   4.000 ± 0%  -20.00% (p=0.001 n=7)
StatsHandler/varying_ips-24                    9.000 ± 0%   8.000 ± 0%  -11.11% (p=0.001 n=7)
geomean                                        6.082        5.040       -17.14%
@vadimberezniker
Copy link
Member

vadimberezniker commented Mar 12, 2026

do we need one package per platform? the tools are simple enough that you could put them into a single tool. then it's just one binary to run to refresh all the ranges.

@vanja-p
Copy link
Contributor Author

vanja-p commented Mar 12, 2026

@vadimberezniker , I blame claude. I'll tell it to merge the commands. Do you think we should still have separate files for each provider? It might make diffs a bit easier to see.

@vadimberezniker
Copy link
Member

vadimberezniker commented Mar 12, 2026 •
edited
Loading

@vadimberezniker , I blame claude. I'll tell it to merge the commands. Do you think we should still have separate files for each provider? It might make diffs a bit easier to see.

yeah one file per provider is fine. if you want you can also filter out the IPV6 ranges from the files. we currently don't serve over ipv6.

@vanja-p vanja-p changed the title WIP: egress stats handler Create a grpc StatsHandler which exports ingress and egress bytes metrics with cloud provider and region dimensions Mar 12, 2026
@vanja-p vanja-p marked this pull request as ready for review March 12, 2026 21:06
vadimberezniker
vadimberezniker requested changes Mar 12, 2026
View reviewed changes
enterprise/server/util/trafficstats/trafficstats.go Outdated
if info == nil {
return ctx
}
return context.WithValue(ctx, connDestinationKey{}, h.classifier.classify(info.RemoteAddr))
Copy link
Member

@vadimberezniker vadimberezniker Mar 12, 2026

Choose a reason for hiding this comment

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

Because of the load balancer siting in the middle info.RemoteAddr is going to give you the address of the LB proxy. You can use the clientip package to get the IP of the real client. This also means you can't cache this at the connection level.

Copy link
Contributor Author

@vanja-p vanja-p Mar 13, 2026

Choose a reason for hiding this comment

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

thanks, done.

vadimberezniker
vadimberezniker approved these changes Mar 13, 2026
View reviewed changes
Copy link
Member

@vadimberezniker vadimberezniker left a comment

Choose a reason for hiding this comment

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

You should add a classifier for private IPs to explicitly categorize internal traffic, otherwise looks good.

https://pkg.go.dev/net/netip#Addr.IsPrivate

@vanja-p
Copy link
Contributor Author

vanja-p commented Mar 13, 2026

You should add a classifier for private IPs to explicitly categorize internal traffic, otherwise looks good.

Good idea. Done.

@vanja-p vanja-p merged commit e4f4548 into master Mar 13, 2026
12 of 13 checks passed
@vanja-p vanja-p deleted the vanja-egressstats branch March 13, 2026 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iain-macdonald iain-macdonald iain-macdonald approved these changes

@vadimberezniker vadimberezniker vadimberezniker approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vanja-p @vadimberezniker @iain-macdonald