benchmarking: move glutton's internals into internal/benchmarking/glutton - #1311
Draft
Sairaj Pokale (sairajp-rewind) wants to merge 2 commits into
Draft
benchmarking: move glutton's internals into internal/benchmarking/glutton#1311Sairaj Pokale (sairajp-rewind) wants to merge 2 commits into
internal/benchmarking/glutton#1311Sairaj Pokale (sairajp-rewind) wants to merge 2 commits into
Conversation
cmd/benchmarking/glutton/main.go held the entire workload: the service, its otel instruments, the gossip loop, and both transports. None of it was reachable outside package main, which left internal/benchmarking/glutton holding nothing but a fake of a server implemented elsewhere. Move everything except process lifecycle into the internal package, matching cmd/benchmarking/boomer-glutton. main.go keeps flag parsing, telemetry boot, the listener, and the shutdown defers. The --mode switch moves out too, since it selects a transport rather than managing the process, and now returns an error for an unknown mode instead of calling serverboot.Fatal. The destination is top-level internal/ because both packages that want to share glutton's route contract (the fake, and boomer's glutton user class) sit outside cmd/benchmarking/glutton/. Renames: gluttonService to Service, newGluttonService to New, newServer to NewServer, and the mode switch to Handler. sizes.go moves along with its only caller. No behavior change. Signed-off-by: Sairaj Pokale <sairajpokale@google.com>
glutton.go arrived from the previous commit as a single 640-line file, and the routes it serves were spelled out twice: once in the mux, once in the fake that stands in for it in boomer's tests. Split it by concern into server.go, service.go, metrics.go, and gossip.go, leaving glutton.go with the package doc and the constants describing the workload's contract. The instrument block moves out of New into Service.initMetrics. Add Name, ModeGRPC/ModeHTTP, and one constant per route. The fake re-exports the route constants instead of declaring its own, so a renamed path cannot leave the stand-in answering something the actor does not. Name replaces the four "glutton" literals across the tracer scope, meter scope, and the binary's tracing and metrics init. Instrument names stay written out in full, since hack/verify/metrics.sh greps for them. Boomer keeps its own private route constants: it is the client, and pointing it at the server's implementation package would link the gRPC server and otel instruments into a binary that runs neither. The tests split the same way, and gain coverage for Handler's mode selection and the /writeram route. Signed-off-by: Sairaj Pokale <sairajpokale@google.com>
Collaborator
|
Marked as draft, cannot merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR and Why we need it ?
Glutton's implementation lived in
cmd/benchmarking/glutton/main.go, which Go'sinternal/rule makes importable only fromcmd/benchmarking/glutton/.... That iswhy
internal/benchmarking/glutton/fake/existed as a subdirectory with no parentpackage: the fake could not import the thing it was faking. It duplicated the route
strings instead, and so did boomer.
This moves the implementation to
internal/benchmarking/glutton/, where the fake sitsnext to it and can share its constants.
main.gokeeps flag parsing and boot. No behavior change.Commits
1. Move the core out of
cmd/.main.go→internal/benchmarking/glutton/glutton.goverbatim, plus
sizes.go/sizes_test.gorelocated from where #1130 landed them.newGluttonService→New,handlerForMode→Handler; the proto import is aliasedgluttonpbper repo convention.2. Split the package and export the routes. The 640-line file becomes
glutton.go(identity and route consts),
server.go,service.go,metrics.go,gossip.go; testssplit into
server_test.goandservice_test.go.fake/server.gonow aliases the realroute constants rather than restating them, so the stand-in cannot answer a path the
actor does not.
Validation
make verifygofmt -l ./cmd/benchmarking ./internal/benchmarking(clean)go build ./...go vet ./cmd/benchmarking/... ./internal/benchmarking/...go test ./cmd/benchmarking/... ./internal/benchmarking/...git diff -Mshows{cmd => internal}forsizes.go,sizes_test.go)