service: allow more flexibility in conf-dependent configuration (#52039)

The primary goal of this change is to allow services more flexibility if
they opt out of `conf` integration. It removes the public-facing
`useConfPackage` argument in favour of:

1. `svcmain.SingleServiceMain`, which assumes site configuration access
and initialization.
2. `svcmain.SingleServiceMainWithoutConf`, which does _not_ assume site
configuration access, and allows custom hooks for
site-config-dependent-configuration via a new options struct
`OutOfBandConfiguration`, allowing callers to set up custom
configuration given a lack of `conf` package access

This allows services like LLM-proxy (#50726) and maybe even exeuctors
(the only other service to have `useConfPackage=false` at the moment) to
set up custom configuration for e.g. Sentry sinks on their own terms.

Some other changes:

- Remove the `validateConfig` argument - this was only set to false in
one place, and it seems to belong better in the existing
`svcmain.Config` struct, where it now lives as `SkipValidate`
- Tracing package now accepts an interface with a smaller scope, rather
than all site configuration
- Add `LLM_PROXY_SENTRY_DSN` using the new configuration options

## Test plan

CI, `sg start`, `sg start app`

cmd/sourcegraph-oss/osscmd/osscmd.go

@@ -24,5 +24,5 @@ func MainOSS(services []service.Service, args []string) {
 // SingleServiceMainOSS is called from the `main` function of a command in the OSS build
 // to start a single service (such as frontend or gitserver).
 func SingleServiceMainOSS(service service.Service) {
-	svcmain.SingleServiceMain(service, config, true, true)
+	svcmain.SingleServiceMain(service, config)
 }

enterprise/cmd/llm-proxy/BUILD.bazel

@@ -7,7 +7,11 @@ go_library(
     visibility = ["//visibility:private"],
     deps = [
         "//enterprise/cmd/llm-proxy/shared",
+        "//internal/conf",
+        "//internal/env",
         "//internal/service/svcmain",
+        "@com_github_getsentry_sentry_go//:sentry-go",
+        "@com_github_sourcegraph_log//:log",
     ],
 )
 

enterprise/cmd/llm-proxy/main.go

@@ -1,10 +1,32 @@
 package main
 
 import (
+	"github.com/getsentry/sentry-go"
+	"github.com/sourcegraph/log"
+
 	"github.com/sourcegraph/sourcegraph/enterprise/cmd/llm-proxy/shared"
+	"github.com/sourcegraph/sourcegraph/internal/conf"
+	"github.com/sourcegraph/sourcegraph/internal/env"
 	"github.com/sourcegraph/sourcegraph/internal/service/svcmain"
 )
 
+var sentryDSN = env.Get("LLM_PROXY_SENTRY_DSN", "", "Sentry DSN")
+
 func main() {
-	svcmain.SingleServiceMain(shared.Service, svcmain.Config{}, true, false)
+	svcmain.SingleServiceMainWithoutConf(shared.Service, svcmain.Config{}, svcmain.OutOfBandConfiguration{
+		Logging: func() conf.LogSinksSource {
+			if sentryDSN == "" {
+				return nil
+			}
+
+			return conf.NewStaticLogsSinksSource(log.SinksConfig{
+				Sentry: &log.SentrySink{
+					ClientOptions: sentry.ClientOptions{
+						Dsn: sentryDSN,
+					},
+				},
+			})
+		}(),
+		Tracing: nil,
+	})
 }

enterprise/cmd/sourcegraph/enterprisecmd/enterprisecmd.go

@@ -19,7 +19,7 @@ func MainEnterprise(services []service.Service, args []string) {
 // SingleServiceMainEnterprise is called from the `main` function of a command in the
 // enterprise (non-OSS) build to start a single service (such as frontend or gitserver).
 func SingleServiceMainEnterprise(service service.Service) {
-	svcmain.SingleServiceMain(service, config, true, true)
+	svcmain.SingleServiceMain(service, config)
 }
 
 func init() {

enterprise/cmd/sourcegraph/enterprisecmd/executorcmd/executorcmd.go

@@ -10,10 +10,12 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/service/svcmain"
 )
 
-var config = svcmain.Config{}
+var config = svcmain.Config{
+	SkipValidate: true,
+}
 
 // SingleServiceMainEnterprise is called from the `main` function of a command in the
 // enterprise (non-OSS) build to start a single service (such as frontend or gitserver).
 func SingleServiceMainEnterprise(service service.Service) {
-	svcmain.SingleServiceMain(service, config, false, false)
+	svcmain.SingleServiceMainWithoutConf(service, config, svcmain.OutOfBandConfiguration{})
 }

internal/conf/log_sinks.go

@@ -3,10 +3,27 @@ package conf
 import (
 	"github.com/getsentry/sentry-go"
 	"github.com/sourcegraph/log"
+
+	"github.com/sourcegraph/sourcegraph/internal/conf/conftypes"
 )
 
-func GetLogSinks() log.SinksConfig {
-	cfg := Get()
+type LogSinksSource interface {
+	SinksConfig() log.SinksConfig
+
+	// Watchable allows the caller to be notified when the configuration changes.
+	conftypes.Watchable
+}
+
+// NewLogsSinksSource wraps WatchableSiteConfig with a method that generates a valid
+// sinks configuration for sourcegraph/log.
+func NewLogsSinksSource(c conftypes.WatchableSiteConfig) LogSinksSource {
+	return logSinksSource{c}
+}
+
+type logSinksSource struct{ conftypes.WatchableSiteConfig }
+
+func (s logSinksSource) SinksConfig() log.SinksConfig {
+	cfg := s.SiteConfig()
 
 	var sentrySink *log.SentrySink
 	if cfg.Log != nil {
@@ -23,3 +40,15 @@ func GetLogSinks() log.SinksConfig {
 		Sentry: sentrySink,
 	}
 }
+
+// NewStaticLogsSinksSource procs immediately and only once when Watch is called,
+// and returns a static config.
+func NewStaticLogsSinksSource(cfg log.SinksConfig) LogSinksSource {
+	return staticLogSinksSource{cfg: cfg}
+}
+
+type staticLogSinksSource struct{ cfg log.SinksConfig }
+
+func (s staticLogSinksSource) SinksConfig() log.SinksConfig { return s.cfg }
+
+func (s staticLogSinksSource) Watch(fn func()) { fn() } // proc immediately

internal/service/svcmain/svcmain.go

@@ -29,7 +29,10 @@ import (
 )
 
 type Config struct {
-	AfterConfigure func() // run after all services' Configure hooks are called
+	// SkipValidate, if true, will skip validation of service configuration.
+	SkipValidate bool
+	// AfterConfigure, if provided, is run after all services' Configure hooks are called
+	AfterConfigure func()
 }
 
 // Main is called from the `main` function of the `sourcegraph-oss` and
@@ -86,7 +89,7 @@ func Main(services []sgservice.Service, config Config, args []string) {
 	app.Action = func(_ *cli.Context) error {
 		logger := log.Scoped("sourcegraph", "Sourcegraph")
 		singleprogram.Init(logger)
-		run(liblog, logger, services, config, true, true)
+		run(liblog, logger, services, config, nil)
 		return nil
 	}
 
@@ -97,8 +100,13 @@ func Main(services []sgservice.Service, config Config, args []string) {
 }
 
 // SingleServiceMain is called from the `main` function of a command to start a single
-// service (such as frontend or gitserver).
-func SingleServiceMain(svc sgservice.Service, config Config, validateConfig, useConfPackage bool) {
+// service (such as frontend or gitserver). It assumes the service can access site
+// configuration and initializes the conf package, and sets up some default hooks for
+// watching site configuration for instrumentation services like tracing and logging.
+//
+// If your service cannot access site configuration, use SingleServiceMainWithoutConf
+// instead.
+func SingleServiceMain(svc sgservice.Service, config Config) {
 	liblog := log.Init(log.Resource{
 		Name:       env.MyName,
 		Version:    version.Version(),
@@ -112,7 +120,39 @@ func SingleServiceMain(svc sgservice.Service, config Config, validateConfig, use
 		),
 	)
 	logger := log.Scoped("sourcegraph", "Sourcegraph")
-	run(liblog, logger, []sgservice.Service{svc}, config, validateConfig, useConfPackage)
+	run(liblog, logger, []sgservice.Service{svc}, config, nil)
+}
+
+// OutOfBandConfiguration declares additional configuration that happens continuously,
+// separate from service startup. In most cases this is configuration based on site config
+// (the conf package).
+type OutOfBandConfiguration struct {
+	// Logging is used to configure logging.
+	Logging conf.LogSinksSource
+
+	// Tracing is used to configure tracing.
+	Tracing tracer.WatchableConfigurationSource
+}
+
+// SingleServiceMainWithConf is called from the `main` function of a command to start a single
+// service WITHOUT site configuration enabled by default. This is only useful for services
+// that are not part of the core Sourcegraph deployment, such as executors and managed
+// services. Use with care!
+func SingleServiceMainWithoutConf(svc sgservice.Service, config Config, oobConfig OutOfBandConfiguration) {
+	liblog := log.Init(log.Resource{
+		Name:       env.MyName,
+		Version:    version.Version(),
+		InstanceID: hostname.Get(),
+	},
+		// Experimental: DevX is observing how sampling affects the errors signal.
+		log.NewSentrySinkWith(
+			log.SentrySink{
+				ClientOptions: sentry.ClientOptions{SampleRate: 0.2},
+			},
+		),
+	)
+	logger := log.Scoped("sourcegraph", "Sourcegraph")
+	run(liblog, logger, []sgservice.Service{svc}, config, &oobConfig)
 }
 
 func run(
@@ -120,19 +160,32 @@ func run(
 	logger log.Logger,
 	services []sgservice.Service,
 	config Config,
-	validateConfig bool,
-	useConfPackage bool,
+	// If nil, will use site config
+	oobConfig *OutOfBandConfiguration,
 ) {
 	defer liblog.Sync()
 
 	// Initialize log15. Even though it's deprecated, it's still fairly widely used.
 	logging.Init() //nolint:staticcheck // Deprecated, but logs unmigrated to sourcegraph/log look really bad without this.
 
-	if useConfPackage {
+	// If no oobConfig is provided, we're in conf mode
+	if oobConfig == nil {
 		conf.Init()
-		go conf.Watch(liblog.Update(conf.GetLogSinks))
-		tracer.Init(log.Scoped("tracer", "internal tracer package"), conf.DefaultClient())
+		oobConfig = &OutOfBandConfiguration{
+			Logging: conf.NewLogsSinksSource(conf.DefaultClient()),
+			Tracing: tracer.ConfConfigurationSource{WatchableSiteConfig: conf.DefaultClient()},
+		}
 	}
+
+	if oobConfig.Logging != nil {
+		go oobConfig.Logging.Watch(func() {
+			liblog.Update(oobConfig.Logging.SinksConfig)
+		})
+	}
+	if oobConfig.Tracing != nil {
+		tracer.Init(log.Scoped("tracer", "internal tracer package"), oobConfig.Tracing)
+	}
+
 	profiler.Init()
 
 	obctx := observation.NewContext(logger)
@@ -154,7 +207,7 @@ func run(
 	// Validate each service's configuration.
 	//
 	// This cannot be done for executor, see the executorcmd package for details.
-	if validateConfig {
+	if !config.SkipValidate {
 		for i, c := range serviceConfigs {
 			if c == nil {
 				continue

internal/tracer/BUILD.bazel

@@ -3,6 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 go_library(
     name = "tracer",
     srcs = [
+        "conf.go",
         "doc.go",
         "logged_ot.go",
         "logged_otel.go",
@@ -23,6 +24,7 @@ go_library(
         "//internal/tracer/oteldefaults/exporters",
         "//internal/version",
         "//lib/errors",
+        "//schema",
         "@com_github_opentracing_opentracing_go//:opentracing-go",
         "@com_github_sourcegraph_log//:log",
         "@io_opentelemetry_go_otel//:otel",
@@ -42,7 +44,6 @@ go_test(
     srcs = ["watch_test.go"],
     embed = [":tracer"],
     deps = [
-        "//internal/conf/conftypes",
         "//internal/trace/policy",
         "//schema",
         "@com_github_sourcegraph_log//:log",

internal/tracer/conf.go

@@ -0,0 +1,17 @@
+package tracer
+
+import "github.com/sourcegraph/sourcegraph/internal/conf/conftypes"
+
+// ConfConfigurationSource wraps a basic conf-based configuration source in
+// a tracing-specific configuration source.
+type ConfConfigurationSource struct{ conftypes.WatchableSiteConfig }
+
+var _ WatchableConfigurationSource = ConfConfigurationSource{}
+
+func (c ConfConfigurationSource) Config() Configuration {
+	s := c.SiteConfig()
+	return Configuration{
+		ExternalURL:          s.ExternalURL,
+		ObservabilityTracing: s.ObservabilityTracing,
+	}
+}

internal/tracer/tracer.go

@@ -19,6 +19,7 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/hostname"
 	"github.com/sourcegraph/sourcegraph/internal/tracer/oteldefaults"
 	"github.com/sourcegraph/sourcegraph/internal/version"
+	"github.com/sourcegraph/sourcegraph/schema"
 )
 
 // options control the behavior of a TracerType
@@ -55,8 +56,24 @@ func (t TracerType) isSetByUser() bool {
 	return false
 }
 
+type Configuration struct {
+	ExternalURL string
+	*schema.ObservabilityTracing
+}
+
+type ConfigurationSource interface {
+	Config() Configuration
+}
+
+type WatchableConfigurationSource interface {
+	ConfigurationSource
+
+	// Watchable allows the caller to be notified when the configuration changes.
+	conftypes.Watchable
+}
+
 // Init should be called from the main function of service
-func Init(logger log.Logger, c conftypes.WatchableSiteConfig) {
+func Init(logger log.Logger, c WatchableConfigurationSource) {
 	// Tune GOMAXPROCS for kubernetes. All our binaries import this package,
 	// so we tune for all of them.
 	//

internal/tracer/watch.go

@@ -6,7 +6,6 @@ import (
 	"github.com/sourcegraph/log"
 	oteltracesdk "go.opentelemetry.io/otel/sdk/trace"
 
-	"github.com/sourcegraph/sourcegraph/internal/conf/conftypes"
 	"github.com/sourcegraph/sourcegraph/internal/trace/policy"
 )
 
@@ -16,7 +15,7 @@ import (
 // provider and debugMode references.
 func newConfWatcher(
 	logger log.Logger,
-	c conftypes.SiteConfigQuerier,
+	c ConfigurationSource,
 	// provider will be updated with the appropriate span processors.
 	provider *oteltracesdk.TracerProvider,
 	// spanProcessorBuilder is used to create span processors to configure on the provider
@@ -36,7 +35,7 @@ func newConfWatcher(
 	// return function to be called on every conf update
 	return func() {
 		var (
-			siteConfig     = c.SiteConfig()
+			siteConfig     = c.Config()
 			tracingConfig  = siteConfig.ObservabilityTracing
 			previousPolicy = policy.GetTracePolicy()
 			setTracerType  = None

internal/tracer/watch_test.go

@@ -11,18 +11,17 @@ import (
 	oteltracesdk "go.opentelemetry.io/otel/sdk/trace"
 	"go.opentelemetry.io/otel/sdk/trace/tracetest"
 
-	"github.com/sourcegraph/sourcegraph/internal/conf/conftypes"
 	"github.com/sourcegraph/sourcegraph/internal/trace/policy"
 	"github.com/sourcegraph/sourcegraph/schema"
 )
 
 type mockConfig struct {
-	get func() schema.SiteConfiguration
+	get func() Configuration
 }
 
-var _ conftypes.SiteConfigQuerier = &mockConfig{}
+var _ ConfigurationSource = &mockConfig{}
 
-func (m *mockConfig) SiteConfig() schema.SiteConfiguration { return m.get() }
+func (m *mockConfig) Config() Configuration { return m.get() }
 
 func TestConfigWatcher(t *testing.T) {
 	var (
@@ -43,8 +42,8 @@ func TestConfigWatcher(t *testing.T) {
 		doUpdate := newConfWatcher(
 			logger,
 			&mockConfig{
-				get: func() schema.SiteConfiguration {
-					return schema.SiteConfiguration{
+				get: func() Configuration {
+					return Configuration{
 						ObservabilityTracing: nil,
 					}
 				},
@@ -67,8 +66,8 @@ func TestConfigWatcher(t *testing.T) {
 
 	t.Run("enable tracing with 'observability.tracing: {}'", func(t *testing.T) {
 		mockConfig := &mockConfig{
-			get: func() schema.SiteConfiguration {
-				return schema.SiteConfiguration{
+			get: func() Configuration {
+				return Configuration{
 					ObservabilityTracing: &schema.ObservabilityTracing{},
 				}
 			},
@@ -123,8 +122,8 @@ func TestConfigWatcher(t *testing.T) {
 		})
 
 		t.Run("disable tracing after enabling it", func(t *testing.T) {
-			mockConfig.get = func() schema.SiteConfiguration {
-				return schema.SiteConfiguration{
+			mockConfig.get = func() Configuration {
+				return Configuration{
 					ObservabilityTracing: &schema.ObservabilityTracing{Sampling: "none"},
 				}
 			}
@@ -155,8 +154,8 @@ func TestConfigWatcher(t *testing.T) {
 
 	t.Run("update tracing with debug and sampling all", func(t *testing.T) {
 		mockConf := &mockConfig{
-			get: func() schema.SiteConfiguration {
-				return schema.SiteConfiguration{
+			get: func() Configuration {
+				return Configuration{
 					ObservabilityTracing: &schema.ObservabilityTracing{
 						Debug:    true,
 						Sampling: "all",
@@ -204,8 +203,8 @@ func TestConfigWatcher(t *testing.T) {
 		t.Run("sanity check - swap existing processor with another", func(t *testing.T) {
 			spansRecorder2 := tracetest.NewSpanRecorder()
 			updatedSpanProcessor = spansRecorder2
-			mockConf.get = func() schema.SiteConfiguration {
-				return schema.SiteConfiguration{
+			mockConf.get = func() Configuration {
+				return Configuration{
 					ObservabilityTracing: &schema.ObservabilityTracing{
 						Debug:    true,
 						Sampling: "all",