From 370c6d16929627e722646f716bb20980bf547181 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Tue, 16 Apr 2024 17:44:34 +0200 Subject: [PATCH] [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 https://github.com/open-telemetry/opentelemetry-collector/issues/4970#issuecomment-1911978462, 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 <223565+codeboten@users.noreply.github.com> --- service/service.go | 27 +++++++++++++++++++++++++-- service/telemetry.go | 29 +++++++++++++---------------- service/telemetry_test.go | 4 +--- 3 files changed, 39 insertions(+), 21 deletions(-) diff --git a/service/service.go b/service/service.go index febff396417..283e9c94d09 100644 --- 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 index c7aab098747..365ea3d23c5 100644 --- 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 index c95a42ab90b..2377a077052 100644 --- 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 {