diff --git a/pkg/cmd/serve.go b/pkg/cmd/serve.go index 8fe1fc54db..67d3fae5e0 100644 --- a/pkg/cmd/serve.go +++ b/pkg/cmd/serve.go @@ -209,6 +209,11 @@ func RegisterServeFlags(cmd *cobra.Command, config *server.Config) error { // NOTE: cobraotel.New takes service name as an arg rather than command name. otel := cobraotel.New("spicedb") otel.RegisterFlags(tracingFlags) + // Deprecate --otel-provider: the provider is now inferred automatically + // from standard OTEL_* environment variables when not explicitly set. + if err := tracingFlags.MarkDeprecated("otel-provider", "provider is inferred automatically from configuration; this flag will be removed in a future release"); err != nil { + return fmt.Errorf("failed to mark otel-provider as deprecated: %w", err) + } loggingFlagSet := nfs.FlagSet(BoldBlue("Logging")) loggingFlagSet.BoolVar(&config.EnableRequestLogs, "grpc-log-requests-enabled", false, "enable logging of API request payloads") diff --git a/pkg/cmd/serve_test.go b/pkg/cmd/serve_test.go index ec48e4d59a..b5cd401b5b 100644 --- a/pkg/cmd/serve_test.go +++ b/pkg/cmd/serve_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + cobrautil "github.com/jzelinskie/cobrautil/v2" "github.com/spf13/cobra" "github.com/stretchr/testify/require" @@ -198,3 +199,121 @@ func TestConfigPrecedence(t *testing.T) { require.Equal(t, uint16(23000), mergedConfig.DatastoreConfig.WatchBufferLength) }) } + +// runOTelProviderTest wires up a real serve command (so all flags are +// registered) but replaces PreRunE with a minimal stack that only runs +// SyncViperDotEnvPreRunE followed by inferOTelProviderFromEnvPreRunE. This +// means no actual OTel exporter connection is ever opened, keeping the tests +// hermetic. The resolved value of --otel-provider is returned. +func runOTelProviderTest(t *testing.T, args []string, envVars map[string]string) string { + t.Helper() + + for k, v := range envVars { + t.Setenv(k, v) + } + + config := server.NewConfigWithOptionsAndDefaults() + cmd := NewServeCommand("spicedb", config) + require.NoError(t, RegisterRootFlags(cmd)) + require.NoError(t, RegisterServeFlags(cmd, config)) + + // Disable metrics singletons to avoid test pollution. + config.DispatchClusterMetricsEnabled = false + config.DispatchClientMetricsEnabled = false + config.DatastoreConfig.EnableDatastoreMetrics = false + config.DispatchCacheConfig.Metrics = false + config.ClusterDispatchCacheConfig.Metrics = false + config.NamespaceCacheConfig.Metrics = false + + var resolvedProvider string + + // Replace PreRunE with only the two hooks we care about: + // 1. SyncViper – so SPICEDB_* env vars (and .env file) are applied. + // 2. inferOTelProviderFromEnvPreRunE – the logic under test. + // cobraotel.RunE is deliberately excluded so no collector connection + // is ever opened. + cmd.PreRunE = func(cmd *cobra.Command, cmdArgs []string) error { + syncViper := cobrautil.SyncViperPreRunE("spicedb") + if err := syncViper(cmd, cmdArgs); err != nil { + return err + } + infer := server.InferOTelProviderFromEnvPreRunE() + return infer(cmd, cmdArgs) + } + + // RunE just reads the flag after PreRunE has fired. + cmd.RunE = func(cmd *cobra.Command, _ []string) error { + f := cmd.Flags().Lookup("otel-provider") + if f != nil { + resolvedProvider = f.Value.String() + } + return nil + } + + baseArgs := []string{"--grpc-preshared-key", "test-key"} + cmd.SetArgs(append(baseArgs, args...)) + require.NoError(t, cmd.Execute()) + + return resolvedProvider +} + +// TestOTelEnvOnlyEnablesTracing verifies that setting a standard OTEL_* +// environment variable (without any SpiceDB flags) causes the provider to be +// inferred as "otlpgrpc". +func TestOTelEnvOnlyEnablesTracing(t *testing.T) { + provider := runOTelProviderTest(t, nil, map[string]string{ + "OTEL_EXPORTER_OTLP_ENDPOINT": "localhost:4317", + }) + require.Equal(t, "otlpgrpc", provider) +} + +// TestOTelTracesEndpointEnablesTracing covers the OTEL_EXPORTER_OTLP_TRACES_ENDPOINT variant. +func TestOTelTracesEndpointEnablesTracing(t *testing.T) { + provider := runOTelProviderTest(t, nil, map[string]string{ + "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT": "localhost:4317", + }) + require.Equal(t, "otlpgrpc", provider) +} + +// TestOTelTracesExporterEnablesTracing covers the OTEL_TRACES_EXPORTER variant. +func TestOTelTracesExporterEnablesTracing(t *testing.T) { + provider := runOTelProviderTest(t, nil, map[string]string{ + "OTEL_TRACES_EXPORTER": "otlp", + }) + require.Equal(t, "otlpgrpc", provider) +} + +// TestOTelFlagOnlyUnchanged verifies that when only SpiceDB flags are used, +// they are passed through unchanged. +func TestOTelFlagOnlyUnchanged(t *testing.T) { + provider := runOTelProviderTest(t, + []string{"--otel-provider", "otlphttp"}, + nil, + ) + require.Equal(t, "otlphttp", provider) +} + +// TestOTelExplicitNoneDisablesTracing verifies that --otel-provider=none +// overrides any OTEL_* environment variables; tracing stays disabled. +func TestOTelExplicitNoneDisablesTracing(t *testing.T) { + provider := runOTelProviderTest(t, + []string{"--otel-provider", "none"}, + map[string]string{ + "OTEL_EXPORTER_OTLP_ENDPOINT": "localhost:4317", + }, + ) + require.Equal(t, "none", provider) +} + +// TestOTelNoConfigDefaultsToNone verifies the baseline: with no flags and no +// OTEL_* env vars, the provider remains "none" and tracing is disabled. +func TestOTelNoConfigDefaultsToNone(t *testing.T) { + // Scrub any OTEL_* vars that might be set in the test environment. + os.Unsetenv("OTEL_EXPORTER_OTLP_ENDPOINT") + os.Unsetenv("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT") + os.Unsetenv("OTEL_TRACES_EXPORTER") + + provider := runOTelProviderTest(t, nil, nil) + require.Equal(t, "none", provider) +} + diff --git a/pkg/cmd/server/defaults.go b/pkg/cmd/server/defaults.go index c68f67b4fb..c176a05c7d 100644 --- a/pkg/cmd/server/defaults.go +++ b/pkg/cmd/server/defaults.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/http/pprof" + "os" "sync" "time" @@ -25,6 +26,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" "github.com/rs/zerolog" + "github.com/spf13/cobra" "go.opentelemetry.io/otel/trace" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -66,6 +68,43 @@ func ServeExample(programName string) string { ) } +// otelEnvVars is the set of standard OpenTelemetry environment variables that +// indicate the user intends to configure tracing via the environment rather +// than SpiceDB-specific flags. +var otelEnvVars = []string{ + "OTEL_EXPORTER_OTLP_ENDPOINT", + "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", + "OTEL_TRACES_EXPORTER", +} + +// InferOTelProviderFromEnvPreRunE returns a CobraRunFunc that, when the +// --otel-provider flag has NOT been explicitly set by the user, checks whether +// any standard OTEL_* environment variables are present. If they are, it +// promotes the provider to "otlpgrpc" so that cobraotel.RunE will actually +// initialise a tracer provider and let the OpenTelemetry SDK read the rest of +// the configuration from the environment. +func InferOTelProviderFromEnvPreRunE() cobrautil.CobraRunFunc { + return func(cmd *cobra.Command, args []string) error { + flag := cmd.Flags().Lookup("otel-provider") + if flag == nil || flag.Changed { + // Flag does not exist on this command, or the user set it explicitly. + // In either case do not touch it. + return nil + } + + for _, envVar := range otelEnvVars { + if os.Getenv(envVar) != "" { + // At least one standard OTEL env var is set; activate tracing. + if err := flag.Value.Set("otlpgrpc"); err != nil { + return fmt.Errorf("failed to infer otel provider from environment: %w", err) + } + return nil + } + } + return nil + } +} + // DefaultPreRunE sets up viper, zerolog, and OpenTelemetry flag handling for a // command. func DefaultPreRunE(programName string) cobrautil.CobraRunFunc { @@ -82,6 +121,10 @@ func DefaultPreRunE(programName string) cobrautil.CobraRunFunc { // and zero under the same load and 0.9 cobraproclimits.SetMemLimitRunE(memlimit.WithRatio(0.9)), cobraproclimits.SetProcLimitRunE(), + // Infer the OTel provider from standard OTEL_* env vars when the user + // has not explicitly specified --otel-provider. This must run before + // cobraotel.RunE so the correct provider is in place. + InferOTelProviderFromEnvPreRunE(), cobraotel.New("spicedb", cobraotel.WithLogger(zerologr.New(&logging.Logger)), ).RunE(),