perf(deps): let a build omit the gRPC server, Dgraph migrator and OTLP exporters it does not use - #4168
Conversation
7989966 to
a969749
Compare
b9edbcb to
fcdd06d
Compare
PiyushSingh-ZS
left a comment
There was a problem hiding this comment.
Reviewed this on top of #4167. The conjunctive-not-additive analysis is the most useful thing in the PR and I think it is correct. My main feedback is about packaging rather than correctness: there are four production bug fixes in here that are more urgent than the build tags and are currently blocked behind them.
The four bug fixes should be their own PR, first
Listed under "Additional Information", all unrelated to build tags:
initTracerbuilt aBatchSpanProcessorover a nil exporter.getExporterreturns nil-and-no-error for an unsupportedTRACE_EXPORTER, so a typo in config is enough, and the processor panics on its first flush on its own goroutine. That is a crash in production reachable from a one-character config mistake.container.Error(err)logged{"message":{}}because the logger JSON-marshals the value anderrors.errorStringhas no exported fields — every trace-exporter failure logged nothing usable. The note that the same pattern is likely elsewhere is right and deserves its own issue.- Four exported gRPC methods dereferenced
App.grpcServerwithout a nil check, in a casefactory.goexplicitly treats as recoverable (it logs and continues whennewGRPCServerfails, e.g. an out-of-rangeGRPC_PORT). Four panics in an already-handled error path. TestUnifiedAuthenticationRegistrationcould not fail —>= 2when two are registered by default.
Each is small, independently reviewable and independently valuable. As things stand they can only land if maintainers also accept three build tags they may not have decided on yet, and #4168 is stacked on #4167 which is itself unresolved. (1) in particular looks like something worth getting in quickly on its own.
gofr_nodgraph may not need to exist
The PR's own finding is that pkg/gofr/migration is the only dgo importer in the root module. Following that through, it is a single expression:
// pkg/gofr/migration/dgraph.go
_, err = c.DGraph.Mutate(context.Background(), &api.Mutation{
SetJson: jsonPayload,
})One struct literal, in one place, is what pins github.com/dgraph-io/dgo/v210 — and through it google.golang.org/grpc — into the root go.mod for every GoFr user, including those who have never used Dgraph. Meanwhile pkg/gofr/datasource/dgraph is already its own module and already depends on dgo.
Since Mutate takes mu any and type-asserts on the other side, the payload can cross that boundary without naming dgo's type at all — a MutateJSON(ctx, []byte) on the DGraph interface, or accepting []byte in Mutate and constructing api.Mutation inside the datasource module where dgo already lives.
That removes the dependency for everyone with no build tag, no stub file, no CI matrix entry and no migrator-interface double-maintenance. It also seems strictly better than a tag worth 1.12 MB only to users who set it. Worth trying before committing to gofr_nodgraph.
(It touches the same call path as #4158, which is fixing Mutate's commit semantics right now — those two want sequencing.)
gofr_nogrpc is the one that needs a maintainer decision
I agree the API break is forced rather than a design choice: RegisterService, AddGRPCServerOptions, AddGRPCUnaryInterceptors and AddGRPCServerStreamInterceptors all name grpc types in their signatures, so a no-op stub is impossible and compiling them out is the only option. Scoping that CI step to ./pkg/... because the gRPC examples are supposed to fail under the tag is the right call too.
So the honest framing is: this is a tag that changes GoFr's exported API, and on its own it is worth 0.67 MB. That is a real trade and I think it should be put to maintainers as its own question rather than sitting inside a 900-line diff with five other things.
grpcRunner and the type-assertion guidance
CONTRIBUTING says, under take-interfaces-return-concrete-types:
Be careful of type assertions in this context. If you take an interface and type assert to a type - then it's similar to taking a concrete type.
App.grpcServer becomes grpcRunner, and then several call sites assert straight back out:
func (a *App) grpcSrv() *grpcServer {
g, ok := a.grpcServer.(*grpcServer)
...
}grpcRunner genuinely abstracts only Run and Shutdown; everything else goes through the assertion. I think that is forced by the grpc types too — but a second, concrete, tag-guarded field declared only in grpc.go (grpcConcrete *grpcServer) would give the same linkage result with no assertions at all, and would sit better with the guideline.
Smaller things
- The
auth.gosplit is the right instinct — "HTTP auth is not gRPC's to take away" — and keepingauth.gountagged so HTTP auth works in every build is correct. ButaddGRPCBasicAuth(users, validateFunc, validateFuncWithDatasources)with exactly one of three non-nil at every call site is a triple-nil flag-argument shape. A small options struct in an untagged package would carry the same information without threenils at each site. addGRPCInterceptorsguarding with!ok || g == nilis correct and worth calling out: now that the field is an interface,a.grpcServer != nilno longer catches a typed nil. That is the same class of bug #4164 is fixing in the container, caught proactively here.newGRPCRunnerreturning the interface rather than*grpcServer, specifically so a nil result cannot reachAppas a non-nil interface holding a nil pointer, with the nod tocontainer.isNil— good.disabledGRPC.Runlogs an error on a path the comment argues is unreachable (grpcRegisteredis only set byRegisterService, which does not exist in this build). Defensive and harmless; no objection.
CI
Noted that this targets perf/optional-datasources-graphql so the pipeline does not fire. Flagging it clearly in the description was the right thing to do — but it does mean nobody has a green signal on ~900 lines yet, which is another argument for pulling the bug fixes out where they can be tested on their own against development.
Umang01-hash
left a comment
There was a problem hiding this comment.
Verified end-to-end locally at the head SHA in a clean worktree: the tag mechanism works, each tag drops its tree (dgo→0, OTLP exporters→0, grpc server −316 syms), the full tagset takes the binary 56.5MB→20.6MB with grpc fully gone, and the default build links the identical dependency trees as base (28=28, gofmt/vet/tests clean). Nice work — the typed-nil handling, fail-loud stubs, and boundary comments are excellent.
Requesting changes only on CI rigor — two gaps that would let a future regression ship green:
- The "Default build links the same packages as before" step doesn't actually assert that; it only compiles. A
go list -depsdiff (or a grpc-count check) vs a baseline would make the step earn its name — and would guard the whole PR's value. - The three tags are only built in isolation. Combinations (esp. all-3) are never built. I confirmed they compile today, but nothing keeps that true.
Minor (non-blocking): the grpcSrv()==nil guard — a real panic→log fix — has no test; and the panic→log-and-return behavior change on AddGRPC*/RegisterService is worth a changelog line.
…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.
|
@Umang01-hash — all three threads addressed at 1. The all-tags combination is now builtYou're right, and it's the failure mode tags are most prone to: a symbol that resolves under one tag and breaks under two ships green and fails only for the user who sets both. CI now builds, vets and tests all six together. Verified locally — clean, and 412 packages against the default 829. Scoped to 2. The package-diff — inherited from #4167Same fix, same reasoning: 3. The nil-runner no-op is testedAgreed this needed locking in.
|
| Tags | google.golang.org/grpc packages |
|---|---|
| none | 82 |
gofr_nogrpc |
82 |
gofr_nogrpc gofr_nootlp |
81 |
gofr_nogrpc gofr_nootlp gofr_nodgraph |
81 |
+ gofr_nopubsub |
0 |
The OTLP exporters pin gRPC, but so does the Google Pub/Sub client through cloud.google.com/go. A shared dependency goes when its last importer does — so gofr_nootlp is necessary and not sufficient, and this PR's tags only pay off in combination with #4167's. That's a property of the tags worth documenting rather than a detail of this one, so it's in docs/advanced-guide/slim-builds with the table above.
⚠️ Worth knowing for whoever reviews this: CI barely runs here
This PR targets perf/optional-datasources-graphql, and go.yml triggers on pull_request: branches: [main, development] — so the 20-job matrix has never run on it. The only check is Snyk, which isn't branch-filtered. The green tick has been misleading since the PR opened.
Everything above was therefore verified locally: all six tags build, go vet and the tagged tests pass, and golangci-lint with all six tags reports 0 new issues. Real CI arrives the moment #4167 merges and this retargets to development; keeping it stacked until then keeps the diff reviewable.
akshat-kumar-singhal
left a comment
There was a problem hiding this comment.
Request changes — mainly the rebase. Reviewed as the delta over #4167 at 4e21215 (this PR lacks #4167's last three commits). ./pkg/... builds and vets for every tag combination; default go list -deps ./... matches its merge base exactly; moved tests (TestUnifiedAuthenticationRegistration, TestStartGRPCServer_Registered, OTLP Test_initTracer) are all still run. The grpc package table matches measurement exactly (86 / 82 / 81 / 81 / 0).
Blocking: conflicts with development, and the OTLP-trace half targets code that moved.
git merge-tree origin/development pr/4168 conflicts in pkg/gofr/otel.go, otel_test.go and grpc_test.go. #4206/#4207/#4247/#4249 moved the OTLP trace exporter into pkg/gofr/traces/exporters/otlp.go, which imports otlptracegrpc and self-registers. pkg/gofr/otlp_trace.go/otlp_trace_disabled.go split code out of an otel.go that no longer holds it — resolved mechanically, -tags gofr_nootlp still links otlptracegrpc and its grpc tree via traces/exporters, silently removing only the metrics half. Put the gofr_nootlp constraint on traces/exporters/otlp.go with a stub that registers "otlp"/"jaeger" and returns the tag error, then re-measure.
Should-fix: CI never checks the three new tags remove anything. Without 8803000 the "Each tag removes the packages it claims to" step doesn't cover gofr_nogrpc/gofr_nodgraph/gofr_nootlp. After rebasing onto #4167, add checks for go.opentelemetry.io/otel/exporters/otlp/, github.com/dgraph-io/dgo and google.golang.org/grpc under the combined tags.
Should-fix (doc): "The tags remove implementations, never API" — page.md:70-72
gofr_nogrpc removes RegisterService and AddGRPC*. go build -tags gofr_nogrpc ./... fails on examples/grpc/grpc-{unary,streaming}-server/server/health_gofr.go:37 (*gofr.App does not implement grpc.ServiceRegistrar (missing method RegisterService)), as will any gofr-cli-generated health_gofr.go — so the doc's own all-six command at L20 fails in this repo. Call out gofr_nogrpc as the exception.
Should-fix (doc): wrong settings for gofr_nootlp — page.md:34: GoFr never reads OTEL_EXPORTER_OTLP_ENDPOINT, and TRACER_URL still works for zipkin/gofr. The affected settings are TRACE_EXPORTER=otlp|jaeger and METRICS_EXPORTER=otlp.
Should-fix (doc): gofr_nodgraph is broader than "Dgraph migrations". migration.go chains the Dgraph migrator whenever c.DGraph is set; the stub's checkAndCreateMigrationTable errors and migration.go:82 calls c.Fatalf. A service with a Dgraph datasource and only SQL migrations exits at app.Migrate under this tag. Exiting loudly is defensible; the doc should say so.
Nit — otlp_trace_disabled.go:22: says "TRACE_EXPORTER=otlp is unavailable" even for TRACE_EXPORTER=jaeger, which takes the same path.
Nit (PR description): default-build behavior changes worth mentioning, both improvements: AddGRPC*/RegisterService now log and return when newGRPCServer failed (e.g. bad GRPC_PORT) instead of panicking on a nil *grpcServer; initTracer skips the BatchSpanProcessor for a nil exporter (fixes a panic on an unsupported TRACE_EXPORTER) and switches Error(err) to Errorf, changing the log format.
Total counts are ~20 above my linux/amd64 go1.26.0 measurements (e.g. none 809 vs 829; all six 415 vs 412) — the same platform offset as #4167. gofr_nodgraph alone removes 2 packages, but combined it matters: nogrpc,nootlp,nopubsub without it still links 64 grpc packages (dgo protos pin grpc); with it, 0.
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.
… 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.
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.
Building and testing under a tag proved the tagged code compiled; nothing asserted it dropped anything, which is the only reason the tags exist. Dropping the build constraint from drivers.go, or an import elsewhere pulling lib/pq back in by another path, left the Slim Build Tags job green. The new step diffs `go list -deps gofr.dev/pkg/gofr` per tag against the default build and fails if lib/pq, modernc.org/sqlite, kafka-go, cloud.google.com/go/pubsub, paho.mqtt.golang or graphql-go is still linked. It uses the package path, not ./..., because a pattern lists its own matches whether or not anything links them. It also fails when a pattern is absent from the DEFAULT build, so a renamed module cannot make it pass vacuously. This is the assertion the sql package cannot make from a test: drivers_testdeps_test.go blank-imports pq and modernc.org/sqlite because TestNewSQL_GetDBDialect opens a postgres DSN, so the test binary has both drivers registered under the tag too. What the library links is only observable from outside that binary. Renames drivers_disabled_test.go to drivers_registration_test.go: it has no build tag on purpose, and a _disabled_test.go name next to a tagged drivers_disabled.go reads like one that was forgotten. Measured on this branch: gofr.dev/pkg/gofr links 829 packages by default, 785 with gofr_nosqldrivers, 617 with gofr_nopubsub, 809 with gofr_nographql and 553 with all three.
…pment The figures were measured before this branch merged development, which changed the default set by one package, so every number in the section was off by one. Re-measured at this commit with `go list -deps gofr.dev/pkg/gofr`: 828 by default, 616 / 784 / 808 per tag and 552 with all three. Adds the binary-size figure the section was missing -- examples/http-server goes from 60,028,914 to 43,357,218 bytes, 27.8% -- and says plainly that these move with dependencies, so nobody reads them as a contract. The contract is the Slim Build Tags job, which fails if a tag stops removing the packages it names.
…tests compiling Rebased onto development, which brought in two untagged tests that do not hold under a tag, plus akshat-kumar-singhal's review of the CI step. **The "Default build links the same packages as before" step is removed.** It diffed `go list -deps ./...` against the merge base across the whole root module, and this job runs on every pull request -- so it meant "no PR may add a package or a dependency, ever". That is not a rule this repo has, and it would have gone red on changes with nothing to do with build tags. What it was protecting is already covered, and better, by the step above it: `grep -Eq "$pattern" /tmp/deps.default` fails when a tagged-out package is missing from the DEFAULT build, so the per-tag checks cannot pass vacuously and an untagged build cannot quietly stop linking them. A comment says so, and asks that the broad diff not be re-added. **Two tests from development needed a home under the tags.** TestApp_setupGraphQL_MissingSchema (#4274) names errSchemaMissing, which graphql.go compiles out; it moves from the untagged gofr_test.go into graphql_test.go, which already carries !gofr_nographql. TestContainer_createKafkaPubSub_InvalidConfigs asserts what kafka.New's own validation logs, and under gofr_nopubsub there is no kafka.New -- it takes the pubsubBackendsLinked skip the file already uses for three other tests. Both failed `go vet`/`go test` under their tag while the non-test build stayed green, which is the tagged-build blind spot this PR's CI job exists for: it caught them. **The doc now states where its counts were measured** -- darwin/arm64, Go 1.26.3 -- and that they are platform-dependent, so a linux/amd64 reader seeing roughly twenty fewer knows why the absolute numbers differ and the deltas do not. Verified across all five tag configurations: build, vet and test clean. typos clean, gofmt clean, go.yml parses. golangci-lint reports 5 issues against 7 on development, none new.
2139269 to
f1634f2
Compare
-tags gofr_nogrpc leaves out GoFr's gRPC server, its reflection and
health services and the recovery middleware. Measured on a minimal GoFr
service, alongside the other three tags:
nosqldrivers+nopubsub+nographql 41.26 MB 554 packages
... and nogrpc 40.59 MB 543 packages
0.67 MB and 11 packages, which is small, and the reason is worth stating
plainly rather than burying: 66 packages of google.golang.org/grpc stay
in the binary regardless, because the OTLP exporters pin them. Both
otlptracegrpc (pkg/gofr/otel.go) and otlpmetricgrpc
(pkg/gofr/metrics/exporters) speak gRPC, and even otlpmetrichttp imports
it for status codes. pkg/gofr/migration pins a fourth copy through
dgraph's protobufs. Until those are optional too, this tag can only
remove GoFr's own gRPC layer, not gRPC.
So the point of this change is the shape, not today's 0.67 MB. App's
field becomes grpcRunner, an interface naming the two methods the
lifecycle actually calls -- Run and Shutdown -- and grpc.go carries the
tag. As with GraphQL, both halves are needed: an interface field does
not stop the concrete file from linking the library in.
Unlike the datasource and GraphQL tags, this one does change the
exported API under the tag. RegisterService, AddGRPCServerOptions,
AddGRPCUnaryInterceptors and AddGRPCServerStreamInterceptors all name
grpc types in their signatures, so they cannot exist without the import
and are compiled out. A build that sets the tag and calls them fails to
compile, which is the honest outcome -- it asked for a binary without
gRPC and then used gRPC -- and it is why the CI step for this tag builds
./pkg/... rather than ./...: the gRPC examples are supposed to fail
under it. Nothing changes for a default build, which keeps all four
methods and links the same packages.
HTTP auth is not gRPC's to take away. EnableBasicAuth and the other five
entry points build gRPC interceptors, which names
pkg/gofr/grpc/middleware, which imports grpc -- so auth.go could not
stay untagged while doing it. The gRPC half now lives in
grpc_auth_enabled.go behind three bridges that take plain credentials;
auth.go keeps the HTTP half in every build, and the no-op counterparts
in grpc_auth_disabled.go skip only the interceptors.
Two fixes fall out of the refactor:
TestUnifiedAuthenticationRegistration asserted the server had >= 2
interceptors after three Enable calls. Two are registered by default, so
it passed on the defaults alone and would not have noticed any of the
three auth bridges going missing. It now asserts exactly five, and fails
if one is removed.
AddGRPCServerOptions, the two interceptor setters and RegisterService
dereferenced App.grpcServer without checking it. factory.go leaves it
nil when newGRPCServer fails -- an out-of-range GRPC_PORT is enough --
and logs and continues, so all four panicked on a nil pointer in a case
the surrounding code treats as recoverable. They now log and return.
pkg/gofr/migration is the only package in GoFr that imports
github.com/dgraph-io/dgo, and it imports it for one value. DGraph.Mutate
takes any, but the driver behind it type-asserts to *api.Mutation, so
commitMigration has to construct one:
_, err = c.DGraph.Mutate(context.Background(), &api.Mutation{...})
That single construction links dgo's protobuf tree into every GoFr
binary, whether or not the service has ever heard of Dgraph. The
container's own Dgraph interface is dgo-free -- its methods take and
return any -- so nothing else in the default path depends on it.
-tags gofr_nodgraph leaves it out. Measured on a minimal GoFr service:
default 57.10 MB 831 packages
-tags gofr_nodgraph 56.01 MB 829 packages
the other four tags 40.59 MB 543 packages
... and nodgraph 39.47 MB 541 packages
1.12 MB for two packages, which is worth saying plainly: dgo drags in
google.golang.org/grpc, but so do the OTLP exporters, so removing dgo
alone drops only dgo. These tags are conjunctive on the gRPC tree, not
additive. With the OTLP transports also optional, this tag and
gofr_nogrpc together take the same service to 35.35 MB and 414 packages
with zero gRPC packages linked.
A build that sets the tag and then configures Dgraph does not silently
skip its migrations. checkAndCreateMigrationTable returns an error
naming the tag, and Run treats that as fatal, which is what stops a
migration being recorded as applied when it was not. The stub keeps
implementing migrator in full, so the interface cannot drift out from
under it unnoticed -- the new CI step compiles and tests it.
This is the tag that actually releases google.golang.org/grpc. The OTLP
exporters are what pin it in a GoFr binary -- otlptracegrpc from
otel.go, otlpmetricgrpc from metrics/exporters, and otlpmetrichttp too,
which imports grpc for its status codes -- so gofr_nogrpc and
gofr_nodgraph each removed their own package and left the 66-package
gRPC tree standing. With gofr_nootlp set, all three go and the tree goes
with them.
Measured on a minimal GoFr service (gofr.New, one route, Run):
default 57.10 MB 831 pkgs 86 grpc
+nosqldrivers,nopubsub 42.07 MB 574 pkgs 70 grpc
+nographql 41.26 MB 554 pkgs 70 grpc
+nogrpc 40.59 MB 543 pkgs 66 grpc
+nodgraph 39.47 MB 541 pkgs 66 grpc
+nootlp 35.35 MB 414 pkgs 0 grpc
4.12 MB and 127 packages for this tag alone, and 57.10 -> 35.35 MB
(-38.1%) and 831 -> 414 packages (-50.2%) for the six together. The
tags are conjunctive on that tree, not additive: this one is worth 127
packages only because the other two went first.
What the tag removes is a transport, not observability. Prometheus is
untouched and is still the default metrics exporter; gcp brings its own
transport; TRACE_EXPORTER=zipkin and gofr do not go through OTLP and
still export. What a tagged build cannot do is speak OTLP, and it says
so: "otlp" stays registered in the metrics registry and fails at the
point it would open the connection, rather than vanishing and letting
METRICS_EXPORTER=otlp quietly resolve to a different exporter than the
operator configured. Both errors name the tag and the alternatives.
Two defects came out of the code this touches, both pre-existing and
both on the path the tag exercises:
initTracer built a BatchSpanProcessor over whatever getExporter
returned, without checking for nil. getExporter returns a nil exporter
and a nil error for an unsupported TRACE_EXPORTER -- a typo in config is
enough -- and a nil exporter with an error whenever an exporter fails to
build. The processor panics on its first flush, on its own goroutine.
It now returns instead, leaving the tracer provider installed so trace
and span IDs still appear on every request.
initTracer reported exporter failures with container.Error(err), which
JSON-marshals the error value; an errors.errorString has no exported
fields, so every such failure logged {"message":{}} and the reason was
lost. It uses Errorf now. The same pattern is likely elsewhere and is
worth a sweep of its own.
…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.
… 82 and is 86 The composition table in the slim-builds guide opened with 82 grpc packages linked by default and 82 under gofr_nogrpc, which reads as the tag removing nothing at all. Measured against gofr.dev/pkg/gofr the default is 86, so the tag does take 4; the point the table is making -- that the remaining 82 leave only when the last importer is tagged out -- is stronger with the right baseline than with a wrong one that overstates it. The go.yml comment quoting the same figures gains the 86 for the same reason: 'leaves 82' is unreadable without the number it is a reduction from.
… and correct the docs akshat-kumar-singhal's blocking finding was right, and the measurement is unambiguous. #4206/#4207/#4247/#4249 moved the OTLP trace exporter to pkg/gofr/traces/exporters/otlp.go, where it self-registers and imports otlptracegrpc. This PR's gofr_nootlp split code out of an otel.go that no longer holds it, so the tag removed the metrics half and nothing else: otlptrace packages linked, before this commit default: 10 gofr_nootlp: 10 <- the tag removed none of them after default: 10 gofr_nootlp: 0 The constraint now sits on traces/exporters/otlp.go, with otlp_disabled.go registering "otlp" and "jaeger" to a builder that fails naming the tag. Registering rather than omitting the names is deliberate: Build's unknown-exporter path would report TRACE_EXPORTER=otlp as a name it does not recognize, which reads like a typo and sends an operator to check their spelling. The stub tells them the name is right and the binary does not carry it. Build's nil-exporter path then installs the NeverSample provider, so the service still starts with working correlation IDs. The obsolete pkg/gofr/otlp_trace*.go files are dropped. The metrics half (metrics/exporters/otlp_transport*.go) is unchanged and still correct. **CI now checks the three new tags.** gofr_nootlp and gofr_nodgraph get their own patterns; gofr_nogrpc is checked against the combined tag set because grpc is pinned by the OTLP exporters, the Dgraph protos and cloud.google.com/go as well as by the server -- alone it takes 4 of 86 packages, so a bare "still linked" assertion would fail for reasons unrelated to the tag working. All six checks pass locally. **Three doc corrections, all as reported:** - the all-six build command dropped gofr_nogrpc, because ./... cannot build with it in this repo; the exception is stated where the doc used to claim the tags never remove API - gofr_nootlp's settings are TRACE_EXPORTER=otlp|jaeger and METRICS_EXPORTER=otlp. GoFr never reads OTEL_EXPORTER_OTLP_ENDPOINT, and TRACER_URL still works for zipkin and gofr - gofr_nodgraph is broader than "Dgraph migrations": migration.go chains the Dgraph migrator whenever c.DGraph is set, so a service with a Dgraph datasource and only SQL migrations exits at app.Migrate Counts re-measured on darwin/arm64, Go 1.26.3: 828 default, 786 nootlp, 784 nosqldrivers, 808 nographql, 818 nogrpc, 826 nodgraph, 411 with all six. redact_test.go's four OTLP cases take an otlpTraceLinked skip, mirroring container.pubsubBackendsLinked. Verified: build and vet clean for default, each tag and all six; tests pass untagged and under all six; typos, gofmt and golangci-lint clean.
ac84f21 to
238cf65
Compare
|
Thanks @akshat-kumar-singhal — the blocking finding was correct and the measurement is unambiguous. Rebased onto the updated #4167 (
|
| tag | packages |
|---|---|
gofr_nosqldrivers |
828 → 784 |
gofr_nopubsub |
828 → 616 |
gofr_nographql |
828 → 808 |
gofr_nootlp |
828 → 786 |
gofr_nodgraph |
828 → 826 |
nogrpc,nootlp,nodgraph,nopubsub |
828 → 475 |
All three doc corrections
gofr_nogrpcremoved from the all-six build command, because./...genuinely cannot build with it in this repo — the doc's own command was failing. The exception is now stated where the page used to claim the tags never remove API.gofr_nootlp's settings areTRACE_EXPORTER=otlp|jaegerandMETRICS_EXPORTER=otlp. You were right that GoFr never readsOTEL_EXPORTER_OTLP_ENDPOINTand thatTRACER_URLstill works for zipkin and gofr.gofr_nodgraphis broader than "Dgraph migrations" — it has its own section now, saying plainly that a service with a Dgraph datasource and only SQL migrations exits atapp.Migrate, and why failing loudly is the right call.
Counts re-measured on darwin/arm64, Go 1.26.3 (stated in the doc, per your platform note): 828 default, 786 nootlp, 784 nosqldrivers, 808 nographql, 818 nogrpc, 826 nodgraph, 411 with all six.
One more tagged-build break the rebase surfaced
redact_test.go's four OTLP/jaeger cases assert what the real builder logs, so they fail once the stub is what is registered. They take an otlpTraceLinked skip, mirroring container.pubsubBackendsLinked. That is the third instance in this stack of an untagged test not surviving a tag — worth noting as the ongoing maintenance cost @PiyushSingh-ZS predicted.
Verification
Build and vet clean for default, each tag alone, and all six together. Tests pass untagged and under all six (container, datasource/sql, migration, traces/exporters, metrics/exporters). typos, gofmt and golangci-lint clean.
Base left at perf/optional-datasources-graphql deliberately. I briefly retargeted it to development to get CI on it and reverted immediately — stacked as it is, that makes the PR show #4167's diff as well. It still means this has had almost no CI, which is the one thing about it I cannot fix from here; everything above is local.
@PiyushSingh-ZS — your mechanism question and the "four bug fixes should ship first" point are both still open and both still stand. Nothing here answers them; this only makes the tag do what it claims.
…ropped 194a60f added a step building, vetting and testing all six tags together and widened the tagged lint to all six. Both were lost when the branch was rebased onto the updated #4167, whose own lint step carries only its three tags, so no CI step built nogrpc/nodgraph/nootlp in combination and the tagged files this PR adds were never linted. Restoring the lint surfaced two wsl_v5 findings in otlp_disabled_test.go that no job would have reported; the test now reads the registry through the package's lookup helper.
@Umang01-hashAll three threads from 2026-09-18, re-checked at 1. Package-set diff. I added the What guards this PR now is the per-tag check at
2. The all-tags combination: this regressed, and it is restored now. The six-tag build/vet/test step and the six-tag lint I described on 2026-09-23 were lost in the 2026-09-24 rebase. From that point until
Restoring the lint found 2 3. Nil-runner no-op. @akshat-kumar-singhalEverything in your 2026-09-24 review is in place at
Your PR-description nit: the description is now stale in the other direction. Bugs 1 and 2 describe |
Umang01-hash
left a comment
There was a problem hiding this comment.
Reviewed at head 3f357a31 (against #4167), verified locally — no blockers.
- All six tags strip correctly: grpc goes 86 → 0 packages with the combined set (matching the note that grpc only unlinks once its last importer, Google Pub/Sub, is gone). Disabled stubs fail loudly naming the tag.
- The auth split is behavior-preserving: traced every
Enable*Authold→new, providers are built identically and applied under the same nil-guard.-raceauth+gRPC tests pass; unary + streaming gRPC example integration tests pass on the default build. - gofmt/vet clean, golangci-lint 0 new issues (default + all-6-tags).
One thing to flag rather than block: gofr_nogrpc is the one tag that removes exported methods (RegisterService, AddGRPC*, which name grpc types) — so it's a conditional API change gated behind an opt-in tag. The default (untagged) API is unchanged, so no semver break, and CI correctly scopes that tag to ./pkg/... since the gRPC examples can't compile under it (documented + intended). Commenting rather than approving just so this exported-surface property is explicitly on the record.
Otherwise clean and merge-ready after #4167.
…grpc-dgraph-otlp # Conflicts: # .github/workflows/go.yml # docs/advanced-guide/slim-builds/page.md
… as providers Two points from review. App.grpcServer was a grpcRunner interface so that gofr_nogrpc could swap the implementation, but the AddGRPC* setters, RegisterService and the auth bridge all type-asserted straight back to *grpcServer to reach methods that name grpc types. The interface abstracted Run and Shutdown and nothing else, which CONTRIBUTING calls out: asserting an interface back to a type is taking the concrete type anyway. grpc_disabled.go now declares its own grpcServer and newGRPCServer under the tag instead, so App holds *grpcServer in both builds. grpcRunner, grpcSrv and newGRPCRunner are gone, and gofr.go, factory.go and run.go are back to exactly what development has. The typed-nil concern that newGRPCRunner existed for goes with the interface: a nil *grpcServer is just nil. The four nil guards that fixed the panic on a failed server build stay, and TestGRPCSetters_NoServerIsANoOp still fails all four subtests when they are removed. addGRPCBasicAuth and addGRPCAPIKeyAuth took three arguments of which every caller set exactly one. They now take the HTTP middleware's BasicAuthProvider and APIKeyAuthProvider, which already carry the same fields, live in an untagged package, and are what auth.go builds for the HTTP half anyway. The gRPC provider still always gets the app's container, as before.
@PiyushSingh-ZSSorry this took so long. Your review from 2026-09-16 never got a reply. Here is each point, checked at
|
The tagged lint ran over ./... with gofr_nogrpc set, so the two gRPC example servers failed typecheck on RegisterService, which that tag removes. only-new-issues does not filter typecheck errors, so the step failed on the first CI run this PR has had. The build step for the same tag set is already scoped to ./pkg/... for this reason; the lint now is too. Reproduced locally with the CI invocation: 1 typecheck issue on ./..., 0 on ./pkg/...
|
CI is green at On The other failure on that run was @akshat-kumar-singhal @Umang01-hash: your change requests are still standing from |
akshat-kumar-singhal
left a comment
There was a problem hiding this comment.
Re-reviewed at 193b43f02. Everything in my 09-24 review is addressed, and I checked it against the code rather than the replies. Three doc points below.
Verified locally (darwin/arm64):
- Merges cleanly into current
development. traces/exporters/otlp.gocarries!gofr_nootlp.otlp_disabled.goregisters "otlp" and "jaeger" to a builder that names the tag and the configured exporter.- Packages linked by
gofr.dev/pkg/gofr:
| tags | otlp | grpc | dgo |
|---|---|---|---|
| none | 28 | 86 | 1 |
gofr_nootlp |
0 | 86 | 1 |
gofr_nodgraph |
28 | 86 | 0 |
gofr_nogrpc |
28 | 82 | 1 |
nogrpc,nootlp,nodgraph,nopubsub |
0 | 0 | 0 |
| all six | 0 | 0 | 0 |
go build ./pkg/...andgo vet ./pkg/gofr/...pass with no tags, with each new tag alone, and with all six.- traces/metrics/migration tests and the root gRPC/auth tests pass untagged and under all six.
- The CI checks at
go.ymlcover all three new tags, withgofr_nogrpcchecked under the combined set. The tagged lint is scoped to./pkg/....
Non-blocking doc points, page.md:
- L79-83 leaves Dgraph out of the list of things that pin gRPC. The prose names only the OTLP exporters and Google Pub/Sub. But
nogrpc,nootlp,nopubsubwithoutgofr_nodgraphstill links 64 grpc packages; adding it gives 0. The table hides this because it addsgofr_nodgraphbeforegofr_nopubsub. A reader who follows the prose and skipsgofr_nodgraphwould get 64 and conclude the tags don't work. Suggest naming the Dgraph protos alongside the other two. - L107-109 ("The service still starts") and L127-129 ("The tags change nothing about how a service behaves at runtime") are contradicted by the new
gofr_nodgraphsection at L50-59: a service with a Dgraph datasource exits atapp.Migrate. Both need a "exceptgofr_nodgraph" qualifier. - The
gofr_nogrpcexception is stated three times: L25-29, L61-64 and L91-95. L61-64 is inside thegofr_nodgraphsection, where it reads as misplaced. L25-29 plus L91-95 are enough.
I did not re-diff the default (untagged) package set against development. My 09-24 check covered it, and nothing since touches go.mod or an untagged import.
…time claims - The gRPC composition table now shows each of the four tags left out in turn. The old rows added gofr_nodgraph before gofr_nopubsub, which hid that github.com/dgraph-io/dgo/v210/protos/api pins gRPC too: without gofr_nodgraph, nogrpc+nootlp+nopubsub still links 64 grpc packages. - "The service still starts" and "the tags change nothing at runtime" now carry the gofr_nodgraph exception: a service with a Dgraph datasource exits at app.Migrate. - Drop the third statement of the gofr_nogrpc API exception from inside the gofr_nodgraph section; the intro and "Nothing in your code changes" already state it.
|
@akshat-kumar-singhal Thanks. All three doc points are fixed in
This is a docs-only change: |
NitinKumar004
left a comment
There was a problem hiding this comment.
I checked this out at 55cbaf68 and went through the default build and the tagged builds separately. The design holds up. The default build is unchanged in every way I could measure, and the size numbers reproduce. There are two things I'd fix before merge, plus a handful of smaller notes.
What I verified
Default build (no tags)
go doc -allis byte-identical todevelopmentforpkg/gofr,metrics/exporters,traces/exportersandmigration.go list -deps ./...is identical: 859 packages on both.- Auth refactor. I ran a probe over all 7 entry points (
EnableBasicAuth,…WithFunc,…WithValidator,EnableAPIKeyAuth,…WithFunc,…WithValidatorandEnableOAuth) on base and PR, and the output is byte-identical:- the same unary and stream interceptor counts, in the same order;
- HTTP 200/401 and gRPC accept/reject match;
- validators receive
app.containeron both transports.
- OTLP metrics after the
otlp.go→otlp_transport.gosplit, throughexporters.BuildwithMETRICS_EXPORTER=otlp:- HTTP to a path-less URL still posts to
/v1/metrics, so the #4351 fix survives the move; - gRPC still hits
MetricsService/Export.
- HTTP to a path-less URL still posts to
- Tests, lint and coverage:
go build ./...,go vet ./..., andgo test -raceonpkg/gofr,grpc/...,metrics/...,traces/exportersandmigrationare all green.examples/grpcfails identically on base (theTestBiDiStreamraces and the client-wrapper tests), so those failures are pre-existing.- Full golangci-lint v2.12.2 on
./pkg/gofr/...gives 202 issues on both trees, with an empty diff. pkg/gofrcoverage goes 95.7% → 96.0%.
Tagged builds
- Every combination I tried builds
./pkg/...and vets./pkg/gofr/...: each new tag alone, each with the three #4167 tags, all six, and the three pairs. - Every
//go:buildis an exactX/!Xpair on line 1. No file is on both sides or on neither. - Sizes on
using-add-rest-handlersare within 0.2% of the table: all six gives 35.92 MiB and 413 packages, against 35.88 and 414 in the table. The composition table inslim-builds/page.mdreproduces exactly. One addition: the "only the last tag releases the grpc tree" point holds for whichever of the four tags goes last, not onlygofr_nopubsub. - Runtime:
gofr_nodgraphwith a Dgraph migration logsFATAL … dgraph migrations are unavailable …, exits 1, andUPnever runs.gofr_nogrpccompiles out exactly the four gRPC-typed methods, keeps HTTP auth (401 without credentials, 200 with), and logs nothing misleading about gRPC.
Should fix
1. The root package's tests fail under gofr_nootlp, and CI doesn't run them.
$ go test -tags gofr_nootlp -run 'Test_initTracer_invalidConfig|Test_App_initTracer|Test_initTracer_doesNotLogCredentials' ./pkg/gofr/
--- FAIL: Test_initTracer_invalidConfig (gofr_test.go:838)
--- FAIL: Test_App_initTracer/{otlp,otlp_over_TLS,jaeger_on_the_deprecated_host_and_port} (otel_test.go:51)
--- FAIL: Test_initTracer_doesNotLogCredentials/{otlp_userinfo,jaeger_ignored_TRACER_INSECURE} (otel_test.go:509)
The same happens in every combination that includes gofr_nootlp: alone, with the old three, and all six. The tests are untagged, but they assert what the real OTLP builder logs. Under the tag the stub logs the tag error instead. In invalidConfig, a missing TRACER_URL now gets the tag error rather than "missing TRACER_URL".
The equivalent tests in traces/exporters are gated through otlpTraceLinked, but that's unexported, so pkg/gofr can't reuse it. CI misses this for two reasons:
- the nootlp step only tests
./pkg/gofr/metrics/exporters/and./pkg/gofr/traces/exporters/; - the all-six step runs the root package with
-run 'TestGRPCDisabled|TestGraphQLDisabled'.
Running the whole ./pkg/gofr/ under all six takes about 7s locally, and it would catch runtime regressions, not just compile breaks. Gating these cases with a root-level linked constant, or a tagged split like the one the exporters use, would fix the tests. Running the full root package under all six in CI would keep them fixed.
2. In the default build, a bad GRPC_PORT now degrades silently where it used to crash.
I set GRPC_PORT=99999 (anything above 65535 reaches this; non-numeric or ≤0 falls back to 9000) and called RegisterService:
- base:
panic: runtime error: invalid memory address or nil pointer dereference - PR:
ERROR no gRPC server to register service grpc.health.v1.Health on, and then it returns.
grpcRegistered stays false, so startGRPCServer never runs. A gRPC-only service therefore comes up healthy with only the metrics server, never listens on a gRPC port, and only exits when it is signalled.
Fixing the nil panic is right. But everything else on this path treats "a server that was asked for but can't be served" as fatal:
- a blocked port is
Fatalf(grpc.go:206); ensureServerfailing inRunisFatalf(:199);- the container injection inside
RegisterServiceisFatalf(:289); - #3942's
bindMCPServeraborts startup.
Fatalf in RegisterService when there's no server would keep the loud failure with a clear message. Alternatively, record it and let Run abort through startupOutcome. Log-and-return is fine for the three AddGRPC* setters, since RegisterService always follows them. If you keep log-and-return, the release notes should say that such an app no longer exits.
Smaller
-
The auth bridges' field mapping isn't pinned. The refactor adds a copy step, from the HTTP provider to the gRPC one, where a field can be dropped.
TestUnifiedAuthenticationRegistrationnow checks there are exactly 5 interceptors, but not what they do. These mutations pass the fullpkg/gofrandpkg/gofr/grpc/...suites:addGRPCBasicAuthwithContainer: nil(grpc_auth_enabled.go:33). AWithValidatorfunc would get a nil container on gRPC.addGRPCBasicAuthwithValidateFunc: nil.EnableBasicAuthWithFuncthen rejects every gRPC call.EnableAPIKeyAuthWithValidatorpassing an empty provider (auth.go:89).
A small table test over the six
Enable*variants would catch all three. It would run the appended unary and stream interceptors against good and bad metadata, and check that the container the validator sees isapp.container. Base has the same gap, so this isn't a regression. -
TestGRPCDisabled_HTTPAuthStillWorks(grpc_disabled_test.go:44) only assertsNotPanics. An httptest request checking 401 without credentials and 200 with them would pin the thing its name promises. -
Docs:
- "No source changes".
docs/navigation.js:90and theslim-buildsfront matter (lines 2 and 6) still say "without changing a line of your code" / "no source changes".gofr_nogrpcis the exception, and the page body already says so at L25-29 and L90-94. - "Logs an error naming the tag". L96-99 says this about configuring something the binary lacks, but under
gofr_nogrpcnothing is logged: withGRPC_PORTset, nothing listens and nothing is said. That's correct, because the failure there is at compile time, but the sentence needs a nogrpc qualifier. - gRPC metrics disappear. Under
gofr_nogrpc,registerGRPCMetricsdoesn't run.grpc_server_status,grpc_server_errors_total,grpc_services_registered_totalandapp_grpc_rate_limit_exceeded_totalthen disappear from/metrics, and default builds export them even for HTTP-only apps. Worth a line for anyone with dashboards. - What "loud" means for
gofr_nootlp. It's one ERROR at boot plus a fallback to Prometheus and NeverSample, and the app keeps serving. That's consistent with the existing exporter-misconfiguration path, but the page could say exactly that. - No cross-links. The grpc, migrations and observability pages and
references/configs(TRACE_EXPORTER/METRICS_EXPORTER) don't mention the tags. #4167 has the same gap.
- "No source changes".
-
The error messages differ across tags:
- traces say
TRACE_EXPORTER=%s was configured, but this binary was built with -tags …, matching #4167's pubsub message; - metrics say
METRICS_EXPORTER=otlp is unavailable: …; - Dgraph is wrapped as
failed to create gofr_migration table, err: dgraph migrations are unavailable …, where the table prefix is misleading.
Two related oddities:
- traces log the error twice, once in the builder and once in the
Buildcaller; METRICS_EXPORTER=otlpwith an emptyMETRICS_URLreportserrEmptyOTLPEndpointfirst, so the user fixes the URL and then hits the tag error.
- traces say
-
Deleted
Test_initTracercases.gofr_test.golost four jaeger/otlp cases: schemelesslocalhost:4317, with and withoutTRACER_AUTH_KEY. Line coverage is unchanged, but that combination throughApp.initTraceris no longer tested. They could come back behind the same linked-constant skip as item 1. -
CI coverage:
gofr_nogrpcis only checked inside the four-tag set. A single-tagcheck gofr_nogrpc '^gofr\.dev/pkg/gofr/grpc($|/)'is cheap.- The nogrpc build only covers
./pkg/..., so the non-gRPC examples are never compiled under it. I checked, and only the two gRPC servers fail.go build -tags gofr_nogrpc $(go list ./... | grep -v /examples/grpc/)would cover the rest.
-
Stale comments.
grpc_test.go:711says the tests moved "when App.grpcServer became an interface", but it's a concrete*grpcServeragain.:750, "Read through getServer rather than the field", now reads the field. -
Godoc. The four gRPC methods' docs don't mention that
-tags gofr_nogrpcremoves them, and pkg.go.dev only renders the untagged build.AddGRPCServerStreamInterceptorshas no doc comment at all, which is pre-existing.
CI cost for reference: the new steps took about 3m13s in run 36554878360, inside a 7m54s job.
Umang01-hash
left a comment
There was a problem hiding this comment.
Verified independently at head eb33c65 — builds/vet/tests under each tag, all six at once, and golangci-lint --new-from-rev under all six (0 issues).
The part that matters for a build-omission PR — the dep is actually gone, not just still compiling — checks out via go list -deps: gofr_nootlp otlp-exporter 28→0, gofr_nodgraph dgo 1→0, all grpc-importers off → grpc 86→0, and gofr_nogrpc alone 86→82 (conjunctive, as documented). Default build's exported API is diff-identical to development; the 4 removed gRPC methods only go under the opt-in tag.
Loud-fail behavior confirmed for all three (dgraph refuses, METRICS_EXPORTER=otlp fails at connect, TRACE_EXPORTER=otlp/jaeger registers a failing builder). Revert-to-red holds: neutralizing the 4 nil-guards fails all 4 TestGRPCSetters_NoServerIsANoOp subtests. Nice catch on the two pre-existing defects along the way.
LGTM.
# Conflicts: # pkg/gofr/auth.go # pkg/gofr/gofr_test.go
…fr_nootlp Test_App_initTracer, Test_initTracer_doesNotLogCredentials and Test_initTracer_invalidConfig configure TRACE_EXPORTER=otlp or jaeger, which a -tags gofr_nootlp build omits, so they failed under that tag. Move them unchanged into otel_otlp_test.go behind //go:build !gofr_nootlp.
…ogrpc gofr_nogrpc removes App.RegisterService, so the unary and streaming server examples failed to build under it and CI scoped that tag to ./pkg/... to step around them. Tag their gofr-dependent files //go:build !gofr_nogrpc -- the gofr-cli wrappers, the server implementations, their tests and main -- leaving the protoc output untouched, and build ./... under gofr_nogrpc and under all six tags so a regenerated file that loses the tag fails CI.
development made EnableBasicAuth return an error. The merge updated the untagged callers, but grpc_disabled_test.go compiles only under gofr_nogrpc, so errcheck in the tagged lint step was the first thing to see it.
Umang01-hash
left a comment
There was a problem hiding this comment.
Verified locally against head 8201bea (Go 1.26):
- All 5 build configs compile (default, nogrpc, nodgraph, nootlp, combined); default + all disabled-build tests green.
- Tags actually drop the trees they claim: nootlp 28→0 otlp pkgs, nodgraph 1→0 dgo, all-4 tags 86→0 grpc — matches the docs table.
- No breaking change for default users:
go doc ./pkg/gofris byte-identical dev vs head. - Auth split is behaviour-preserving (grpcServer is set eagerly in factory.go, so the nil-guard matches the old
if grpcServer != nil). Live E2E on the auth example: valid key 200, invalid/none 401, health 200, metrics present. - Removed gofr_test.go/otel_test.go tests are relocated into correctly-tagged files, not dropped; disabled-build tests carry real assertions (incl. HTTP auth still works under gofr_nogrpc).
- CI additions are solid: per-tag + combined build/vet/test and go-list-deps still-linked assertions.
Docs are honest about the two sharp edges (nogrpc removes exported methods; nodgraph makes Migrate fatal on a Dgraph-configured service). Scope is cohesive — all three tags drain the same grpc import tree and the auth split is a prerequisite for nogrpc.
Two nits only, both non-gating — see inline.
Description:
Three more opt-in build tags. A build that sets none of them is unchanged.
gofr_nogrpcgofr_nodgraphgofr_nootlpMeasured on
examples/using-add-rest-handlers, darwin/arm64, go1.26.3, atac84f219c:gofr_nogrpcgofr_nodgraphgofr_nootlpAll six tags: 57.69 → 35.88 MB (−37.8%), 831 → 414 packages (−50.2%).
gofr_nogrpcis worth 0.68 MB andgofr_nodgraph1.12 MB — which looks like nothing. That is the point worth understanding before judging them.google.golang.org/grpcwas pinned by four independent importers:&api.Mutation{}inpkg/gofr/migrationotlptracegrpc(traces/exporters/otlp.go) andotlpmetricgrpc(metrics/exporters)otlpmetrichttp— which imports grpc for its status codes, so the HTTP transport cannot be kept eitherAgainst the
gofr.dev/pkg/gofrpackage itself, the grpc tree goes 86 → 82 withgofr_nogrpc, 82 → 81 addinggofr_nootlp, 81 → 81 addinggofr_nodgraph, and 81 → 0 only oncegofr_nopubsubjoins them —cloud.google.com/gopins grpc for the Pub/Sub client. Only the last tag releases the tree, and it then measures 4.10 MB / 128 packages. Each of the first two is a prerequisite, not a standalone win.Breaking Changes (if applicable):
gofr_nogrpcis the only tag in either PR that changes the exported API — and only when the tag is set.RegisterService,AddGRPCServerOptions,AddGRPCUnaryInterceptorsandAddGRPCServerStreamInterceptorsall name grpc types in their signatures, so they cannot exist without the import and are compiled out.A build that sets the tag and calls them fails to compile. That is the intended outcome — it asked for a binary without gRPC and then used gRPC. The two gRPC example servers are exactly such builds, so their gofr-dependent files carry
//go:build !gofr_nogrpcand are left out under the tag; without it they fail with*gofr.App does not implement grpc.ServiceRegistrar (missing method RegisterService). The protoc.pb.gooutput is untagged, and the gRPC clients need no tag.A default build keeps all four methods and is unchanged.
Under
gofr_nootlpandgofr_nodgraphthe affected features fail loudly rather than silently degrading:"otlp"stays registered in the metrics registry and fails where it would open the connection — rather than vanishing and lettingMETRICS_EXPORTER=otlpquietly resolve to a different exporter than the operator configuredcheckAndCreateMigrationTablereturns an error naming the tag andRuntreats it as fatal — which is what stops a migration being recorded as applied when it was notPrometheus,
gcp,TRACE_EXPORTER=zipkinandTRACE_EXPORTER=gofrare untouched, so a tagged build can still export metrics and traces.Additional Information:
EnableBasicAuthand the other five entry points build gRPC interceptors, which namespkg/gofr/grpc/middleware, which imports grpc — soauth.gocould not stay untagged while doing it. The gRPC half now lives ingrpc_auth_enabled.gobehind two bridges,addGRPCBasicAuthandaddGRPCAPIKeyAuth. Each takes the HTTP middleware's ownBasicAuthProvider/APIKeyAuthProvider, so no grpc type crosses intoauth.go, which keeps the HTTP half in every build.App.grpcServerstays a concrete*grpcServerin every build. Undergofr_nogrpc,grpc_disabled.godeclares its owngrpcServertype with a no-opRun/Shutdown. No interface and no type assertions are needed, andgofr.go,factory.goandrun.goare unchanged fromdevelopment.Two pre-existing defects fixed on the way:
App.grpcServerwithout checking it.factory.goleaves it nil when the server fails to build — an out-of-rangeGRPC_PORTis enough — and logs and continues, so all four panicked in a case the surrounding code treats as recoverable. They log and return now, andTestGRPCSetters_NoServerIsANoOp(pkg/gofr/grpc_test.go:768) pins it: deleting the four guards fails all four subtests with a nil-pointer panic. Behaviour change in the default build:AddGRPC*andRegisterServicelog instead of panicking.TestUnifiedAuthenticationRegistrationcould not fail. It asserted>= 2interceptors after threeEnablecalls; two are registered by default, so it passed on the defaults alone and would not have noticed any auth bridge going missing. It now asserts exactly five.Changes since
58b915763d4cff0628development. It madeEnableBasicAuthandEnableOAuthreturn an error and log throughauthDisabled. Inauth.gothe resolution keeps this PR'saddGRPCBasicAuth/addGRPCOAuthbridges and adds development'sreturn nilandauthDisabled.development's copy ofTestUnifiedAuthenticationRegistrationingofr_test.gois dropped because this PR moved that test togrpc_test.go, which now also takesrequire.NoErroron both calls.8a1a71b8fTest_App_initTracer,Test_initTracer_doesNotLogCredentialsandTest_initTracer_invalidConfigmoved unchanged intootel_otlp_test.gobehind//go:build !gofr_nootlp. They configureTRACE_EXPORTER=otlp/jaeger, which the tag omits, so they failed undergofr_nootlpand under all six tags.f0d9c3b7agofr_nogrpc. 16 files inexamples/grpc/grpc-{unary,streaming}-serverget//go:build !gofr_nogrpc: the gofr-cli wrappers, the server implementations, their tests, andmain/main_test. The tag has to cover the whole chain, because*_gofr.goand*_server.gocall intohealth_gofr.goandmainimports the package. CI: thegofr_nogrpcstep and the all-six step now build./...instead of./pkg/....8201bea6dgrpc_disabled_test.gonow checksEnableBasicAuth's error. This was fallout from thedevelopmentmerge: the file compiles only undergofr_nogrpc, so only the tagged lint step saw the unchecked return (errcheck, Slim Build Tags run36664366656).DO NOT EDIT), and regenerating them drops the tag. Building./...undergofr_nogrpcin CI is what catches that. I checked it by removing the tag fromhealth_gofr.go: the build exits 1.Local verification at
8201bea6ddarwin/arm64, go1.26.3.
slim_build_tagsCI steps, extracted verbatim fromgo.ymland run in orderokgo build ./...: untagged,gofr_nogrpc, all sixgo vet: untagged./...;gofr_nogrpcon./examples/...; all six on./...go test ./pkg/..., untaggedgo test ./pkg/gofr/: untagged,gofr_nootlp, all six8a1a71b8f)gofr_nootlpgo test ./examples/grpc/..., untaggedgolangci-lint --new-from-merge-base origin/development: all six tags on./pkg/...(the CI step), and untagged on./...:2121,:8000):TestInitMetricsServer_DefaultPortandTestGraphQL_*were skipped locally, so their results come from CI.Local verification at
58b915763darwin/arm64, go1.26.3. CI now runs on this PR as well; treat CI as the authority for the full suite, for the reason below.
slim_build_tagsCI steps, extracted verbatim fromgo.ymland run in ordergo vet ./pkg/gofr/: untagged,gofr_nogrpc, all sixgolangci-lint --new-from-rev origin/development ./pkg/gofr/...: untagged and all six-race(35 tests)-tags gofr_nogrpcgo test ./pkg/gofr/..., sub-packagesTestGRPCSetters_NoServerIsANoOpwith the four nil guards deleted-racerun was not clean locally, and the cause is not this PR. An unrelated local process held:8000and:2121, GoFr's default HTTP and metrics ports. Every test that callsNew()without overriding the ports hitFATAL port … is blocked. That was 14 tests, andTestApp_OnStartfails the same way on a cleandevelopmentcheckout. With those skipped, the root package ran under-racewith 0 data races. The skipped tests includeTestGRPC_ServerRun_WithInterceptorAndOptionsandTestApp_WithReflection, so their results come from CI.Checklist:
goimportandgolangci-lint.