Make OTel tracing opt-in instead of always-on - #13
Merged
Conversation
InitTracing defaulted OTEL_EXPORTER_OTLP_ENDPOINT to "localhost:4318", so unsetting the variable did not disable tracing -- it silently retargeted the exporter at localhost and the batch processor logged export failures on a loop. There was no way to turn tracing off from configuration at all, which became a problem when we retired our collector: the service kept retrying into the void and filling logs with connection errors. Gate initialization on two conditions, matching how our Node services (openmeet-api, bsky-event-processor, bsky-firehose-consumer) already work, so the whole fleet has one consistent switch: 1. ENABLE_TRACING must be exactly "true" 2. OTEL_EXPORTER_OTLP_ENDPOINT must be set (no default) Either gate failing returns a no-op shutdown and leaves the global TracerProvider untouched. Callers always defer the returned function, so it is never nil. Tests: the previous suite used bare os.Setenv/os.Unsetenv, which leaked state between tests -- TestInitTracing_DefaultValues unset both variables and never restored them, so the two tests after it ran against whatever was left over. Switched to t.Setenv for automatic per-test restore, and added coverage for both disabled paths, for non-"true" values, and for the http:// scheme stripping that the deployment config actually exercises.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
InitTracingdefaultedOTEL_EXPORTER_OTLP_ENDPOINTtolocalhost:4318:So unsetting the variable did not disable tracing — it silently retargeted the exporter at localhost, and the batch processor logged export failures on a loop. Combined with
InitTracingbeing called unconditionally from bothcmd/apiandcmd/consumer, there was no way to turn tracing off from configuration at all.That became a real problem when we retired our collector: the service kept retrying into the void and filling logs with connection errors.
Change
Gate initialization on two conditions, matching how our Node services already behave so the whole fleet has one consistent switch:
ENABLE_TRACINGmust be exactly"true"OTEL_EXPORTER_OTLP_ENDPOINTmust be set (no default)Either gate failing returns a no-op shutdown and leaves the global
TracerProvideruntouched. Callers always defer the returned function, so it is never nil.Tests
The previous suite used bare
os.Setenv/os.Unsetenv, which leaked state between tests —TestInitTracing_DefaultValuesunset both variables and never restored them, so the two tests after it ran against whatever was left over. That is also why gating the behaviour brokeTestInitTracing_CreatesSpans: it had been relying on ambient env from earlier tests.Switched to
t.Setenvfor automatic per-test restore, and added coverage for:ENABLE_TRACING, no endpoint)"true"values (false,1,yes,TRUE) — only exact"true"enableshttp://scheme stripping that the deployment config actually exercisesgo vetclean; full suite passes (templ generatefirst —*_templ.gois gitignored, so a fresh clone needs it beforego build).