feat(tracing): add a trace exporter registry, resource and shutdown flush

[akshat-kumar-singhal]

Stacked PR — 2 of 3. Part of a three-PR stack for IAM-authenticated trace export to Google Cloud (#4204):

#	PR	contents
1	#4205	OTLP transport security from the TRACER_URL scheme + TRACER_INSECURE — ✅ merged
2	this PR	trace exporter registry, resource, shutdown flush (closes #3771)
3	#4207	pkg/gofr/traces/exporters/gcp + examples/using-gcp-traces

PR 1 has merged and this branch is now rebased on development, so the diff is this PR's changes only. Please review and merge 2 → 3.

Description:

Traces resolved their exporter through a hardcoded switch in otel.go, so a vendor exporter had nowhere to live that did not pull that vendor's SDK into the core module. This ports the metrics package's extension point to traces, and fixes the missing trace shutdown along the way.

New package pkg/gofr/traces/exporters:

• A builder registry with Register / RegisterResourceDetector (both Experimental), plus knownExternalExporters so a missing blank import produces an actionable error rather than a bare "unsupported exporter".
• Built-in otlp / jaeger / zipkin builders self-register via init(). otel.go keeps only propagator install, config validation and header parsing.
• Build assembles the TracerProvider and degrades to the NeverSample provider on any failure rather than crashing app start. NeverSample and not a noop provider: spans must keep valid IDs, or X-Correlation-ID goes to all-zeroes.

The gofr exporter stays in pkg/gofr and registers itself from there — gofr.NewExporter is exported API, and traces/exporters imports nothing from pkg/gofr, so there is no import cycle.

The trace resource now merges resource.WithFromEnv() and framework_version, so OTEL_RESOURCE_ATTRIBUTES is no longer ignored, plus the registered resource detector for the selected exporter only. No WithHostID: no trace backend documents a need for it and it costs ~10 ms on darwin at every start.

service.name is the one key the environment cannot set: GoFr keeps taking it from APP_NAME, and OTEL_SERVICE_NAME (or a service.name= entry inside OTEL_RESOURCE_ATTRIBUTES) is discarded with a warning naming the dropped value. That is deliberate rather than an oversight — the already-merged metrics/exporters resource resolves service.name from APP_NAME the same way, so letting only traces follow OTEL_SERVICE_NAME would report one service name to the trace backend and another to the metric backend, breaking the join between a service's traces and its metrics. The option order in buildResource is what decides this, so it is now documented as load-bearing and pinned by Test_buildResource — reordering the []resource.Option slice fails two subtests — and the exception is documented in the tracing guide. Moving both packages to honor OTEL_SERVICE_NAME is a reasonable future call and belongs in the same follow-up that owes metrics/exporters the nil-Logger guard: both signals moving together, or neither.

Closes #3771 — the TracerProvider was never shut down, so the final span batch was dropped at exit. App.Shutdown now flushes traces before closing the container, and the CMD path flushes under the same bounded timeout as metrics (metricsFlushTimeout renamed telemetryFlushTimeout).

Breaking Changes (if applicable):

None to the public API, and no config key changes shape or meaning. Three runtime behaviours do change, all found in review by @aryanmehrotra — an earlier revision of this section wrongly claimed there were none, and a later one listed only the two that sit on error paths:

1. An unparseable TRACER_RATIO now samples everything instead of nothing. strconv.ParseFloat failing used to leave traceRatio at its zero value, so TRACER_RATIO=10% produced TraceIDRatioBased(0) and exported no spans at all; it now falls back to 1, matching the documented default of the config. A service carrying such a typo goes from exporting nothing to exporting every trace, which on a per-span-billed backend is a real cost jump with no config change by the operator. The trade is deliberate: silently disabling tracing on a typo is the worse failure, because it is invisible. The error log now names both the rejected value and the ratio actually applied, Test_App_tracerRatio pins the choice, and both the tracing guide and the config reference document it.

2. An unrecognised TRACE_EXPORTER now stops recording. Previously a nil exporter still got a full TracerProvider; batch_span_processor.go returns early on a nil exporter, so the app looked configured and dropped every span. Build now returns the NeverSample provider, which is the honest state — the app still starts, trace/span IDs stay valid, X-Correlation-ID and the trace_id log field keep working. Documented in the tracing guide.

3. App.Run now blocks until the graceful shutdown has finished — up to SHUTDOWN_GRACE_PERIOD + 1s after a termination signal. This is what makes Buffered trace spans dropped on shutdown: BatchSpanProcessor never shut down #3771 actually fixed rather than nominally: without it the handler starts the drain, Run returns, the process exits, and the final batch is dropped anyway, which is the ticket's own symptom. Unlike the two above it is not on an error path — every user's main() reaches it on every signal. It interacts with Kubernetes terminationGracePeriodSeconds, which must stay comfortably above SHUTDOWN_GRACE_PERIOD, and with any test harness that calls Run and previously saw it return immediately. The +1s is a bounded safety net against a shutdown step that ignores its context, not a second deadline competing with the first; shutdownWaitMargin now carries the reasoning for the value and for leaving it non-configurable. Documented in the graceful-shutdown guide.

The registry API itself is marked Experimental.

Additional Information:

• No new dependencies — the OTel packages involved were already required by the core module.
• The registry is the seam PR 3 plugs into: a vendor exporter registers from its own module, keeping its SDK out of gofr.dev's go.mod.
• New tests cover the registry, the OTLP and Zipkin builders, provider assembly and the degradation path, alongside the updated otel_test.go.
• One ShutdownFunc field rather than Buffered trace spans dropped on shutdown: BatchSpanProcessor never shut down #3771's telemetryShutdown []func slice. The slice would re-plumb the metrics flush that has already merged, giving a traces-only PR a metrics blast radius for no gain here, so container.ShutdownMetrics stays a separate field with its own call site. If a third telemetry signal lands, collapsing both into the slice is the moment for it.
• Config.AppVersion is gone. It was populated with version.Framework — the framework's version under a name promising the app's — and nothing in traces/exporters read it. The metrics twin reads its own as the instrumentation version, but traces has no equivalent consumer: GoFr's tracers are already named gofr-<framework version> and the resource carries framework_version separately. Removing it while the API is Experimental is free, and it can return populated from GetAppVersion() the day something reads it.
• Concurrent Register/Build is now exercised. The registry maps were guarded but never tested under -race; Test_registry_concurrentRegisterAndBuild fails with a concurrent map read/write if either lock is dropped.
• Build now substitutes noopLogger for a nil Logger. config.go documented that fallback for "callers that do not supply one" and nothing ever applied it, so this exported entry point panicked in buildResource on Build(ctx, cfg, nil); the new test fails with a nil-pointer panic without the guard. The identical defect in pkg/gofr/metrics/exporters is inherited rather than introduced here, and is left for a follow-up that fixes both packages together.

Checklist:

• I have formatted my code using goimport and golangci-lint.
• All new code is covered by unit tests.
• This PR does not decrease the overall code coverage.
• I have reviewed the code comments and documentation for clarity.

[aryanmehrotra]

Reviewed at c09b88c49. The design is sound and I verified the claims rather than reading them — but differential testing turned up two behaviour changes that the Breaking Changes section says do not exist, and one defect inherited from the metrics package.

Verified

Claim	Result
No new dependencies	✅ main go.mod/go.sum untouched by the whole stack
No import cycle	✅ go list -deps ./pkg/gofr/traces/exporters never reaches gofr.dev/pkg/gofr
No WithHostID	✅ appears only in the comment explaining its absence
Registry API Experimental	✅ registry.go:24, :70
metricsFlushTimeout → telemetryFlushTimeout	✅ run.go:18

#3771 proven both ways. On development, tp is a local at otel.go:68 and never stored; Shutdown (gofr.go:102-123) closes http/grpc/metrics/mcp and never traces; run.go flushes metrics only. On this branch: emit a span, assert nothing has left (BSP holds for 5s), call shutdownTraces — 1 span delivered to a real collector. The fix works.

Also clean: 50 concurrent Register ‖ lookup under -race — no race. Double shutdown() is safe. Shutdown on the degraded path returns a non-nil func and nil.

1. TRACER_RATIO parse failure flips sampling from 0% to 100%

I ran all 15 TRACE_EXPORTER / config permutations against development and this branch. 13 are byte-identical. Two are not. This is the one that matters:

TRACER_RATIO=abc	development otel.go:63-66	here otel.go:90-99
on ParseFloat error	logs, traceRatio stays 0	logs, return 1
sampler	TraceIDRatioBased(0)	TraceIDRatioBased(1)
measured	recording=false	recording=true

A service running TRACER_RATIO=10% today exports nothing; after this it exports everything. On a per-span-billed backend that is an unbounded volume and cost jump, with no config change by the operator and nothing in the logs beyond the same error line they are already ignoring.

I think return 1 is the better default — silently disabling tracing on a typo is worse than over-sampling, and it matches GetOrDefault("TRACER_RATIO", "1"). The ask is not to revert it. The ask is that "their config keys and their behaviour are unchanged" stops being true while this ships: name it in Breaking Changes, add a line to the tracing docs, and add a test case pinning the chosen behaviour so the next refactor does not flip it back.

2. An unsupported TRACE_EXPORTER now stops recording

TRACE_EXPORTER=carrier-pigeon	development	here
span.IsRecording()	true	false

This one is a fix, and your own note on #4205 explains why better than I could: batch_span_processor.go:153-155 returns early on a nil exporter, so development installs a provider that looks configured and silently drops every span. Build returning the NeverSample provider is the honest state. Your test already asserts it. It just is not a "no behaviour change" — same ask as above, one line in Breaking Changes.

Worth saying: in every one of the 15 configs, on both branches, TraceID and SpanID stayed valid. The correlation-ID invariant holds on the degrade path.

3. Build panics on a nil Logger, and noopLogger never runs

Build(ctx, cfg, nil) -> panic: nil pointer dereference
  otlp.go:102 -> provider.go:111 -> provider.go:52

Build is exported, Experimental, and the package ships the fix already:

config.go:69 — "noopLogger satisfies Logger for callers that do not supply one."

Nothing ever substitutes it. In metrics/exporters it is real — exporter.go:18 passes noopLogger{}. Here it is reachable only from registry_test.go:67, so unused does not flag it and the comment promises something the code does not do.

This is inherited, not introduced — I confirmed metrics/exporters' Build(ctx, cfg, nil) panics identically, so this mirrors a merged defect rather than creating one. It also propagates into #4207's buildExporter. One line at the top of each Build closes all three:

if logger == nil {
    logger = noopLogger{}
}

Your call whether to fix it here or in a follow-up across all three packages — I would take the follow-up, but the comment should not claim the fallback exists until it does.

Adjacent, not yours

TestContext_WriteMessageToSocket hits a data race 3/3 on clean development — websocket.go:47 written by serveWithGoroutine (handler.go:183). go.yml has no -race flag anywhere, so CI cannot see it. Separate issue; flagging because it makes go test ./pkg/gofr/ -race red on this branch through no fault of the change.

Nothing blocking in the code itself. Items 1 and 2 are description-and-docs; item 3 is a one-liner or a follow-up.

[akshat-kumar-singhal]

Thanks — the differential testing across all 15 permutations is more than I gave you to work with, and you were right on all three. Pushed as 2e94347b (also rebased onto development now that #4205 has merged, so the diff is this PR alone).

1. TRACER_RATIO parse failure. Taken as written: the return 1 stays, the "behaviour is unchanged" claim does not. Breaking Changes now names it as behaviour change 1, with the cost framing you gave — a service on TRACER_RATIO=10% goes from exporting nothing to exporting everything, and that is a real bill with no config change by the operator. Three more things beyond the description:

• The error log now names both halves, so the volume jump is attributable rather than just logged: invalid TRACER_RATIO "10%": ...; falling back to 1 (sampling every trace). The old line was a.container.Error(err) — the bare strconv error, which is exactly the line an operator has already learned to scroll past.
• Test_App_tracerRatio (otel_test.go) pins it, including the two cases that matter: "10%" → 1 and "abc" → 1, named so a future refactor has to delete an assertion that says not 0 rather than quietly flip it.
• tracerRatio carries the reasoning as a comment, so the next reader sees why 1 and not 0: over-sampling is visible and costs money, under-sampling is invisible.

Also documented in docs/references/configs and docs/guides/distributed-tracing.

2. Unsupported TRACE_EXPORTER. Same treatment — behaviour change 2 in Breaking Changes, and the tracing guide now says plainly that an unrecognised value disables tracing and that trace/span IDs stay valid so correlation keeps working. Your framing of it ("the honest state") is the one I used.

3. Nil Logger. Fixed here rather than deferred, because the comment promising the fallback is in this PR's own package. Build now starts with if logger == nil { logger = noopLogger{} }, and Test_Build_substitutesNoopLoggerForANilLogger covers all three paths. I checked it is not a vacuous test: with the guard removed it fails with the nil-pointer panic in buildResource; with it, it passes. The Builder doc now states the contract explicitly — "the logger passed to a Builder is never nil" — so #4207's buildExporter and any third-party builder can log unconditionally.

pkg/gofr/metrics/exporters I have left alone, as you suggested: it is a merged package and the fix belongs in its own PR rather than riding along in a tracing change.

On the websocket race — thank you for flagging it, and agreed it is not this PR's. I reproduced it on clean development and confirmed go.yml has no -race anywhere, which is why nothing has caught it. I would rather not open that as a drive-by from a tracing PR; happy to file it (and separately, a CI issue for the missing -race) if you would like it from me rather than raised internally.

[aryanmehrotra]

All three addressed, and I re-verified each by mutation rather than reading the diff. Reviewed end to end at 7b5f1642c, now that the rebase onto development has reduced this to its own changes.

#	Item	Verified
1	TRACER_RATIO behaviour change undeclared	✅ named in Breaking Changes with the cost framing; forced return 0 → 3 subtests fail, including unparsable_value_falls_back_to_1,_not_0
2	Unsupported exporter change undeclared	✅ Breaking change 2 + tracing guide
3	Build panics on a nil Logger	✅ guard added; removing it → FAIL plus the nil-pointer panic in buildResource

Taking metrics/exporters out of this PR was the right call — that one is merged code and deserves its own change.

Differential testing, re-run against the new development

19 TRACE_EXPORTER/config permutations, both branches:

identical	16 of 19
differing	exactly the two now declared
TraceID/SpanID valid	19 of 19 on both sides — the correlation-ID invariant holds on every degrade path

On TRACER_RATIO — I am satisfied with return 1, and here is the argument I did not make last time

An unset TRACER_RATIO already resolves to 1 via GetOrDefault, so GoFr exports every trace by default today. return 1 makes a malformed value behave the same as an absent one, which is the least surprising rule available. The old 0 was never a decision — traceRatio simply kept its zero value after an unhandled ParseFloat error, and moving the logic into a helper is what forced someone to actually choose. Only malformed values change; 0, 0.1 and 1 are all untouched.

The mitigations make it defensible: the error names both the rejected value and the ratio applied, the docs carry it in two places, and the test is phrased so a future refactor has to delete an assertion rather than quietly flip a constant.

The second change is not really a choice

Preserving the old behaviour would mean deliberately handing a nil exporter to WithBatcher, which is the bug: batch_span_processor.go:153 returns early on a nil exporter, so development installs a provider that reports IsRecording() == true and discards every span. Returning the NeverSample provider is the honest state, and nobody was getting traces from a misspelled exporter before either.

#3771 verified again on this head

emit span → assert 0 delivered (the BSP holds for 5s) → shutdownTraces()
  => spans flushed at shutdown: 1

Against development: tp is a local at otel.go:68 that is never stored, Shutdown closes http/grpc/metrics/mcp and never traces, and run.go flushes metrics only. The gap is real and this closes it.

Gates, all run here

Gate	Result
gofmt, go build ./..., go vet ./pkg/gofr/...	clean
go test ./pkg/gofr/ ./pkg/gofr/traces/...	ok / ok
golangci-lint run ./pkg/gofr/traces/...	0 issues
-race -count=2	0 races
coverage, new package	94.8%
CI	14 pass, 0 fail

Cost, measured at this head

go.mod / go.sum / go.work	not one byte changed; dependency count 104 → 104
binary (examples/using-add-rest-handlers)	61,118,658 → 61,119,138 = +480 bytes (0.0008%)
startup, tracing enabled	+1,150 B, +19 allocs, once per process
startup, no exporter (the default)	+216 B, +4 allocs
per-request path	untouched — every exporters.* call site is init() or initTracer

Edge cases I probed

TRACE_EXPORTER=" OTLP " resolves; Build twice on the same Config yields a stable resource; an empty exporter returns a non-nil provider and ShutdownFunc; double shutdown returns nil twice (sync.Once); 50 concurrent Register/lookup under -race is clean.

Two small things, neither blocking:

• Build(ctx, nil, logger) panics on a nil Config. Inherited rather than introduced — metrics/exporters/provider.go:43 dereferences cfg the same way — and unreachable from the container, which always passes a value. One line next to the logger guard would close it whenever you next touch the file.
• Register silently overrides an existing name. Documented, and harmless today; worth a thought if a second submodule ever claims a name another already holds.

One thing worth checking separately, not a change request here

This closes the tracer half of #3771 — the TracerProvider is now shut down and the batch flushed. In my review of #3925 I measured a second half on the SIGTERM path: startAllServers returns, Run returns and main exits before the shutdown goroutine finishes, so on Kubernetes the flush can be cut off even when it is wired correctly. That is run.go's join, which this PR does not add and #3925 does. Worth confirming which of the two ends up owning it before #3771 is closed — I will raise it there rather than hold this up.

The registry itself is well shaped: the resource is resolved before any builder runs so a builder sees what its backend will receive, knownExternalExporters turns a missing blank import into an actionable message, the degrade path keeps correlation IDs valid, and traces/exporters imports nothing from pkg/gofr so the gofr exporter can register from there without a cycle.

LGTM.

[Umang01-hash]

Verified independently at 7b5f164 (not trusting the diff or prior approval).

• No breaking API change: every removed otel.go symbol is unexported, NewExporter intact, new package all-new + Experimental. go.mod/go.sum/go.work untouched.
• No import cycle (go list -deps never reaches pkg/gofr).
• #3771 fix wired: shutdownTraces in App.Shutdown (before container close) + runCMD under telemetryFlushTimeout; double-call safe via sync.Once.
• Correlation-ID invariant is fail-on-revert: mutating neverSampleProvider -> a real noop fails Test_App_initTracer on all 3 degrade subtests. TRACER_RATIO return-1 similarly pinned (return 0 -> test fails).
• E2E on examples/http-server with no exporter: valid, unique, non-zero X-Correlation-Id per request.
• gofmt/build/vet clean, traces package 94.8% cover, docs cover both declared behaviour changes.

Nits only, both already noted and deferred: Build(ctx, nil, logger) panics on a nil Config (inherited, unreachable from container); Register silently overrides a name. Neither blocks.

LGTM.

[aryanmehrotra]

Following up on my earlier approval. Sorry to come back after signing off: I ran the branch end to end locally before merge, and found one thing I missed the first time. Everything else held up, so the good news first.

What I checked, and it works

I tested fe96de3cf against a real Jaeger (OTLP gRPC + Zipkin), with the same app built against development (23852af71) for comparison:

Scenario	Result
otlp, jaeger alias, zipkin, http:// scheme TRACER_URL	✅ spans delivered
OTEL_RESOURCE_ATTRIBUTES=deployment.environment=...	✅ reaches Jaeger (ignored on development)
framework_version resource attribute	✅
TRACER_RATIO=10%	✅ logs falling back to 1 (sampling every trace)
TRACE_EXPORTER=otpl / no exporter / unreachable collector	✅ app serves, X-Correlation-ID stays non-zero
TRACE_EXPORTER=gcp without the import	✅ the blank-import hint is really useful
CMD app: final span on exit	✅ delivered (0 on development)
build, vet, traces/exporters with -race ×3, pkg/gofr/..., lint	✅

The flush itself is correct. If the process stays alive for 1.5s after Run() returns (less than the 5s batch timeout), spans arrive 5/5 on this branch and 0/5 on development. That's #3771 fixed at the provider level.

The HTTP SIGTERM path doesn't get to run the flush

For a typical main that just calls app.Run(), the process exits before App.Shutdown reaches the new flush:

SIGTERM
  shutdown goroutine: httpServer.Shutdown ─► … ─► shutdownTraces (network) ─► container.Close()
                              │
  main: servers stop ─► wg.Wait() returns ─► Run() returns ─► main returns ─► process exits

Run() returns as soon as the servers stop and never waits for the goroutine started in startShutdownHandler. That race already exists on development, it isn't introduced here. But this PR puts a network round trip ahead of container.Close(), so shutdown now loses the race almost every time.

Setup: start the app, send one GET, send SIGTERM immediately. 10 trials per build:

	this PR	development
Application shutdown complete logged	0/10	9/10
final spans delivered	0/10	0/10

So on the signal path #3771 isn't fixed yet for HTTP apps. As a side effect, container.ShutdownMetrics and container.Close() (datasource cleanup) are now skipped where they used to run.

Repro

func main() {
	app := gofr.New()
	app.GET("/hello", func(c *gofr.Context) (any, error) {
		c.Trace("child").End()
		return "hi", nil
	})
	app.Run()
}

TRACE_EXPORTER=otlp TRACER_URL=127.0.0.1:4317 APP_NAME=repro ./app &
curl -s localhost:8000/hello; kill -TERM $!
# log ends at "Shutting down server with a timeout of 30s"; no "Application shutdown complete",
# and Jaeger shows no trace for service "repro"

Suggestion

Would you be open to having Run() wait for the shutdown handler to finish? For example, the goroutine closes a done channel, and Run waits on it after startAllServers returns, bounded by the shutdown timeout. It's a small change and it's what makes the flush in this PR reachable. Doing it here seems better than a follow-up, because merging without it trades the dropped-spans bug for skipped datasource cleanup on SIGTERM.

One thing to keep in mind once Run waits: with the collector unreachable, shutdown took 10.0s on this branch (the OTLP exporter's export timeout) versus ~1ms on development. That's still inside the default 30s grace period, but a short SHUTDOWN_GRACE_PERIOD would cut the flush off. Probably worth a line in the tracing guide.

Happy to pair on the Run change or push a commit to the branch if that helps. Thanks again for this PR, the registry and the resource handling are a nice improvement.

[aryanmehrotra]

Marking this as changes requested so it doesn't get merged by accident before the SIGTERM shutdown path is sorted. It isn't a judgement on the rest of the PR, which I'm still happy with. Details and the repro are in my comment above. Once Run() waits for the shutdown handler, I'll re-run the same local check and re-approve.

[akshat-kumar-singhal]

Thanks — you were right, and it's a better find than the three before it: the flush this PR adds was unreachable on the path almost everyone actually takes. Done as you suggested, in this PR rather than a follow-up, at a69a57f3 (two commits on top of fe96de3c, merged with development after — no rebase, so your earlier review anchors still resolve).

The change

startShutdownHandler now returns a channel it closes on exit, and Run waits on it after startAllServers returns. Two constraints that aren't obvious from the suggestion:

• The wait has to be conditional. If the servers stopped for any reason other than a signal — a bind failure, say — the handler goroutine is still parked on <-ctx.Done(), and that context is canceled only by Run's own defer stop(). Waiting unconditionally deadlocks Run on exactly the paths that are already going wrong. So it waits only when ctx.Err() != nil.
• And bounded, at SHUTDOWN_GRACE_PERIOD + 1s. App.Shutdown's steps take the shutdown context, but container.Close() reaches driver code that need not honour it; without a bound, one stuck datasource turns "process exits at the grace period" into "process hangs until SIGKILL". On the timeout it logs graceful shutdown did not finish within …, exiting anyway and returns.

Your repro, 10 trials each

Built against this branch and against fe96de3c (this PR without the new commit), both in a container on the same network as a real Jaeger, OTLP gRPC:

	fe96de3c	a69a57f3
Application shutdown complete logged	0/10	10/10
final span in Jaeger	0/10	10/10

So the pre-fix column reproduces your 0/10 exactly, and #3771 is now fixed on the signal path too, not just for CMD apps. Script and full logs are reproducible from the repro in your review — the only change I made to it was setting APP_NAME per trial so each run is its own Jaeger service.

The unreachable-collector cost you flagged

Measured on this branch, TRACER_URL pointed at an unroutable address:

SHUTDOWN_GRACE_PERIOD	shutdown took	outcome
30s (default)	10.3s, 10.4s	completes; the OTLP exporter's own 10s export timeout, as you measured
3s	3.01s, 3.06s	flush cut off at the deadline, still logs Application shutdown complete

Run's own bound never fired in either case — the shutdown context does the work, which is what it should be. The 3s row is the case worth documenting, so the tracing guide's gotchas now carry it: the flush shares the drain's deadline, an unreachable collector spends 10s inside it, keep the grace period above that.

On the test

It runs the app in a helper process rather than in the test binary. A real SIGTERM is delivered process-wide, so sending it from a test also tears down every app the other tests in pkg/gofr leave running — my first attempt did exactly that and panicked a gomock controller whose test had already finished. (It also has to use os.Executable(), not os.Args[0]: cmd_test.go overwrites os.Args and doesn't always put it back.) The test fails on fe96de3c with the marker saying Run returned first, and there's a table-driven test for the three awaitShutdown branches, including the never-signalled one that would otherwise hang.

go build ./..., go test ./pkg/gofr/..., the shutdown tests under -race, and lint are clean locally; CI is green on a69a57f3.

Would you re-run your local check when you get a chance? And the two offers from earlier still stand — filing the websocket-race and missing--race-in-CI issues, and running examples/using-gcp-traces against real Cloud Run before #4207.

[aryanmehrotra]

Thanks for turning this around so quickly, and for the extra care on the conditional wait and the bound. Both are subtle, and they hold up. I re-ran everything locally against a real Jaeger, comparing a69a57f3 with fe96de3c.

The shutdown fix: confirmed

Case	fe96de3c	a69a57f3
SIGTERM right after a request (10 trials): Application shutdown complete	0/10	✅ 10/10
SIGTERM (10 trials): final spans in Jaeger	0/10	✅ 10/10
otlp / jaeger / zipkin / http:// scheme / OTEL_RESOURCE_ATTRIBUTES / TRACER_RATIO=10%, no linger after Run	0 spans	✅ 4 spans each
Collector unreachable, default grace period	—	✅ 10.19s, completes
Collector unreachable, SHUTDOWN_GRACE_PERIOD=3s	—	✅ 3.04s, completes
A pub/sub Close() that ignores its deadline (sleeps 30s), grace period 2s	—	✅ Run gives up at 3.26s with graceful shutdown did not finish within 3s
App.Shutdown from a handler, no signal	no hang	✅ no hang, Run returns at once
HTTP port already bound	exit 1	✅ exit 1, unchanged
CMD app flush	✅	✅

go vet, the run/shutdown tests with -race ×5 (including TestApp_awaitShutdown and TestApp_Run_waitsForShutdown), traces/exporters with -race ×3, go test ./pkg/gofr/... and lint are all clean. Watching the deadline-ignoring Close get cut off at grace + 1s was the part I most wanted to see live, and it behaves exactly as described.

One thing left before I can re-approve: the conflict with #4247

development gained #4247 (5d1811fe6) after your merge, so the branch now conflicts in pkg/gofr/otel.go. It's more than a mechanical conflict. #4247 redacts tracer config in logs, and the log lines it touched are the ones this PR moved into traces/exporters/. Resolving the conflict on the otel.go side alone would leave the new files unredacted, and GitHub wouldn't show it.

Checked live with TRACER_URL=http://tokenuser:<token>@127.0.0.1:4317:

	the token appears in logs
development (with #4247)	0 times
this branch	2 times: Exporting traces to otlp at http://tokenuser:<token>@…

TRACE_EXPORTER is echoed verbatim here too (unsupported TRACE_EXPORTER: <value>), which #4247 also redacts.

Could the merge bring redactURL / redactExporterName / redactUnparsedURL and #4247's tests into traces/exporters, and route these through them?

• otlp.go: lines 75, 112, 116
• zipkin.go: line 37
• provider.go: lines 108, 111, 119

Once that's in, I'll re-run the same checks (credentials in TRACER_URL must show 0, plus the flush and SIGTERM cases above) and re-approve. It's a quick one on my side.

A small side note, not for this PR: while building the deadline-ignoring client, a struct-valued pubsub.Client passed to AddPubSub panics in container.isNil during shutdown (reflect.Value.IsNil on struct Value). That's the existing bug #4164 addresses. It's on development already, not something this PR introduced.

[akshat-kumar-singhal]

Thanks for catching this, and for framing it the way you did — "GitHub wouldn't show it" is exactly the shape of the problem. development is merged (twice — #4249 landed midway, e399ebe1 is the head) and the redaction is carried into traces/exporters.

What moved

redactURL, redactExporterName, redactUnparsedURL, escapeControlChars and your tests now live in pkg/gofr/traces/exporters/redact.go — with the lines they protect rather than in the package those lines left. Two are exported, RedactURL and RedactMessage: Builder is a public extension point, and a third-party exporter logs its endpoint for the same reason the built-ins do. redactExporterName stays unexported; nothing outside the registry echoes a configured name.

Every line you listed is routed:

otlp.go:75 TRACER_INSECURE is ignored for TRACER_URL=…	RedactURL
otlp.go:112 plaintext-with-headers warning	RedactURL
otlp.go:116 Exporting traces to <name> at <url>	RedactURL, and the name is the matched constant
zipkin.go:37 Exporting traces to zipkin at …	RedactURL
provider.go:108, 119	the normalized, registry-matched name
provider.go:111 unsupported TRACE_EXPORTER	redactExporterName

On the name: rather than lowercasing cfg.Exporter at the log line, the OTLP builder is registered as a closure over the constant it was registered under (Register(exporterOTLP, otlpBuilder(exporterOTLP))). TRACE_EXPORTER=" JaEgEr " now logs jaeger, not the padded string — same principle as your buildOtlpFamilyExporter, moved into the registration.

Your tracerInsecure change came along too: the raw value is not echoed.

Two more places it leaked

Running your check against the whole process rather than against startup turned up two paths the log-line redaction doesn't cover:

1. Builder errors. zipkin.New reports an unparsable endpoint as invalid collector URL "<the raw URL>": parse "<the raw URL>": …, and spanExporter logs that error. Redacting the log line above it is not enough when the constructor hands the endpoint back inside the error.
2. Runtime export errors. otelErrorHandler logs what the SDK raises, and a failed export reads request to <TRACER_URL> failed: … — on every batch a collector rejects, long after startup. This one is on development today, not something the registry introduced; my zipkin run hit it. Happy to pull it out into its own PR against development if you'd rather have it separate from this one — say the word and I'll revert it here.

RedactMessage(msg, endpoint) handles both. It replaces the %q-escaped spelling as well as the raw one, which matters more than it looks: url.Error renders with %q, and %q escapes exactly the characters (control, quote, backslash) that put a value on redactURL's unparsed path in the first place — so matching the raw string alone would miss the very case that produced the error.

Measured

Docker, against Jaeger, one run per exporter with the credential in TRACER_URL and TRACER_AUTH_KEY, counting occurrences of the token in the app's own log — before = a69a57f3, after = e399ebe1:

case	before	after
otlp, userinfo	2	✅ 0 — Exporting traces to otlp at http://REDACTED@jaeger:4317
jaeger, userinfo	1	✅ 0
TRACER_INSECURE set and ignored	3	✅ 0
invalid TRACER_INSECURE	1	✅ 0
zipkin, ?api-key=	2	✅ 0 (1 of the 2 was the runtime export error above)
gofr, userinfo	1	✅ 0
secret pasted into TRACE_EXPORTER	1	✅ 0 — unsupported TRACE_EXPORTER: REDACTED

And the shutdown behavior you verified, re-run on the merge result: shutdown-complete 10/10, spans-delivered 10/10.

go build ./..., go vet, golangci-lint (0 issues on traces/exporters), go test ./pkg/gofr/... and -race -count=3 on traces/... are clean.

Tests

Your Test_redactURL and Test_redactExporterName travel with the helpers, unchanged. Two wiring tests are new, because the function-level ones are precisely the ones that keep passing through a move like this:

• Test_initTracer_doesNotLogCredentials (pkg/gofr) — your end-to-end table, adapted, driving the real initTracer for otlp / jaeger / zipkin / gofr / a secret in TRACE_EXPORTER.
• Test_builders_doNotLogCredentials (traces/exporters) — every registered builder, asserting the secret is absent from both the log and the returned error.

Neither asserts a helper's return value, so neither can follow a helper into another package and stay green while the wiring is gone.

One message wording differs from development: the unsupported-exporter line is unsupported TRACE_EXPORTER: <name>; tracing is disabled rather than expected one of otlp, jaeger, zipkin or gofr, since the registry accepts names it can't enumerate up front (gcp, and anything registered). gofr_test.go is updated accordingly. Say if you'd rather it listed the currently-registered names.

#4249 is in as well: the functions it renames mostly moved into traces/exporters, where they already took an endpoint; the rename is applied to the two left in pkg/gofr (isValidConfig's parameter, buildGoFrExporter's local). The otlp/jaeger/zipkin name constants it adds to constants.go stay dropped here — the registry matches those names, so pkg/gofr no longer switches on them.

CI is green on e399ebe1 — all 16 checks, including typos and Code Quality.

[aryanmehrotra]

Reviewed by planning #3771 and part 2 of #4204 independently first, freezing that plan, then reading the diff and comparing the two blind. Base origin/development @ 4f3e02dfc, branch @ e399ebe1e (rebased and current).

The port itself is right. Builder / Register / RegisterResourceDetector / Config / Logger / ShutdownFunc / Build mirror metrics/exporters symbol-for-symbol, both Experimental markers are carried, the NeverSample scar comment survives the move, and #3771 is fixed properly rather than nominally. go test -race -cover ./pkg/gofr/traces/... → ok, 95.2%; go vet clean; ./pkg/gofr ok.

Three things to settle before merge, then two notes.

---

1 — OTEL_SERVICE_NAME is silently discarded, and the test picks the one key that hides it

provider.go:148-155 applies resource.WithFromEnv() before resource.WithAttributes(service.name=cfg.AppName). resource.New merges each later option as the winner (otel/sdk v1.46.0 resource/auto.go:75, resource/resource.go:210-212), so GoFr's app name wins.

Reproduced against the real buildResource on this branch:

env set	resulting service.name
OTEL_SERVICE_NAME=from-env	app-name-from-gofr	discarded, no warning
OTEL_RESOURCE_ATTRIBUTES=service.name=from-env	app-name-from-gofr	discarded, no warning
OTEL_RESOURCE_ATTRIBUTES=deployment.environment=prod	attribute present	works

Why this is blocking rather than a nit. The PR description's headline is "OTEL_RESOURCE_ATTRIBUTES is no longer ignored", and for the single key an operator is most likely to set, it still is. Three things line up badly:

• Test_buildResource (provider_test.go:93-94) sets only deployment.environment, so the suite passes without ever touching the colliding key.
• The docs diff never mentions OTEL_RESOURCE_ATTRIBUTES at all, so there is nowhere an operator could learn the exception.
• Nothing pins the option order, so a later reshuffle of the []resource.Option slice flips the behaviour in either direction with nothing failing — on a resource path whose output is debugged from the backend, not from logs.

Cheapest fix: pick a precedence and write it down — either apply the service.name attribute only when env did not supply one, or keep GoFr's name and Warnf when both are set. Then add a service.name case to Test_buildResource and one line to the tracing guide.

Note this is inherited, not introduced: metrics/exporters/provider.go:81-107 has the same order and its own test picks location — the same blind spot. Probably worth the same treatment as the nil-Logger deferral, i.e. one follow-up fixing both packages.

2 — App.Run now blocks for up to SHUTDOWN_TIMEOUT + 1s, and it is not in Breaking Changes

run.go:68-71 + 109-129 add awaitShutdown, so Run no longer returns while the graceful shutdown is in flight.

The change is correct and it is what makes #3771 actually fixed — without it the handler starts the drain, Run returns, the process exits, and the batch is dropped anyway, which is the ticket's own symptom. My independent plan missed this entirely; registering a ShutdownFunc alone would not have fixed the bug.

But the Breaking Changes section lists two behaviour changes, both on error paths. This is a third and it is not on an error path: every user's main() can now take up to SHUTDOWN_TIMEOUT + 1s to return after a signal, which interacts with Kubernetes terminationGracePeriodSeconds and with any test harness that calls Run. Shipped unannounced, the first report of it arrives as a hang rather than as a known change.

Cheapest fix: one bullet in Breaking Changes and the release note. Also worth a sentence on why shutdownWaitMargin is 1 * time.Second — it currently has no cited basis, and it is the value that decides when the safety net fires.

3 — Config.AppVersion is written, never read, and carries the wrong value

otel.go:61 sets AppVersion: version.Framework — the framework version. The metrics equivalent passes the app's own (container/container.go:140, c.GetAppVersion()). Nothing in pkg/gofr/traces/exporters reads the field.

framework_version is already carried separately at provider.go:145, so the field is redundant as well as mislabelled. The first vendor exporter to trust it gets GoFr's version under a name that promises the app's.

Cheapest fix: drop the field, or populate it from GetAppVersion(). It is on an Experimental API, so now is the free moment.

---

Notes, not blocking

• One ShutdownFunc field vs #3771's telemetryShutdown []func slice. You diverged from the issue's own "proposal B" and container.ShutdownMetrics stays a separate field with its own call site. I think that is defensible — the slice version would re-plumb the already-merged metrics flush, giving a traces-only PR a metrics blast radius — but the divergence has no stated reason, and by this repo's own standard a divergence you can justify in one sentence is a decision while one you cannot is a second pattern nobody chose. One sentence in the PR body would close it.
• Concurrent Register/Build. -race passes, but nothing exercises concurrent access to the package-level registry. Low blast radius (the sanctioned path is init() before main, and the mutex is inherited), but the API is public.

Things I checked and am deliberately not flagging

Several of these are better than what I planned independently — expand

• NeverSample over a noop provider — correct, and verified at the pin: otel/sdk v1.46.0 trace/tracer.go:97-107 generates IDs before ShouldSample runs, and trace/span.go:857 returns that SpanContext verbatim, so X-Correlation-ID survives. A noop provider would zero it, which is the scar at the old otel.go:50-56.
• redactEndpointInError / RedactMessage on otelErrorHandler — no precedent, new, and it closes a pre-existing leak: the SDK quotes TRACER_URL verbatim in runtime export errors and the old handler logged them raw. My plan only asked that start-time redaction survive the move. This is better than what I specified.
• Moving the missing-endpoint decision into the builder (ErrMissingEndpoint). My plan kept isValidConfig intact, which would have left the registry unreachable for #4207's URL-less ADC exporter — the old isValidConfig hard-rejects any TRACE_EXPORTER with no TRACER_URL except gofr. This relaxation is load-bearing for the stack and I missed it.
• Deprecated TRACER_HOST/TRACER_PORT. I suspected the isValidConfig relaxation broke the warning. Verified live — still warns (TRACER_HOST and TRACER_PORT are deprecated, use TRACER_URL instead) and still resolves localhost:4317. Not a regression.
• Experimental markers — I initially filed these as an unasked assumption; they are precedent (metrics/exporters/registry.go:15-18, :61-62). Keeping them is right.
• sync.Once in shutdownFunc — stronger than relying on the SDK's own idempotence, and provider_test.go:75-80 asserts the double call.
• Explicit nil-exporter check before WithBatcher — right call, and not merely tidiness: NewBatchSpanProcessor(nil) is documented as a no-op but still allocates its queue and starts a background goroutine (trace/batch_span_processor.go:111-130).
• Omitting resource.WithHostID() — justified. Measured on an M-series Mac: it forks /usr/sbin/ioreg and costs 20–30 ms, so "~10 ms" is conservative, not optimistic.
• Zipkin deprecation warning — preserved at traces/exporters/zipkin.go:31.
• Not porting the metrics-only prometheus_target location/instance labels (#4113) — correct. Those belong to Managed Prometheus; Cloud Trace has no such monitored resource, and porting them would produce warnings an operator cannot act on.
• The TRACER_RATIO and unknown-exporter behaviour changes — both documented in the tracing guide and the config reference, both tested, and both the right trade. The new SHUTDOWN_GRACE_PERIOD warning names a real config key (checked).
• CMD-path a.container.Logger deref outside the nil guard — pre-existing (base run.go:35), preserved verbatim. Not this PR's.
• Two-case unregistered-name error — known-external gets the exact blank import, anything else gets plain unsupported with the name redacted. Mirrors metrics/exporters/provider.go:156-161 and diverges in the safer direction.

---

Evidence: go vet clean · go test -race -cover ./pkg/gofr/traces/... ok 95.2% · go test -count=1 ./pkg/gofr/ ok · branch merges cleanly on development.

Merge this before #4207, which is stacked on it.

[akshat-kumar-singhal]

Thanks — planning #3771 and #4204 independently first and then reading the diff blind is a lot of work to do on someone else's PR, and it found the one thing three rounds of review had walked past. All three are fixed at ea43ad40.

1 — OTEL_SERVICE_NAME discarded

Confirmed against the real buildResource rather than from the citation: with OTEL_SERVICE_NAME=from-env set, the resolved resource carries service.name=from-gofr. Your reading of the merge order is right, and so is the sharper point behind it — Test_buildResource passed because deployment.environment is the one key that collides with nothing GoFr supplies. The test was not weak, it was aimed away from the target.

I took your second option — keep APP_NAME, warn — and not because it was cheaper. The first option has a problem neither of us named: metrics/exporters/provider.go resolves service.name from APP_NAME the same way and has already shipped. Letting only traces follow OTEL_SERVICE_NAME would report one service.name to the trace backend and a different one to the metric backend, breaking the join between a service's traces and its metrics — exactly where an operator needs it. A traces-only fix to a consistency bug is a worse consistency bug.

So the behaviour is unchanged and the three things that made it invisible are gone:

• warnIfEnvServiceNameIgnored names the dropped value. It reads through resource.Environment() rather than os.Getenv, so the warning reflects what WithFromEnv actually resolved — including the service.name= form embedded in OTEL_RESOURCE_ATTRIBUTES, which os.Getenv("OTEL_SERVICE_NAME") would have missed.

• Three cases in Test_buildResource: OTEL_SERVICE_NAME loses, service.name= inside OTEL_RESOURCE_ATTRIBUTES loses while deployment.environment beside it still wins, and no warning fires when the environment agrees with APP_NAME. Mutation-checked — swapping the two resource.Option entries fails two of them:

  --- FAIL: Test_buildResource/OTEL_SERVICE_NAME_loses_to_APP_NAME_and_is_reported
      resource attribute "service.name" = "from-env", want "app"
  --- FAIL: Test_buildResource/service.name_in_OTEL_RESOURCE_ATTRIBUTES_loses,_its_siblings_do_not
      resource attribute "service.name" = "from-env", want "app"
  

• The tracing guide gains a "Resource attributes from the environment" section stating the exception, the log line an operator will see, and the cross-signal reason — so there is somewhere to learn it.

The comment above the []resource.Option slice now says the order is load-bearing and why, rather than leaving it to look incidental.

On the follow-up: agreed, and I'd frame it as a question rather than a fix. Moving both packages to honor OTEL_SERVICE_NAME is defensible — it is the OTel-standard override, and "GoFr ignores the standard variable" is a fair complaint. What is not defensible is the two signals disagreeing. So it belongs in the same PR that owes metrics/exporters the nil-Logger guard: both packages, both signals moving together or neither.

2 — App.Run blocking

Fair, and the framing is the useful part: shipped unannounced, the first report arrives as a hang. It is now Breaking Changes item 3, called out as the one that is not on an error path, with the terminationGracePeriodSeconds interaction and the test-harness case named.

shutdownWaitMargin now carries its basis. Short version: the constant only ever decides how long a process hangs after it has already missed its deadline, and both error directions are bounded by that — too small reports an about-to-finish shutdown as failed, too large parks a doomed pod until Kubernetes SIGKILLs it. A second covers the scheduling and log-flush tail that is the only work legitimately past the deadline. Left non-configurable deliberately: a knob there would be a second shutdown deadline to reason about, and SHUTDOWN_GRACE_PERIOD is the one that should move.

3 — Config.AppVersion

Dropped rather than repopulated. I went looking for the consumer that would justify keeping it and there isn't one: the metrics twin genuinely reads its own (mp.Meter(cfg.AppName, WithInstrumentationVersion(...))), but traces has no equivalent — GoFr's tracers are already named gofr-<framework version> (context.go:65, cron.go:169) and the resource carries framework_version separately, which is your point about redundancy. A field that is written, never read, and mislabelled is worse than an absent one, and Experimental makes now the free moment. It can come back populated from GetAppVersion() the day something reads it. #4207 follows.

The two notes

• ShutdownFunc vs the telemetryShutdown []func slice — you are right that an unstated divergence is a second pattern nobody chose. Reason is now in the PR body: the slice would re-plumb the already-merged metrics flush, giving a traces-only PR a metrics blast radius for no gain here, so container.ShutdownMetrics stays a separate field. If a third telemetry signal lands, collapsing both into the slice is the moment for it.
• Concurrent Register/Build — closed rather than deferred, since it was cheap. Test_registry_concurrentRegisterAndBuild runs Register, RegisterResourceDetector and Build against the same names from 24 goroutines; it fails with a concurrent map read/write if either lock is dropped. You are right that the sanctioned path is init() before main, but nothing in the API confines a caller to it.

---

Evidence: go vet clean · go test -race -cover ./pkg/gofr/traces/... ok, 97.2% (was 95.2%) · go test -count=1 ./pkg/gofr/ ok · golangci-lint run ./pkg/gofr/traces/... 0 issues · upstream/development re-fetched at 4f3e02df, unchanged, branch still merges cleanly.

[aryanmehrotra]

Approving at ea43ad40e. All three are resolved, and I re-checked each one against the branch rather than against the description.

1 — service.name. Verified the fix the way the original finding was found: I swapped the two resource.Option entries and re-ran your suite.

--- FAIL: Test_buildResource/OTEL_SERVICE_NAME_loses_to_APP_NAME_and_is_reported
--- FAIL: Test_buildResource/service.name_in_OTEL_RESOURCE_ATTRIBUTES_loses,_its_siblings_do_not

Exactly the two you named, and nothing else. The order is genuinely pinned now — which was the part of the finding I cared about most, since the behaviour itself was defensible and only the silence wasn't.

Your reason for choosing the warning over honouring OTEL_SERVICE_NAME is better than either option I offered, and I want to record that rather than let it pass as agreement. I framed this as a traces bug. It is a cross-signal consistency property: metrics/exporters already resolves service.name from APP_NAME and has shipped, so traces alone following OTEL_SERVICE_NAME would report one service name to the trace backend and another to the metric backend, breaking the join precisely where an operator is looking when something is wrong. "A traces-only fix to a consistency bug is a worse consistency bug" is the right way round, and re-scoping the follow-up to move both packages together or neither — alongside the nil-Logger guard metrics/exporters is already owed — is the correct shape for it.

Reading through resource.Environment() rather than os.Getenv is also the right call for a reason worth stating: it makes the warning fire for the service.name= form embedded in OTEL_RESOURCE_ATTRIBUTES, which is the spelling an operator setting several attributes at once would actually use, and the one a naive os.Getenv("OTEL_SERVICE_NAME") would have missed.

2 — App.Run blocking. Breaking Changes item 3 says the thing that needed saying: not on an error path, every main() reaches it on every signal, and it interacts with terminationGracePeriodSeconds. shutdownWaitMargin now carries its basis, and the argument holds — the constant only decides how long a process hangs after it has already missed its deadline, so both error directions are bounded by that framing. Agreed on leaving it non-configurable; a second shutdown deadline is worse than a fixed tail.

3 — Config.AppVersion. Dropping it rather than repopulating is the better of the two, and for a reason I had not checked: the metrics twin genuinely reads its own via WithInstrumentationVersion, while GoFr's tracers are already named gofr-<framework version> and the resource carries framework_version separately. There is no consumer to serve. Easy to add back populated correctly the day one exists.

Both notes closed. The ShutdownFunc-vs-slice reasoning is now in the PR body, which is all it needed. And Test_registry_concurrentRegisterAndBuild does what it claims — I deleted the write lock in Register and it reports WARNING: DATA RACE three times over. Thanks for closing that rather than deferring it; you are right that nothing in the API confines a caller to init().

---

One correction, non-blocking. The coverage figure in your comment does not reproduce for me:

go test -count=1 -race -cover ./pkg/gofr/traces/...  ->  95.4%
go test -count=1       -cover ./pkg/gofr/traces/...  ->  95.4%
go test -count=1 -cover -coverpkg=./pkg/gofr/traces/... ./pkg/gofr/traces/...  ->  95.4%

95.2% → 95.4%, not 97.2%. The direction is right and the new tests are real, so this changes nothing about the review — but the figure is offered as evidence, and an evidence line that does not reproduce costs more later than the 1.8 points are worth. Worth correcting in the comment so the next reader is not chasing the gap.

---

Everything else I checked on the previous round still holds at this head: go vet clean, go test -count=1 ./pkg/gofr/ ok, CI green (14 success, 2 skipped), and the branch still merges cleanly on development at 4f3e02df.

Good to merge from my side. #4207 next — it needs the rebase, and it is still carrying the TRACER_URL scheme handling and the IAM role question.