perf(deps): let a build omit the datasource drivers and GraphQL engine it does not use - #4167
aryanmehrotra wants to merge 5 commits into
Conversation
A GoFr binary links every datasource driver whether or not the service
opens one. Measured on a plain HTTP service: 827 packages, 57.8 MB of
binary and 31.6 MB of resident memory at rest, against 19.6-20.9 MB for
every other Go HTTP framework at the same observability.
Two things account for it, and neither is reachable from the HTTP path.
modernc.org/sqlite is blank-imported for driver registration and brings
modernc.org/libc, whose netdb init parses embedded copies of /etc/protocols
and /etc/services into permanent Go structs -- 1.69 MB of retained heap,
roughly half the process's fixed heap floor. The three concrete pub/sub
clients bring 212 packages between them, most of it Google Pub/Sub's grpc
and auth stack.
Both are now behind build tags. The default build is unchanged: it links
the same 827 packages as before, byte for byte, so a user who does nothing
sees nothing. A service that uses neither can build with
-tags 'gofr_nosqldrivers gofr_nopubsub'
and gets 42.7 MB of binary (-26%), 22.8 MB idle RSS (-28%) and a 1.5 MB
heap floor (-68%) -- within about 2 MB of gin and gorilla.
Nothing is pinned by a public type, which is what makes this possible
without an API change: Container.PubSub is the pubsub.Client interface, and
the drivers are blank imports whose only effect is init(). Container.SQL and
Container.Redis keep their concrete types and are untouched.
A tagged build that configures an omitted backend still starts and serves.
It logs one ERROR naming the tag for pub/sub, or database/sql's own
'unknown driver' for a dialect, and leaves the client nil -- the same state
an unconfigured datasource already produces. The stubs return an untyped
nil, so both Close and Health take their nil branches. (Health guards SQL
and Redis with isNil but PubSub with a plain != nil, which a typed-nil
would defeat; nothing here produces one, but see the follow-up.) Verified by running a tagged binary
under each of PUBSUB_BACKEND=KAFKA, GOOGLE and MQTT and DB_DIALECT=sqlite:
all four serve correctly and complain loudly.
Tests run in both configurations. The sql suite imports its own drivers, and
the two container tests that assert on a concrete client skip when the
backends are not linked.
Every GoFr binary links graphql-go and gqlparser whether or not the
service registers a resolver: 20 packages and 0.78 MB of binary that a
service with no GraphQL never executes.
-tags gofr_nographql now leaves them out. Measured on a minimal GoFr
service (gofr.New, one route, Run):
default 57.10 MB 831 packages
-tags gofr_nographql 56.32 MB 811 packages
Idle RSS is unchanged, and that is expected: the engine allocates
nothing until a resolver is registered, so what the tag saves is text,
not heap. The saving compounds with the datasource tags -- both of those
plus this one take the same service to 41.26 MB and 554 packages.
The mechanism is the one the pub/sub backends already use, with one
addition. App's field becomes an interface, graphQLRunner, naming the
five methods App actually calls; graphql.go, which holds the concrete
manager, carries the tag. Both halves are needed -- the interface alone
changes nothing, because the concrete file still compiles the library
in. That is the general rule for making any subsystem optional.
Nothing changes for a user who does not set the tag: GraphQLQuery and
GraphQLMutation name only GoFr's own Handler, so the exported surface is
byte-identical in both builds, and the default build links the same
packages it always did.
A build that does set the tag still compiles a user's GraphQL calls.
Registering a resolver logs an error naming the tag rather than failing
silently, and enabled() keeps App from routing /graphql and the
playground against a handler this build cannot provide. Without that
guard setupGraphQL registers a nil handler and the first POST /graphql
panics; TestGraphQLDisabled_SetupRegistersNoRoute fails if the guard is
removed.
noopResponder moves from responder.go into graphql.go, its only user. It
cannot stay: responder.go also holds the exported Responder interface,
so it cannot carry the tag, and leaving the type there makes it dead
code in a tagged build -- which golangci-lint reports as unused, failing
the lint gate for anyone who sets the tag.
The Slim Build Tags CI job now covers gofr_nographql alongside the
datasource tags, and runs the stub tests, which exist only under it.
7989966 to
a969749
Compare
|
A downstream data point for We run a GoFr service that uses no pub/sub at all. Because Last week that last one broke a deploy for us. Google moved the $ curl -s https://proxy.golang.org/github.com/googleapis/enterprise-certificate-proxy/@v/v0.3.19.mod | grep toolchain
toolchain go1.25.8
$ curl -s https://raw.githubusercontent.com/googleapis/enterprise-certificate-proxy/v0.3.19/go.mod | grep toolchain
toolchain go1.26.5Our None of that is GoFr's doing. The point is only where we happened to be standing when it landed: we absorbed it for a module we never import, reached solely through a pub/sub client we don't use, feeding a client-certificate feature we have never enabled — nothing in our tree, infra or workflows sets So the 213 packages For what it's worth, this also reads as a continuation of existing practice rather than a new idea: Happy to build our service against the tag and report back if that would be useful. |
|
Thanks, this is a useful data point, and yes please: building your service against this branch and reporting back would help a lot. The branch head is
One limit to be upfront about, since it bears directly on the failure you hit. I reproduced it against a minimal service on this branch:
The tag removes the code from the build graph, so it is no longer linked into or compiled into your binary. It does not remove it from the module graph. Removing it from the module graph as well would mean moving the pub/sub clients into their own modules, the way |
PiyushSingh-ZS
left a comment
There was a problem hiding this comment.
The engineering here is careful and the measurements are worth having. Before line-level review, though, I think there is a mechanism question that maintainers should rule on, because it decides whether most of this diff is the right shape.
What is clearly right
The warning that the interface alone does nothing is the most valuable thing in the PR:
Extracting
graphQLRunnerleft all the graphql packages linked untilgraphql.goitself carried the tag -- the concrete file still compiles the library in.
That is the failure mode people ship by accident and then wonder why the binary did not shrink. Catching it and stating it plainly is a real contribution regardless of what happens to the rest.
The modernc.org/libc finding is also specific and verifiable: netdb init parsing embedded /etc/protocols and /etc/services into permanent Go structs, retained for the life of the process, unreachable from the HTTP path.
The mechanism question: build tags, or the pattern the repo already has?
GoFr already ships 25 nested go.mod modules under pkg/gofr/datasource/ — mongo, cassandra, clickhouse, scylladb, elasticsearch, oracle, surrealdb, dgraph, file/s3, file/gcs, kv-store/badger, and directly relevant here: pubsub/nats, pubsub/sqs, pubsub/eventhub.
Three pub/sub backends are already separate modules. Kafka, Google and MQTT are the outliers that stayed in the root module. So for gofr_nopubsub — by some distance the largest win in this PR, 213 packages and 8.9 MB — there is an existing, proven, zero-build-tag answer: extract pubsub/kafka, pubsub/google and pubsub/mqtt into their own modules, exactly like pubsub/nats.
| separate module | build tag | |
|---|---|---|
| Default build | unchanged | unchanged |
| Opt-in | go get + wire it |
remember -tags at every build site |
| Wrong config | compile error at the wiring site | runtime ERROR, service starts degraded |
| CI | ordinary go build ./... |
N-way tag matrix |
| Coverage gate | ordinary | tag-only files invisible to the default run |
go doc |
one surface | differs per tag |
| Precedent in this repo | 25 | 0 |
There is also a distribution problem tags have and modules do not: a tag is set by whoever runs go build, not by the dependency graph. A user building through Docker, ko, goreleaser, Bazel or an inherited Makefile has to thread -tags gofr_nopubsub,gofr_nosqldrivers,gofr_nographql through every one of those. Miss it in a single place and the binary silently reverts to 57 MB, with nothing to indicate it. A module boundary cannot be forgotten.
I do not think that argument is fatal to the whole PR — GraphQL lives in package gofr itself and the SQL drivers are blank imports for database/sql registration, so neither moves to a module easily, and a tag may genuinely be the only option for those two. But those are also the two smallest wins (1.4 MB and 6.1 MB against pub/sub's 8.9 MB). It would be worth splitting the pub/sub half out and asking whether it should be modules instead.
If tags are the agreed mechanism, notes on the implementation
1. The tagged build never exercises the tagged behavior for SQL.
drivers_testdeps_test.go blank-imports lib/pq and modernc.org/sqlite for tests, so under -tags gofr_nosqldrivers the test binary still has both drivers registered. The comment explains why and the reasoning is fair. But the consequence is that the documented behavior —
DB_DIALECT=postgresor=sqlitethen fails at startup: NewSQL'sregisterOtelcall reportsdatabase/sql's own "unknown driver" error naming the dialect, and returns a nil DB.
— is never tested in any configuration. One test that opens an unregistered dialect directly would cover it without disturbing the rest of the suite.
2. Coverage. Roughly half the added code (*_disabled.go) compiles only under a tag, so the code-climate gate CONTRIBUTING relies on will never see it. The Slim Build Tags job is the right mitigation — worth saying so in the PR description so the coverage delta is not read as a regression by a reviewer who has not spotted the job.
3. enabled() as an interface method. disabledGraphQL.buildSchema() returns nil and GetHandler() returns nil purely because enabled() guards them, which leaves two methods whose contract is "never call me". A const graphQLLinked = false — mirroring pubsubBackendsLinked, which this PR already introduces for pub/sub — would be consistent with the PR's own vocabulary and would let the compiler drop the dead branches outright.
4. The noopResponder relocation rationale is correct and non-obvious (responder.go cannot carry the tag because it holds the exported Responder interface, and leaving noopResponder there makes it unused under the tag and fails the lint gate). Worth keeping that comment exactly as written — it will save someone an afternoon.
5. createMqttPubSub returning an untyped nil in the disabled stub is clean, and composes correctly with #4164, which switches the container's pub/sub guard to isNil. Might be worth cross-linking the two.
Verifying identical go doc -all across 6 packages and an identical linked-package list for the default build is exactly the assertion this class of change needs.
Umang01-hash
left a comment
There was a problem hiding this comment.
Verified end-to-end at the head SHA in a clean worktree: each tag drops its tree completely (graphql→0, sqldrivers→0, pubsub→0 syms), the full set takes a real binary 56.3MB→39.6MB (−16.7MB/~30%), no API break, no typed-nil, fail-loud confirmed by actually running tagged binaries (PUBSUB_BACKEND/DB_DIALECT → ERROR naming the tag, nil datasource, service still boots). Clean, effective work — the interface seams and verbatim moves are well done, and CI building the all-3 combination is the right call.
Requesting changes on disabled-path test coverage + one overstated CI claim — same pattern flagged on #4168, plus two gaps unique here:
- The "Default build links the same packages as before" step doesn't assert that — it only compiles + tests. A
go list -depsdiff (or dep-count check) vs base would make the step earn its name and guard the whole PR's value. - The SQL disabled fail-loud path (unknown-driver → nil DB) has no test, and
drivers_testdeps_test.gore-registers pq+sqlite untagged, so CI actively masks the disabled state. GraphQL got a dedicated disabled test; SQL should have parity. - The pubsub disabled fail-loud (
pubsubDisabledMsg) is untested — a regression dropping the Errorf (silent no-publish) wouldn't fail CI.
Minor (non-blocking): the graphql_runner.go doc says the tag drops "1.4 MB"; the real measured drop is ~0.83 MB.
|
|
||
| # The tags are opt-in, so the default build must stay exactly as it was. | ||
| # This is the assertion that keeps that true. | ||
| - name: Default build links the same packages as before |
There was a problem hiding this comment.
This step compiles + tests but never compares the linked-package set to a baseline, so it doesn't assert what its name/comment claim. A go list -deps ./... | sort diff against a committed snapshot (or asserting the graphql/sqldriver/pubsub dep count is unchanged) would make it real — and would catch an accidental default-build regression.
| // silent. A user who wants one of these dialects in a tagged build imports | ||
| // the driver in their own main package, exactly as with database/sql directly. | ||
|
|
||
| package sql |
There was a problem hiding this comment.
The fail-loud path this file creates (DB_DIALECT=postgres/sqlite → unknown-driver → nil DB) has no test. drivers_testdeps_test.go blank-imports pq+sqlite untagged, so under -tags gofr_nosqldrivers the test binary re-registers the drivers and the disabled branch is never exercised in CI — the scaffolding masks it. A //go:build gofr_nosqldrivers test asserting NewSQL(...postgres...) returns nil would match the graphql_disabled_test.go treatment.
| return nil | ||
| } | ||
|
|
||
| func (c *Container) createKafkaPubSub(config.Config) { |
There was a problem hiding this comment.
The disabled stubs log fail-loud (pubsubDisabledMsg) but nothing asserts it — container_test.go only adds t.Skip guards. A regression dropping this Errorf (a publisher that silently never publishes) wouldn't fail CI. Worth a tagged test asserting the error is logged and PubSub stays nil, for parity with the graphql disabled path.
… it claims Review follow-ups on #4167. The CI step named "Default build links the same packages as before" only ran go build and go test, so it asserted nothing of the sort. It now diffs `go list -deps ./... | sort` against the merge base and fails on any change, which is the claim the tags rest on: 861 packages, identical, measured locally. The tagged build is also linted now. It was not before, and the first run of the new step found a real noctx failure in graphql_disabled_test.go that no job would ever have reported -- the same class of problem that prompted moving noopResponder and configTrue in the first place. Two fail-loud paths had no test. - The disabled pub/sub stubs log an error naming the tag, but container_test.go only skips under the tag, and a skip proves nothing about the stub. An edit dropping one Errorf would leave every job green while producing the exact failure the tag exists to avoid: a publisher that silently never publishes. pubsub_backends_disabled_test.go asserts all three, and fails when the Errorf is removed. - registerOtel's "unknown driver" error is what makes a misconfigured tagged build legible. It is asserted against a dialect that is registered in NEITHER build, not against postgres under the tag: drivers_testdeps_test.go blank-imports pq and sqlite so the suite behaves identically either way, which means a tagged assertion about postgres would test the fixture rather than the code. graphQLRunner.enabled() becomes `const graphQLLinked`, matching the pubsubBackendsLinked the pub/sub side already uses. Whether the engine is linked is a property of the build, not of an instance, and the compiler can fold the branch away. drivers_disabled.go named only postgres and sqlite; supabase and cockroachdb register under the postgres driver (sql.go:268), so the tag affects them too. Adds the user-facing documentation the tags had none of, with the numbers measured rather than asserted (829 packages by default, 617/785/809 per tag, 553 with all three), and a CONTRIBUTING note that these tags are a closed exception for subsystems already in core -- a new integration ships as its own module.
…rect the grpc claim Review follow-ups on #4168, plus the #4167 follow-ups merged in. Each tag was only ever built in isolation, so a symbol that resolves under one and breaks under two would ship green and fail only for the user who set both. CI now builds, vets and tests all six together -- verified locally: builds clean, and 412 packages against the default 829. The nil-guard on grpcSrv had no test. newGRPCRunner fails on an out-of-range GRPC_PORT and factory.go logs and continues, so App runs on with no gRPC server, and these four setters used to dereference the field blind -- a config typo turning into a nil-pointer panic in the user's own setup code. All four are asserted, since a later edit is as likely to reintroduce it in one of the others as in the one that was reported. Neutralising the guard makes the test panic. The claim that gofr_nootlp "is the tag that actually releases google.golang.org/grpc" was wrong, and measurably so. Against gofr.dev/pkg/gofr: gofr_nogrpc alone leaves 82 grpc packages, adding gofr_nootlp leaves 81, adding gofr_nodgraph still leaves 81, and only adding gofr_nopubsub reaches 0 -- the Google Pub/Sub client pins grpc through cloud.google.com/go. A shared dependency goes when its last importer does, which is a property of these tags worth documenting rather than a detail of this one. The slim-builds documentation added in #4167 covers all six tags accordingly, including the table of that measurement, and notes that gofr_nogrpc is the one tag that changes the API surface.
The tagged lint step ran unscoped, so its first CI run reported every pre-existing goconst and exhaustive finding in the tree rather than anything this branch introduced. only-new-issues matches what the code_quality job already does. The typos check wants US spelling in the new docs page.
|
@PiyushSingh-ZS @Umang01-hash — everything addressed at Tags vs modules: these three are a closed exceptionAgreed that modules are the right pattern, and that's the rule from here on. These three tags are deliberately limited to subsystems already compiled into core, where moving them out breaks existing users:
A new integration has no such users, so there is nothing to break — it goes in its own module, which is the existing pattern. I've written that into @Umang01-hash's three threads — all real, all fixed1. The CI step didn't check what its name claimed. Correct, and it was the worst kind of comment: it asserted the PR's central premise and verified nothing. It now diffs 2. The unknown-driver path had no test, and your point about the scaffolding masking it is exactly right. That's why the assertion is against a dialect registered in neither build rather than 3. The disabled pub/sub stubs had no assertion. Also right — skips prove nothing about what the stub does, and a dropped Also found while fixing those
@PiyushSingh-ZS's implementation notes
DocumentationThere was none for any of these tags — not in the PR, |
Description:
Three opt-in build tags. A build that sets none of them is byte-for-byte what it is today.
gofr_nosqldriversgofr_nopubsubgofr_nographqlMeasured on a minimal GoFr service (
gofr.New, one route,Run):gofr_nosqldriversgofr_nopubsubgofr_nographql−15.84 MB (−27.7%), −277 packages. Idle RSS drops too, driven by the drivers.
Why the drivers cost so much.
modernc.org/sqliteis blank-imported for driver registration and bringsmodernc.org/libc, whosenetdbinit parses embedded copies of/etc/protocolsand/etc/servicesinto permanent Go structs — about 1.7 MB of retained heap, roughly half the process's fixed heap floor. None of it is reachable from the HTTP path. The three pub/sub clients bring 213 packages between them, most of it Google Pub/Sub's gRPC and auth stack.The mechanism, and the part that is easy to get wrong. Making a subsystem optional takes two things: an interface on the consumer naming only the methods it calls, and a build tag on the concrete file.
graphQLRunnerleft all the graphql packages linked untilgraphql.goitself carried the tag — the concrete file still compiles the library in.Breaking Changes (if applicable):
None. Every tag is opt-in and the default build is unchanged.
GraphQLQueryandGraphQLMutationname only GoFr's ownHandler, so the exported surface is identical in both builds.Verified against
development: identicalgo doc -allacross 6 packages, and an identical linked-package list.A build that does set
gofr_nographqlstill compiles a user's GraphQL calls. Registering a resolver logs an error naming the tag rather than failing silently, andenabled()keeps App from routing/graphqland the playground against a handler this build cannot provide.setupGraphQLregisters a nil handler and the firstPOST /graphqlpanics.TestGraphQLDisabled_SetupRegistersNoRoutefails if the guard is removed.Additional Information:
noopRespondermoves fromresponder.gointographql.go, its only user. It cannot stay:responder.goalso holds the exportedResponderinterface so it cannot carry the tag, and leaving it there makes it dead code in a tagged build — whichgolangci-lintreports asunused, failing the lint gate for anyone who sets the tag.configTrueis declared insidepubsub_backends.gofor the same reason: an untagged home would leave it unused undergofr_nopubsub.Checklist:
goimportandgolangci-lint.