[chore] Move logging out of meter provider initialization (#9729)

**Description:** Moves logging messages about the meter provider outside of the meter provider initialization. While working on #4970 (comment), I realized there is an implicit dependency in the initialization order of the different telemetry components. I tried making this work and make the factory have a single `CreateTelemetrySettings`, but this results in an awkward API, so I am trying the alternative here: don't log during the meter provider initialization, but do so outside of it. **Link to tracking Issue:** Relates to #4970 --------- Co-authored-by: Alex Boten <[email protected]>
open-telemetry · Apr 16, 2024 · 370c6d1 · 370c6d1
1 parent fb78b16
commit 370c6d1
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 21 deletions.
diff --git a/service/service.go b/service/service.go
@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"runtime"
 
+	"go.opentelemetry.io/otel/metric"
 	sdkresource "go.opentelemetry.io/otel/sdk/resource"
 	"go.uber.org/multierr"
 	"go.uber.org/zap"
@@ -95,19 +96,20 @@ func New(ctx context.Context, set Settings, cfg Config) (*Service, error) {
 	pcommonRes := pdataFromSdk(res)
 
 	logger := tel.Logger()
+	logger.Info("Setting up own telemetry...")
 	mp, err := newMeterProvider(
 		meterProviderSettings{
 			res:               res,
-			logger:            logger,
 			cfg:               cfg.Telemetry.Metrics,
 			asyncErrorChannel: set.AsyncErrorChannel,
 		},
 		disableHighCard,
-		extendedConfig,
 	)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create metric provider: %w", err)
 	}
+
+	logsAboutMeterProvider(logger, cfg.Telemetry.Metrics, mp, extendedConfig)
 	srv.telemetrySettings = servicetelemetry.TelemetrySettings{
 		Logger:         logger,
 		MeterProvider:  mp,
@@ -133,6 +135,27 @@ func New(ctx context.Context, set Settings, cfg Config) (*Service, error) {
 	return srv, nil
 }
 
+func logsAboutMeterProvider(logger *zap.Logger, cfg telemetry.MetricsConfig, mp metric.MeterProvider, extendedConfig bool) {
+	if cfg.Level == configtelemetry.LevelNone || (cfg.Address == "" && len(cfg.Readers) == 0) {
+		logger.Info(
+			"Skipped telemetry setup.",
+			zap.String(zapKeyTelemetryAddress, cfg.Address),
+			zap.Stringer(zapKeyTelemetryLevel, cfg.Level),
+		)
+		return
+	}
+
+	if len(cfg.Address) != 0 && extendedConfig {
+		logger.Warn("service::telemetry::metrics::address is being deprecated in favor of service::telemetry::metrics::readers")
+	}
+
+	if lmp, ok := mp.(interface {
+		LogAboutServers(logger *zap.Logger, cfg telemetry.MetricsConfig)
+	}); ok {
+		lmp.LogAboutServers(logger, cfg)
+	}
+}
+
 // Start starts the extensions and pipelines. If Start fails Shutdown should be called to ensure a clean state.
 // Start does the following steps in order:
 // 1. Start all extensions.

diff --git a/service/telemetry.go b/service/telemetry.go
@@ -37,26 +37,16 @@ type meterProvider struct {
 
 type meterProviderSettings struct {
 	res               *resource.Resource
-	logger            *zap.Logger
 	cfg               telemetry.MetricsConfig
 	asyncErrorChannel chan error
 }
 
-func newMeterProvider(set meterProviderSettings, disableHighCardinality bool, extendedConfig bool) (metric.MeterProvider, error) {
+func newMeterProvider(set meterProviderSettings, disableHighCardinality bool) (metric.MeterProvider, error) {
 	if set.cfg.Level == configtelemetry.LevelNone || (set.cfg.Address == "" && len(set.cfg.Readers) == 0) {
-		set.logger.Info(
-			"Skipping telemetry setup.",
-			zap.String(zapKeyTelemetryAddress, set.cfg.Address),
-			zap.String(zapKeyTelemetryLevel, set.cfg.Level.String()),
-		)
 		return noopmetric.NewMeterProvider(), nil
 	}
 
-	set.logger.Info("Setting up own telemetry...")
 	if len(set.cfg.Address) != 0 {
-		if extendedConfig {
-			set.logger.Warn("service::telemetry::metrics::address is being deprecated in favor of service::telemetry::metrics::readers")
-		}
 		host, port, err := net.SplitHostPort(set.cfg.Address)
 		if err != nil {
 			return nil, err
@@ -94,11 +84,7 @@ func newMeterProvider(set meterProviderSettings, disableHighCardinality bool, ex
 		}
 		if server != nil {
 			mp.servers = append(mp.servers, server)
-			set.logger.Info(
-				"Serving metrics",
-				zap.String(zapKeyTelemetryAddress, server.Addr),
-				zap.String(zapKeyTelemetryLevel, set.cfg.Level.String()),
-			)
+
 		}
 		opts = append(opts, sdkmetric.WithReader(r))
 	}
@@ -111,6 +97,17 @@ func newMeterProvider(set meterProviderSettings, disableHighCardinality bool, ex
 	return mp, nil
 }
 
+// LogAboutServers logs about the servers that are serving metrics.
+func (mp *meterProvider) LogAboutServers(logger *zap.Logger, cfg telemetry.MetricsConfig) {
+	for _, server := range mp.servers {
+		logger.Info(
+			"Serving metrics",
+			zap.String(zapKeyTelemetryAddress, server.Addr),
+			zap.Stringer(zapKeyTelemetryLevel, cfg.Level),
+		)
+	}
+}
+
 // Shutdown the meter provider and all the associated resources.
 // The type signature of this method matches that of the sdkmetric.MeterProvider.
 func (mp *meterProvider) Shutdown(ctx context.Context) error {

diff --git a/service/telemetry_test.go b/service/telemetry_test.go
@@ -16,7 +16,6 @@ import (
 	"go.opencensus.io/stats/view"
 	"go.opentelemetry.io/contrib/config"
 	"go.opentelemetry.io/otel/metric"
-	"go.uber.org/zap"
 
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/config/configtelemetry"
@@ -241,11 +240,10 @@ func TestTelemetryInit(t *testing.T) {
 			}
 			set := meterProviderSettings{
 				res:               resource.New(component.NewDefaultBuildInfo(), tc.cfg.Resource),
-				logger:            zap.NewNop(),
 				cfg:               tc.cfg.Metrics,
 				asyncErrorChannel: make(chan error),
 			}
-			mp, err := newMeterProvider(set, tc.disableHighCard, tc.extendedConfig)
+			mp, err := newMeterProvider(set, tc.disableHighCard)
 			require.NoError(t, err)
 			defer func() {
 				if prov, ok := mp.(interface{ Shutdown(context.Context) error }); ok {