otel: replace cobraotel with native lifecycle management by Jdepp007004 · Pull Request #3108 · authzed/spicedb

Jdepp007004 · 2026-05-09T14:47:26Z

commited and pushed the vendor directory by mistake so another commit to fix it

Fixes authzed#712 and authzed#3095. - Remove dependency on github.com/jzelinskie/cobrautil/v2/cobraotel - Replicate OTel provider initialization natively in pkg/cmd/server/otel.go - Wire TracerProvider into serve.go signal handler so Shutdown and ForceFlush are called on SIGTERM/SIGINT, preventing span loss on exit - Fix vendored cobrautil Viper global singleton bug: viper.SetEnvPrefix was mutating global state instead of the local instance (v.SetEnvPrefix) - Touch pkg/cmd/util/util.go only to break import cycle between pkg/cmd/util and pkg/cmd/server; all flag registrations unchanged - Add 20 tests across unit, integration, and system build tags

tstirrat15

See comments

tstirrat15 · 2026-05-13T18:34:18Z

+)
+
+// otelProviderContextKey is the unexported context key for the TracerProvider.
+type otelProviderContextKey struct{}


Use ctxkey generally

Done, switched to the ctxkey package to match how the rest of the codebase handles context keys.

tstirrat15 · 2026-05-13T18:34:49Z

+	// Legacy flags
+	f.String("otel-jaeger-endpoint", "", "OpenTelemetry collector endpoint - the endpoint can also be set by using enviroment variables")
+	_ = f.MarkHidden("otel-jaeger-endpoint")
+	f.String("otel-jaeger-service-name", "spicedb", "service name for trace data")
+	_ = f.MarkHidden("otel-jaeger-service-name")


Dropped them from both RegisterOTelFlags and RegisterCommonFlags in util.go

tstirrat15 · 2026-05-13T18:41:10Z

+	shutCtx, shutCancel := context.WithTimeout(ctx, OTelShutdownTimeout)
+	defer shutCancel()
+	return provider.Shutdown(shutCtx)


Why a separate context?

there was no real reason for two separate contexts there, it was an oversight on my part ,
fixed to use one shared context for the whole operation

tstirrat15 · 2026-05-13T18:41:39Z

+	if parent == nil {
+		parent = context.Background()
+	}


Same q here

removed these too

tstirrat15 · 2026-05-13T19:22:22Z

