fix(tracing): derive OTLP transport security from the TRACER_URL scheme

[akshat-kumar-singhal]

Stacked PR — 1 of 3. This is the base of a three-PR stack for IAM-authenticated trace export to Google Cloud (#4204):

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

This PR stands alone and depends on nothing else — it is a self-contained bug fix and can be reviewed and merged on its own. PRs 2 and 3 are branched on top of it and will be rebased onto development after each parent merges, so their diffs shrink as the stack lands. Please review and merge in order 1 → 2 → 3.

Description:

buildOtlpExporter passed TRACER_URL straight to otlptracegrpc.WithEndpoint, which stores it verbatim as the gRPC target, and appended an unconditional otlptracegrpc.WithInsecure().

A scheme-bearing TRACER_URL is not a valid gRPC target, so the dialer never opened a socket and no span was ever exported:

rpc error: code = Unavailable desc = invalid target address https://127.0.0.1:4317,
error info: address https://127.0.0.1:4317:443: too many colons in address

So http://… and https://… endpoints were unusable, not downgraded — the feature was broken for anyone who wrote a scheme. Only a schemeless host:port worked, and only over plaintext, because WithInsecure() was unconditional.

Transport security now comes from the URL scheme when the endpoint has one (WithEndpointURL alone, never combined with WithInsecure — the SDK applies options in slice order, so a trailing WithInsecure() would override the scheme), and from a new TRACER_INSECURE config only for a schemeless host:port endpoint.

TRACER_INSECURE defaults to true, which deliberately diverges from METRICS_INSECURE (defaults to false). Every existing TRACER_URL=host:4317 deployment today points at a plaintext collector, because plaintext was the only transport the exporter could produce. Defaulting those to TLS would break them silently in a minor release. The divergence is commented at both call sites.

Precedence is the TRACER_URL scheme, then TRACER_INSECURE, then the OTel standard OTEL_EXPORTER_OTLP_INSECURE / OTEL_EXPORTER_OTLP_TRACES_INSECURE. The SDK applies its environment config before any explicit option, so whatever GoFr passes wins over them. This PR documents that precedence rather than changing it.

Also added:

• A warning when TRACER_INSECURE is set alongside a scheme-bearing URL (it is ignored).
• A warning when TRACER_HEADERS / TRACER_AUTH_KEY credentials would ride a plaintext connection — mirroring what the metrics exporter already does.

Breaking Changes (if applicable):

None. An https:// TRACER_URL now actually works and uses TLS, which is the documented and intended behavior; schemeless endpoints keep their existing plaintext transport by default.

Additional Information:

• No new dependencies.
• Test_buildOtlpExporter_wireTransport asserts the first bytes the exporter puts on the socket for all four transport decisions — h2c preface (PRI * HTTP/2.0) for schemeless and http://, TLS ClientHello (0x16) for https:// and for schemeless with TRACER_INSECURE=false. Restoring the pre-fix option line fails three of its four subtests, because the scheme-bearing endpoints never open a connection at all.
• Docs updated, including three places that still told users TRACER_URL must be schemeless: docs/guides/production-tracing/page.md, docs/guides/twelve-factor-config/page.md, docs/quick-start/observability/page.md (the Grafana Cloud example tripped this PR's own plaintext-credentials warning), docs/references/configs/page.md.

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]

Thanks for this — the fix itself is correct, and I verified it end to end rather than by reading. Recording what I checked so the next reviewer doesn't repeat it:

Verified against the pinned SDK (otlptracegrpc v1.44.0), not docs:

Claim	Evidence at the pin
WithEndpointURL derives security from the scheme	internal/otlpconfig/options.go:289 → cfg.Traces.Insecure = u.Scheme != "https"
A later WithInsecure() overrides it	options.go:328 sets Insecure = true; opts applied in slice order
SDK applies env first, options after	NewGRPCConfig — ApplyGRPCEnvConfigs(cfg) at options.go:128, opts loop at 129–131
Non-insecure default is system root CAs	credentials.NewTLS(nil) — matches what the docs now say

Verified on the wire and end to end (real plaintext OTLP gRPC TraceServiceServer, spans emitted through App.initTracer()):

TRACER_URL	First bytes on the socket	Spans reached the plaintext collector
schemeless host:port	PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n (h2c)	yes
http://host:port	PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n (h2c)	yes
https://host:port	16 03 01 05 c7 01 (TLS ClientHello)	no — correct
schemeless + TRACER_INSECURE=false	TLS	no — correct

All four transport decisions are right, the deprecated TRACER_HOST/TRACER_PORT path stays plaintext, and TRACE_EXPORTER=zipkin is untouched. Perf is a non-issue: resolveOtlpTransport is 4.2–6.9 ns/op with 0 allocs and runs once per process; binary grows +224 B. Shape matches the merged metrics exporter (pkg/gofr/metrics/exporters/otlp.go:57,100-112), and traces actually improves on it by warning about an invalid flag where metrics swallows it. Observability of a misconfiguration is good too — a TLS mismatch surfaces on stderr in 15 s with the exact cause (tls: first record does not look like a TLS handshake).

I also agree with the TRACER_INSECURE=true default diverging from METRICS_INSECURE=false. Defaulting existing host:4317 deployments to TLS would break them silently in a minor release. The reasoning is sound and it is commented at both call sites.

So the code is good. What I'm asking to change is the description, plus one test gap.

---

1. The "verified on the wire" claim does not reproduce

"the new code sends a TLS ClientHello for an https:// endpoint, while the old code sends the h2c connection preface for the same config"

I ran exactly that probe with the pre-fix wiring restored (opts := []otlptracegrpc.Option{WithInsecure(), WithEndpoint(url)}):

PRE-FIX  https://127.0.0.1:PORT  ->  WIRE: NOTHING - no connection was ever made
PRE-FIX  http://127.0.0.1:PORT   ->  WIRE: NOTHING - no connection was ever made
PRE-FIX  127.0.0.1:PORT          ->  "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"

The old code emitted zero bytes. WithEndpoint("https://host:4317") is an invalid gRPC target:

rpc error: code = Unavailable desc = invalid target address https://127.0.0.1:65382,
error info: address https://127.0.0.1:65382:443: too many colons in address

The dialer never opened a socket, so it cannot have sent an h2c preface. Could you re-run that probe and correct the line?

2. Consequence: the headline framing is wrong

"An https:// TRACER_URL was silently exported over plaintext"

Per the above, pre-fix an https:// URL produced an invalid target — traces were never exported at all, not leaked in the clear. That is still a real bug and this is still the right fix, but it is "the feature was completely broken", not "a TLS downgrade". Worth getting right before it reaches the changelog, since it changes the severity a reader will assign.

3. The OTEL_EXPORTER_OTLP_INSECURE half of the bug is still present

"OTEL_EXPORTER_OTLP_INSECURE=false could not override it either"

Measured on this branch — OTEL_EXPORTER_OTLP_INSECURE=false, schemeless TRACER_URL, TRACER_INSECURE unset:

=> err=<nil>   // spans reached a PLAINTEXT collector

The default insecure=true still appends WithInsecure(), which lands after ApplyGRPCEnvConfigs, so the OTel-standard variable is still silently overridden. Either is fine by me:

• (a) skip WithInsecure() when OTEL_EXPORTER_OTLP_INSECURE / OTEL_EXPORTER_OTLP_TRACES_INSECURE is set and TRACER_INSECURE is not; or
• (b) drop the claim and document the precedence (TRACER_INSECURE > OTEL_EXPORTER_OTLP_*).

4. otlptracehttp is not in this repo

"buildOtlpExporter appended otlptracegrpc.WithInsecure() / otlptracehttp.WithInsecure() unconditionally"

$ grep -rn 'otlptracehttp' --include='*.go' --include='go.mod' .
(no matches)

pkg/gofr/otel.go:10 imports only otlptracegrpc. Please drop the reference.

5. The fix is not guarded by a test

Every new function is at 100% line coverage, and the original bug can still be reintroduced with CI fully green. I replaced the new opts block with the exact pre-fix line and ran the package:

ok  gofr.dev/pkg/gofr  5.713s

The whole suite passes. Test_resolveOtlpTransport exercises a pure function that is not connected to the wiring, and Test_buildOtlpExporter only asserts err == nil && exp != nil, which holds for any option set. Swapping the two branches would also go unnoticed.

A ~40-line test does catch it: stand up a real plaintext TraceServiceServer on 127.0.0.1:0 and assert ExportSpans succeeds for http:// and schemeless, and fails for https://. Under the mutant the http:// case fails with too many colons; on this branch it passes and https:// fails with tls: first record does not look like a TLS handshake. Happy to hand over the file if it saves you time — keep the ExportSpans deadline at ~1 s so the TLS case doesn't sit on the default timeout.

---

Non-blocking

Docs that now contradict this change (none are in the diff):

File	Line	Problem
docs/quick-start/observability/page.md	481-484	TRACER_URL=otlp-gateway-prod-us-east-0.grafana.net:443 with Authorization=Basic .... Schemeless, so it defaults to plaintext — this PR's own new warning fires on our own documented example, and I confirmed it does: traces are exported to ... over plaintext with headers configured. Needs https://.
docs/guides/production-tracing/page.md	15	The {% howto %} step still says "Set TRACER_URL to a bare host:port (no http:// scheme)" — same file as the new TLS section you added.
docs/guides/twelve-factor-config/page.md	89-91	"TRACER_URL must be a bare host:port — no http:// scheme" — no longer true.

Nits:

• strings.HasPrefix(url, "http://") is case-sensitive. HTTP://host:4317 is a legal URL (RFC 3986 §3.1) and falls through to WithEndpoint("HTTP://...") → invalid target. url.Parse inside WithEndpointURL is case-insensitive, so lowercasing before the prefix check closes the gap in one line.
• a.tracerInsecure() is evaluated in getExporter for every exporter, so a malformed TRACER_INSECURE warns under TRACE_EXPORTER=zipkin/gofr where it means nothing — while a valid one there is ignored with no warning at all. Inverse of the otlp path.
• Test_buildOtlpExporter_warnsOnPlaintextCredentials passes both url="collector:4317" and port="9411"; the port is dead since url wins.
• CONTRIBUTING asks for American English — "behaviour" in the Breaking Changes section.
• Please strip the Co-Authored-By: Claude ... trailer and the Claude-Session: line from ce80050a7, and the 🤖 Generated with footer plus session link from the PR description. Same note as #4208 — and since ce80050a7 is the base commit of the stack, it propagates into #4206 and #4207.

Adjacent, pre-existing, not yours: pkg/gofr/otel.go:81 registers sdktrace.NewBatchSpanProcessor(exporter) even when getExporter returned an error and exporter is nil. Worth a separate issue.

Stack note: I checked what #4206 does to this code — it moves otlpTransport, resolveOtlpTransport and all five new tests verbatim into pkg/gofr/traces/exporters/, and all seven sub-cases survive, so no coverage is lost. That means the structural shape here is transient and I have not commented on it. Two things do survive the move and are worth a thought there rather than here: the otlpTransport.insecure field is derivable (insecure == !useEndpointURL && plaintext in all four reachable states), and the added block runs at 0.64 comments per line of code against a 0.20 baseline for pkg/gofr.

[akshat-kumar-singhal]

Thank you — this is an unusually thorough review, and the wire probe caught a real error in my description. I re-ran your probe before changing anything and confirm all five points.

1 & 2. The probe and the framing — you are right, corrected

Reproduced with the pre-fix wiring restored (WithInsecure() + WithEndpoint(url)):

PRE-FIX  https://127.0.0.1:52881  ->  WIRE: NOTHING (no connection)
         err: invalid target address https://127.0.0.1:52881,
              error info: address https://127.0.0.1:52881:443: too many colons in address
PRE-FIX  http://127.0.0.1:52882   ->  WIRE: NOTHING (no connection)   (same error)
PRE-FIX  127.0.0.1:52893          ->  "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"

WithEndpoint stores the value verbatim (otlpconfig/options.go:270-275), so a scheme-bearing TRACER_URL was never a valid gRPC target and the dialer never opened a socket. My earlier claim that the old code sent an h2c preface for that config was wrong — it sent nothing.

The description and the commit message now say what actually happened: an http:///https:// endpoint was unusable, not downgraded. Only a schemeless host:port ever worked, and only over plaintext. Agreed that this changes the severity a reader assigns, and it is better to have it right before the changelog.

3. OTEL_EXPORTER_OTLP_INSECURE — taking option (b)

Confirmed on this branch: the default insecure=true still appends WithInsecure() after ApplyGRPCEnvConfigs, so the OTel variable is still overridden. I have dropped the claim and documented the precedence instead:

TRACER_URL scheme → TRACER_INSECURE → OTEL_EXPORTER_OTLP_INSECURE / OTEL_EXPORTER_OTLP_TRACES_INSECURE

It is in the resolveOtlpTransport doc comment, in docs/references/configs/page.md, and in the TLS section of docs/guides/production-tracing/page.md. I preferred this over (a) because GoFr's own config keys are the documented surface everywhere else in the framework, and silently deferring to an SDK variable only when a GoFr key is unset is a precedence rule users would have to learn from the source.

4. otlptracehttp — dropped

Confirmed, no matches anywhere in the repo. The reference is gone from the description.

5. Test gap — added, and mutation-checked

Test_buildOtlpExporter_wireTransport stands up a throwaway listener and asserts the first bytes the exporter puts on the socket, across all four transport decisions:

config	asserted
schemeless	PRI * HTTP/2.0
http://	PRI * HTTP/2.0
https://	0x16 (TLS ClientHello)
schemeless + TRACER_INSECURE=false	0x16

I went with a raw listener rather than a real TraceServiceServer so the test adds no dependency — go.opentelemetry.io/proto/otlp is currently indirect, and promoting it to direct in the root go.mod for one test seemed the wrong trade. The assertion is on the wire either way.

Your mutant is caught. Restoring the exact pre-fix line:

--- FAIL: Test_buildOtlpExporter_wireTransport (8.01s)
    --- FAIL: .../http_scheme_speaks_h2c (3.00s)
    --- FAIL: .../https_scheme_speaks_TLS (3.00s)
    --- FAIL: .../schemeless_with_TRACER_INSECURE=false_speaks_TLS (1.00s)

Three of four subtests fail — swapping the two branches is caught as well, not just deleting the block.

Docs

All three fixed in this PR:

• docs/quick-start/observability/page.md — the Grafana Cloud example is now https://otlp-gateway-prod-us-east-0.grafana.net:443. Good catch that our own documented example tripped this PR's new warning.
• docs/guides/production-tracing/page.md:15 — the {% howto %} step no longer says "bare host:port (no http:// scheme)".
• docs/guides/twelve-factor-config/page.md:89-91 — same, and it now points at TRACER_INSECURE for upgrading a bare host:port.

Nits

• Case-sensitivity — fixed; the prefix check lowercases first, with a table case for HTTPS://collector:4317. url.Parse inside WithEndpointURL lowercases the scheme, so this is now consistent end to end.
• a.tracerInsecure() evaluated for every exporter — fixed; it is resolved inside the otlp/jaeger case, so a malformed value no longer warns under zipkin/gofr. One thing to flag honestly: in feat(tracing): add a trace exporter registry, resource and shutdown flush #4206 this field moves into exporters.Config and is populated for every exporter again, because the config is built before the exporter is selected. The right home for that warning there is the OTLP builder that actually reads the flag, and I would rather make that change under feat(tracing): add a trace exporter registry, resource and shutdown flush #4206's own review than reshape it here.
• Dead port argument in Test_buildOtlpExporter_warnsOnPlaintextCredentials — removed.
• "behaviour" — now "behavior".
• Trailers — removed from the commit and from all four PR descriptions. As you noted, the base commit changed, so feat(tracing): add a trace exporter registry, resource and shutdown flush #4206 and feat(tracing): keyless trace export to Google Cloud (TRACE_EXPORTER=gcp) #4207 have been rebased onto it and force-pushed.

Adjacent

Agreed that pkg/gofr/otel.go:81 registering a BatchSpanProcessor over a nil exporter is a real and separate bug, and it is pre-existing and out of scope here. Two details I checked while filing it:

• It does not panic — batch_span_processor.go:153 guards bsp.e == nil, so the batcher is installed and silently drops every span. The default: branch of getExporter reaches the same state with a nil error, so an unsupported TRACE_EXPORTER also leaves a provider that looks configured and exports nothing.
• feat(tracing): add a trace exporter registry, resource and shutdown flush #4206 already fixes it: exporters.Build returns the NeverSample provider when the builder yields no exporter (provider.go:52-55), so the degrade path is explicit there. I have filed the issue anyway since it is live on development until that merges.

Stack note

Thank you for checking the move in #4206 — that saved a round trip. Both observations you parked there are fair: otlpTransport.insecure is derivable from the other two fields, and the comment density on the new block is high. I will take both up on #4206 rather than churn this one.

All three branches build, test and lint clean at the rebased commits.

[aryanmehrotra]

All five addressed, and I re-verified each rather than taking the reply on trust. Reviewed at df89d8725.

#	Ask	Verified
1+2	Wire claim / framing corrected	✅ description now says "unusable, not downgraded"
3	OTEL_EXPORTER_OTLP_INSECURE	✅ claim dropped, precedence documented in otel.go and docs/references/configs/page.md
4	otlptracehttp	✅ 0 matches in the description
5	Test guards the fix	✅ Test_buildOtlpExporter_wireTransport — and I ran your mutant

Mutation result — restoring the exact pre-fix line opts := []otlptracegrpc.Option{WithInsecure(), WithEndpoint(url)}:

--- FAIL: Test_buildOtlpExporter_wireTransport/http_scheme_speaks_h2c
--- FAIL: Test_buildOtlpExporter_wireTransport/https_scheme_speaks_TLS
--- FAIL: Test_buildOtlpExporter_wireTransport/schemeless_with_TRACER_INSECURE=false_speaks_TLS
FAIL	gofr.dev/pkg/gofr	8.866s

Three of four subtests fail; clean run is ok 5.072s. The gap is closed, and asserting on first bytes rather than standing up a TraceServiceServer was the right call — it keeps go.opentelemetry.io/proto/otlp indirect in the root go.mod. I used a real collector in my probe and would have had to promote that dependency; yours is the better trade.

Nits: case-insensitive prefix ✅ (with an HTTPS:// table case), tracerInsecure() moved into the otlp/jaeger arm ✅, dead port argument removed ✅, "behavior" ✅, trailers gone from the commit and all four descriptions ✅. Docs: all three corrected, including the Grafana Cloud example that tripped this PR's own warning.

On the nil BatchSpanProcessor — you are right and I was wrong. I probed for a panic and could not produce one but did not find the reason; batch_span_processor.go:153-155 guards bsp.e == nil and returns early, exactly as you say. Your added detail is the important part: the default: branch reaches that state with a nil error, so an unsupported TRACE_EXPORTER leaves a provider that looks configured and silently drops every span. Thanks for filing it.

Noted on tracerInsecure moving back into exporters.Config under #4206 — agreed that belongs in #4206's review, and I will pick it up there.

LGTM.

[Umang01-hash]

Verified against head df89d87. Correct, backward-compatible fix. Confirmed the bug at the wire: reverting to the old WithInsecure()+WithEndpoint(url) line makes scheme-bearing TRACER_URLs never open a connection and downgrades a schemeless TRACER_INSECURE=false to h2c — Test_buildOtlpExporter_wireTransport catches all of it (asserts real h2c preface vs TLS ClientHello). Checked the SDK source directly: WithEndpointURL sets Insecure = scheme != "https", and NewGRPCConfig applies env before opts, so the precedence comment holds. Default path (host:4317, no TRACER_INSECURE) stays byte-identical plaintext — no regression. No exported-API break; docs + config reference accurate; gofmt/vet/lint/build clean, otel suite green under -race.

Two optional nits, noted here rather than inline (both on unchanged lines):

• TRACER_INSECURE defaults to plaintext while METRICS_INSECURE defaults to TLS — intentional and documented for backward compat, just flagging the two-knobs-opposite-defaults for possible future convergence.
• resolveOtlpTransport detects the scheme on the untrimmed url (tracerInsecure trims its value, url isn't), so a stray-whitespace TRACER_URL would miss the scheme. Pre-existing and negligible.