+// it as the global OTel provider, and returns it for lifecycle management.
+//
+// Returns (nil, nil) when otel-provider is "none". Callers must handle nil.
+func InitOTelProvider(cmd *cobra.Command) (otelShutdowner, error) {


cobraotel tied the provider to the lifecycle of the command because that was what made sense in that package, but I don't think we need or want it in this context.

The general flow for setting up components of SpiceDB is:

Take cobra flags that are defined in the command and put their values into a config struct

Take that config struct and construct the required object

Pass that object down and in as necessary

The telemetry prometheus registry is probably a decent analogue.

Restructured to follow the pattern you described. Flag values now get copied into an OTelConfig struct, InitOTelProvider takes that struct along with a context and has no dependency on Cobra anymore. The provider is constructed inside Complete and registered with closeables so shutdown is handled automatically as part of the normal server shutdown sequence.

tstirrat15 · 2026-05-13T19:23:29Z

 func RegisterCommonFlags(cmd *cobra.Command) {
-	otel := cobraotel.New("spicedb")
-	otel.RegisterFlags(cmd.Flags())
+	f := cmd.Flags()


I like this

Thanks, glad it reads better there

tstirrat15 · 2026-05-13T19:24:13Z

+			defer func() {
+				// Shutdown OTel provider to ensure all traces are flushed
+				if provider := server.OTelProviderFromContext(cmd.Context()); provider != nil {
+					if err := server.ShutdownOTelProvider(context.Background(), provider); err != nil {
+						log.Warn().Err(err).Msg("failed to cleanly shutdown OpenTelemetry provider")
+					}


Yeah, this makes sense, but I'd rather that we configure the otel provider here as well, not in the RunE

why are we doing the flushing here and not putting the shutdown function of the provider inside of the closeables struct that the server already has?

spicedb/pkg/cmd/server/server.go

Lines 177 to 178 in ee7c9a7

func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {

closeables := util.CloseableStack{}

You're right, the defer approach was the wrong place for it. I wasn't fully across the closeables pattern when I first wrote that. Moved into Complete now so OTel shuts down in the same ordered sequence as everything else rather than racing against it.

tstirrat15 · 2026-05-13T19:25:18Z

+	cmd.SetContext(context.Background())
+	require.NoError(t, cmd.Flags().Set("otel-provider", "otlpgrpc"))
+	require.NoError(t, cmd.Flags().Set("otel-endpoint", "localhost:4317"))
+	require.NoError(t, cmd.Flags().Set("otel-insecure", "true"))


I generally like the tests. Can we add a test or two that establishes 1. that you can use the otel environment variables to configure flags that aren't set and 2. which value overrides which if you declare both?

Done, added TestOTelConfig_EnvVarConfiguresUnsetFlag and TestOTelConfig_ExplicitFlagOverridesEnvVar to the integration test file. First one verifies the SDK picks up OTEL_EXPORTER_OTLP_ENDPOINT when the endpoint flag is not explicitly set, second one documents that an explicit flag value takes precedence over the env var.

miparnisari

Would it be possible to add a new struct within

spicedb/pkg/cmd/server/server.go

Line 63 in ee7c9a7

type Config struct {

called otelConfig or something like that, so that anyone doing server.NewConfigWithOptionsAndDefaults can set these programmatically?

- use ctxkey package for context key instead of a bare struct type - drop legacy otel-jaeger-* flags - collapse ShutdownOTelProvider to a single context shared across ForceFlush and Shutdown - move OTel initialization out of the Cobra layer into Config.Complete via an OTelConfig struct, matching the pattern used by other components - register provider shutdown with closeables so it participates in the ordered server shutdown sequence - add env var precedence tests

codecov · 2026-05-15T12:40:08Z

Codecov Report

❌ Patch coverage is 56.35359% with 79 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.61%. Comparing base (ee7c9a7) to head (edc506d).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/cmd/server/otel.go	59.87%	54 Missing and 5 partials ⚠️
pkg/cmd/server/server.go	26.67%	10 Missing and 1 partial ⚠️
pkg/cmd/serve.go	10.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3108      +/-   ##
==========================================
+ Coverage   75.52%   75.61%   +0.10%     
==========================================
  Files         503      503              
  Lines       61820    62165     +345     
==========================================
+ Hits        46683    47001     +318     
- Misses      11722    11742      +20     
- Partials     3415     3422       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Jdepp007004 · 2026-05-15T12:49:50Z

Addressing miparnisari's review — added OTelConfig as a struct in the server package with an OTel field on Config so it can be set programmatically without going through Cobra flags. InitOTelProvider now takes that struct directly along with a context and has no Cobra dependency at all.

Jdepp007004 added 2 commits May 9, 2026 20:08

remove vendor directory from tracking

ef4547f

Jdepp007004 requested a review from a team as a code owner May 9, 2026 14:47

github-actions Bot added area/cli Affects the command line area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels May 9, 2026

Jdepp007004 closed this May 9, 2026

Jdepp007004 reopened this May 9, 2026

Jdepp007004 changed the title ~~removes vendor directory from tracking~~ otel: replace cobraotel with native lifecycle management May 9, 2026

github-actions Bot locked and limited conversation to collaborators May 9, 2026

authzed unlocked this conversation May 13, 2026

tstirrat15 reviewed May 13, 2026

View reviewed changes

miparnisari requested changes May 13, 2026

View reviewed changes

	func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {
	closeables := util.CloseableStack{}

Conversation

Jdepp007004 commented May 9, 2026

Uh oh!

tstirrat15 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miparnisari May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miparnisari left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jdepp007004 commented May 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

miparnisari May 13, 2026 •

edited

Loading

codecov Bot commented May 15, 2026 •

edited

Loading