From b26e22d0242a1648d9e520c9c5cac3cc7eb58274 Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Wed, 18 Sep 2024 11:47:19 -0700 Subject: [PATCH] Deprecate service::telemetry::metrics::address Signed-off-by: Bogdan Drutu --- .chloggen/deprecate-address.yaml | 20 +++ otelcol/collector_test.go | 4 +- otelcol/config_test.go | 18 ++- otelcol/go.mod | 2 +- ...address.yaml => otelcol-emptyreaders.yaml} | 2 +- .../otelcol-invalid-receiver-type.yaml | 7 +- otelcol/testdata/otelcol-invalid.yaml | 7 +- otelcol/testdata/otelcol-invalidprop.yaml | 7 +- otelcol/testdata/otelcol-nop.yaml | 7 +- ...-nometrics.yaml => otelcol-noreaders.yaml} | 0 otelcol/testdata/otelcol-statuswatcher.yaml | 7 +- otelcol/testdata/otelcol-validprop.yaml | 7 +- otelcol/unmarshaler_test.go | 7 +- service/config_test.go | 12 +- service/service.go | 1 + service/service_test.go | 86 ++---------- service/telemetry/config.go | 62 ++++++++- service/telemetry/config_test.go | 123 +++++++++++++++--- service/telemetry/factory.go | 14 +- service/telemetry/factory_test.go | 19 ++- service/telemetry/metrics.go | 29 +---- service/telemetry/metrics_test.go | 8 +- .../testdata/config_deprecated_address.yaml | 5 + ...config_deprecated_address_and_readers.yaml | 11 ++ .../testdata/config_empty_readers.yaml | 5 + .../config_invalid_deprecated_address.yaml | 5 + 26 files changed, 321 insertions(+), 154 deletions(-) create mode 100644 .chloggen/deprecate-address.yaml rename otelcol/testdata/{otelcol-noaddress.yaml => otelcol-emptyreaders.yaml} (88%) rename otelcol/testdata/{otelcol-nometrics.yaml => otelcol-noreaders.yaml} (100%) create mode 100644 service/telemetry/testdata/config_deprecated_address.yaml create mode 100644 service/telemetry/testdata/config_deprecated_address_and_readers.yaml create mode 100644 service/telemetry/testdata/config_empty_readers.yaml create mode 100644 service/telemetry/testdata/config_invalid_deprecated_address.yaml diff --git a/.chloggen/deprecate-address.yaml b/.chloggen/deprecate-address.yaml new file mode 100644 index 00000000000..2ebaab41b54 --- /dev/null +++ b/.chloggen/deprecate-address.yaml @@ -0,0 +1,20 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: 'deprecation' + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: service/telemetry + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Deprecate service::telemetry::metrics::address in favor of service::telemetry::metrics::readers. + +# One or more tracking issues or pull requests related to the change +issues: [11205] + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [user] diff --git a/otelcol/collector_test.go b/otelcol/collector_test.go index 7db8215eb39..2e84e32b876 100644 --- a/otelcol/collector_test.go +++ b/otelcol/collector_test.go @@ -389,8 +389,8 @@ func TestCollectorRun(t *testing.T) { tests := []struct { file string }{ - {file: "otelcol-nometrics.yaml"}, - {file: "otelcol-noaddress.yaml"}, + {file: "otelcol-noreaders.yaml"}, + {file: "otelcol-emptyreaders.yaml"}, } for _, tt := range tests { diff --git a/otelcol/config_test.go b/otelcol/config_test.go index 25b1417cd2c..c1e43afba69 100644 --- a/otelcol/config_test.go +++ b/otelcol/config_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/contrib/config" "go.uber.org/zap/zapcore" "go.opentelemetry.io/collector/component" @@ -275,8 +276,17 @@ func generateConfig() *Config { InitialFields: map[string]any{"fieldKey": "filed-value"}, }, Metrics: telemetry.MetricsConfig{ - Level: configtelemetry.LevelNormal, - Address: ":8080", + Level: configtelemetry.LevelNormal, + Readers: []config.MetricReader{ + { + Pull: &config.PullMetricReader{Exporter: config.MetricExporter{ + Prometheus: &config.Prometheus{ + Host: newPtr("localhost"), + Port: newPtr(8080), + }, + }}, + }, + }, }, }, Extensions: []component.ID{component.MustNewID("nop")}, @@ -290,3 +300,7 @@ func generateConfig() *Config { }, } } + +func newPtr[T int | string](str T) *T { + return &str +} diff --git a/otelcol/go.mod b/otelcol/go.mod index cfb4fd40b3b..047331134b6 100644 --- a/otelcol/go.mod +++ b/otelcol/go.mod @@ -17,6 +17,7 @@ require ( go.opentelemetry.io/collector/processor v0.110.0 go.opentelemetry.io/collector/receiver v0.110.0 go.opentelemetry.io/collector/service v0.110.0 + go.opentelemetry.io/contrib/config v0.10.0 go.uber.org/goleak v1.3.0 go.uber.org/multierr v1.11.0 go.uber.org/zap v1.27.0 @@ -79,7 +80,6 @@ require ( go.opentelemetry.io/collector/processor/processorprofiles v0.110.0 // indirect go.opentelemetry.io/collector/receiver/receiverprofiles v0.110.0 // indirect go.opentelemetry.io/collector/semconv v0.110.0 // indirect - go.opentelemetry.io/contrib/config v0.10.0 // indirect go.opentelemetry.io/contrib/propagators/b3 v1.30.0 // indirect go.opentelemetry.io/otel v1.30.0 // indirect go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp v0.6.0 // indirect diff --git a/otelcol/testdata/otelcol-noaddress.yaml b/otelcol/testdata/otelcol-emptyreaders.yaml similarity index 88% rename from otelcol/testdata/otelcol-noaddress.yaml rename to otelcol/testdata/otelcol-emptyreaders.yaml index c363f848e53..3a71b541f1a 100644 --- a/otelcol/testdata/otelcol-noaddress.yaml +++ b/otelcol/testdata/otelcol-emptyreaders.yaml @@ -7,7 +7,7 @@ exporters: service: telemetry: metrics: - address: "" + readers: [] pipelines: metrics: receivers: [nop] diff --git a/otelcol/testdata/otelcol-invalid-receiver-type.yaml b/otelcol/testdata/otelcol-invalid-receiver-type.yaml index 5837810fcca..7bcad410bb9 100644 --- a/otelcol/testdata/otelcol-invalid-receiver-type.yaml +++ b/otelcol/testdata/otelcol-invalid-receiver-type.yaml @@ -10,7 +10,12 @@ exporters: service: telemetry: metrics: - address: localhost:8888 + readers: + - pull: + exporter: + prometheus: + host: "localhost" + port: 8888 pipelines: traces: receivers: [nop_logs] diff --git a/otelcol/testdata/otelcol-invalid.yaml b/otelcol/testdata/otelcol-invalid.yaml index 133ce74d85b..dc48aee782f 100644 --- a/otelcol/testdata/otelcol-invalid.yaml +++ b/otelcol/testdata/otelcol-invalid.yaml @@ -10,7 +10,12 @@ exporters: service: telemetry: metrics: - address: localhost:8888 + readers: + - pull: + exporter: + prometheus: + host: "localhost" + port: 8888 pipelines: traces: receivers: [nop] diff --git a/otelcol/testdata/otelcol-invalidprop.yaml b/otelcol/testdata/otelcol-invalidprop.yaml index c84fbdbd017..4022c90cbea 100644 --- a/otelcol/testdata/otelcol-invalidprop.yaml +++ b/otelcol/testdata/otelcol-invalidprop.yaml @@ -14,7 +14,12 @@ service: - "unknown" - "tracecontext" metrics: - address: localhost:8888 + readers: + - pull: + exporter: + prometheus: + host: "localhost" + port: 8888 pipelines: traces: receivers: [nop] diff --git a/otelcol/testdata/otelcol-nop.yaml b/otelcol/testdata/otelcol-nop.yaml index c83a07c536c..3d361b0afdd 100644 --- a/otelcol/testdata/otelcol-nop.yaml +++ b/otelcol/testdata/otelcol-nop.yaml @@ -16,7 +16,12 @@ connectors: service: telemetry: metrics: - address: localhost:8888 + readers: + - pull: + exporter: + prometheus: + host: "localhost" + port: 8888 extensions: [nop] pipelines: traces: diff --git a/otelcol/testdata/otelcol-nometrics.yaml b/otelcol/testdata/otelcol-noreaders.yaml similarity index 100% rename from otelcol/testdata/otelcol-nometrics.yaml rename to otelcol/testdata/otelcol-noreaders.yaml diff --git a/otelcol/testdata/otelcol-statuswatcher.yaml b/otelcol/testdata/otelcol-statuswatcher.yaml index 2dcc322d341..90970a234d7 100644 --- a/otelcol/testdata/otelcol-statuswatcher.yaml +++ b/otelcol/testdata/otelcol-statuswatcher.yaml @@ -14,7 +14,12 @@ extensions: service: telemetry: metrics: - address: localhost:8888 + readers: + - pull: + exporter: + prometheus: + host: "localhost" + port: 8888 extensions: [statuswatcher] pipelines: traces: diff --git a/otelcol/testdata/otelcol-validprop.yaml b/otelcol/testdata/otelcol-validprop.yaml index 5c611333c17..1d478cabae9 100644 --- a/otelcol/testdata/otelcol-validprop.yaml +++ b/otelcol/testdata/otelcol-validprop.yaml @@ -14,7 +14,12 @@ service: - "b3" - "tracecontext" metrics: - address: localhost:8888 + readers: + - pull: + exporter: + prometheus: + host: "localhost" + port: 8888 pipelines: traces: receivers: [nop] diff --git a/otelcol/unmarshaler_test.go b/otelcol/unmarshaler_test.go index 436d88c0c0f..9fdb0832491 100644 --- a/otelcol/unmarshaler_test.go +++ b/otelcol/unmarshaler_test.go @@ -159,7 +159,7 @@ func TestServiceUnmarshalError(t *testing.T) { }, }, }), - expectError: "error decoding 'telemetry.logs.level': unrecognized level: \"UNKNOWN\"", + expectError: "error decoding 'telemetry': decoding failed due to the following error(s):\n\nerror decoding 'logs.level': unrecognized level: \"UNKNOWN\"", }, { name: "invalid-metrics-level", @@ -170,7 +170,7 @@ func TestServiceUnmarshalError(t *testing.T) { }, }, }), - expectError: "error decoding 'telemetry.metrics.level': unknown metrics level \"unknown\"", + expectError: "error decoding 'telemetry': decoding failed due to the following error(s):\n\nerror decoding 'metrics.level': unknown metrics level \"unknown\"", }, { name: "invalid-service-extensions-section", @@ -204,8 +204,7 @@ func TestServiceUnmarshalError(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { err := tt.conf.Unmarshal(&service.Config{}) - require.Error(t, err) - assert.Contains(t, err.Error(), tt.expectError) + require.ErrorContains(t, err, tt.expectError) }) } } diff --git a/service/config_test.go b/service/config_test.go index 2dd0246e734..413887de731 100644 --- a/service/config_test.go +++ b/service/config_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/contrib/config" "go.uber.org/zap/zapcore" "go.opentelemetry.io/collector/component" @@ -67,7 +68,7 @@ func TestConfigValidate(t *testing.T) { cfgFn: func() *Config { cfg := generateConfig() cfg.Telemetry.Metrics.Level = configtelemetry.LevelBasic - cfg.Telemetry.Metrics.Address = "" + cfg.Telemetry.Metrics.Readers = nil return cfg }, expected: nil, @@ -96,8 +97,13 @@ func generateConfig() *Config { InitialFields: map[string]any{"fieldKey": "filed-value"}, }, Metrics: telemetry.MetricsConfig{ - Level: configtelemetry.LevelNormal, - Address: ":8080", + Level: configtelemetry.LevelNormal, + Readers: []config.MetricReader{{ + Pull: &config.PullMetricReader{Exporter: config.MetricExporter{Prometheus: &config.Prometheus{ + Host: newPtr("localhost"), + Port: newPtr(8080), + }}}}, + }, }, }, Extensions: extensions.Config{component.MustNewID("nop")}, diff --git a/service/service.go b/service/service.go index 83a8a12c848..ada723b7c90 100644 --- a/service/service.go +++ b/service/service.go @@ -187,6 +187,7 @@ func logsAboutMeterProvider(logger *zap.Logger, cfg telemetry.MetricsConfig, mp return } + //nolint if len(cfg.Address) != 0 { logger.Warn("service::telemetry::metrics::address is being deprecated in favor of service::telemetry::metrics::readers") } diff --git a/service/service_test.go b/service/service_test.go index 8420e52a62c..23cbaa5509f 100644 --- a/service/service_test.go +++ b/service/service_test.go @@ -289,78 +289,6 @@ func TestServiceTelemetryCleanupOnError(t *testing.T) { } func TestServiceTelemetry(t *testing.T) { - for _, tc := range ownMetricsTestCases() { - t.Run(fmt.Sprintf("ipv4_%s", tc.name), func(t *testing.T) { - testCollectorStartHelper(t, tc, "tcp4") - }) - t.Run(fmt.Sprintf("ipv6_%s", tc.name), func(t *testing.T) { - testCollectorStartHelper(t, tc, "tcp6") - }) - } -} - -func testCollectorStartHelper(t *testing.T, tc ownMetricsTestCase, network string) { - var once sync.Once - loggingHookCalled := false - hook := func(zapcore.Entry) error { - once.Do(func() { - loggingHookCalled = true - }) - return nil - } - - var ( - metricsAddr string - zpagesAddr string - ) - switch network { - case "tcp", "tcp4": - metricsAddr = testutil.GetAvailableLocalAddress(t) - zpagesAddr = testutil.GetAvailableLocalAddress(t) - case "tcp6": - metricsAddr = testutil.GetAvailableLocalIPv6Address(t) - zpagesAddr = testutil.GetAvailableLocalIPv6Address(t) - } - require.NotZero(t, metricsAddr, "network must be either of tcp, tcp4 or tcp6") - require.NotZero(t, zpagesAddr, "network must be either of tcp, tcp4 or tcp6") - - set := newNopSettings() - set.BuildInfo = component.BuildInfo{Version: "test version", Command: otelCommand} - set.ExtensionsConfigs = map[component.ID]component.Config{ - component.MustNewID("zpages"): &zpagesextension.Config{ - ServerConfig: confighttp.ServerConfig{Endpoint: zpagesAddr}, - }, - } - set.ExtensionsFactories = map[component.Type]extension.Factory{component.MustNewType("zpages"): zpagesextension.NewFactory()} - set.LoggingOptions = []zap.Option{zap.Hooks(hook)} - - cfg := newNopConfig() - cfg.Extensions = []component.ID{component.MustNewID("zpages")} - cfg.Telemetry.Metrics.Address = metricsAddr - cfg.Telemetry.Resource = make(map[string]*string) - // Include resource attributes under the service::telemetry::resource key. - for k, v := range tc.userDefinedResource { - cfg.Telemetry.Resource[k] = v - } - - // Create a service, check for metrics, shutdown and repeat to ensure that telemetry can be started/shutdown and started again. - for i := 0; i < 2; i++ { - srv, err := New(context.Background(), set, cfg) - require.NoError(t, err) - - require.NoError(t, srv.Start(context.Background())) - // Sleep for 1 second to ensure the http server is started. - time.Sleep(1 * time.Second) - assert.True(t, loggingHookCalled) - - assertResourceLabels(t, srv.telemetrySettings.Resource, tc.expectedLabels) - assertMetrics(t, metricsAddr, tc.expectedLabels) - assertZPages(t, zpagesAddr) - require.NoError(t, srv.Shutdown(context.Background())) - } -} - -func TestServiceTelemetryWithReaders(t *testing.T) { for _, tc := range ownMetricsTestCases() { t.Run(fmt.Sprintf("ipv4_%s", tc.name), func(t *testing.T) { testCollectorStartHelperWithReaders(t, tc, "tcp4") @@ -408,7 +336,6 @@ func testCollectorStartHelperWithReaders(t *testing.T, tc ownMetricsTestCase, ne cfg := newNopConfig() cfg.Extensions = []component.ID{component.MustNewID("zpages")} - cfg.Telemetry.Metrics.Address = "" cfg.Telemetry.Metrics.Readers = []config.MetricReader{ { Pull: &config.PullMetricReader{ @@ -742,8 +669,13 @@ func newNopConfigPipelineConfigs(pipelineCfgs pipelines.ConfigWithPipelineID) Co InitialFields: map[string]any(nil), }, Metrics: telemetry.MetricsConfig{ - Level: configtelemetry.LevelBasic, - Address: "localhost:8888", + Level: configtelemetry.LevelBasic, + Readers: []config.MetricReader{{ + Pull: &config.PullMetricReader{Exporter: config.MetricExporter{Prometheus: &config.Prometheus{ + Host: newPtr("localhost"), + Port: newPtr(8888), + }}}}, + }, }, }, } @@ -775,3 +707,7 @@ func newConfigWatcherExtensionFactory(name component.Type) extension.Factory { component.StabilityLevelDevelopment, ) } + +func newPtr[T int | string](str T) *T { + return &str +} diff --git a/service/telemetry/config.go b/service/telemetry/config.go index 197adf491aa..6735d1db21d 100644 --- a/service/telemetry/config.go +++ b/service/telemetry/config.go @@ -5,14 +5,27 @@ package telemetry // import "go.opentelemetry.io/collector/service/telemetry" import ( "fmt" + "net" + "strconv" "time" "go.opentelemetry.io/contrib/config" "go.uber.org/zap/zapcore" "go.opentelemetry.io/collector/config/configtelemetry" + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/featuregate" ) +var _ confmap.Unmarshaler = (*Config)(nil) + +var disableAddressFieldForInternalTelemetryFeatureGate = featuregate.GlobalRegistry().MustRegister( + "telemetry.disableAddressFieldForInternalTelemetry", + featuregate.StageAlpha, + featuregate.WithRegisterFromVersion("v0.111.0"), + featuregate.WithRegisterToVersion("v0.114.0"), + featuregate.WithRegisterDescription("controls whether the deprecated address field for internal telemetry is still supported")) + // Config defines the configurable settings for service telemetry. type Config struct { Logs LogsConfig `mapstructure:"logs"` @@ -119,7 +132,7 @@ type MetricsConfig struct { // - "detailed" adds dimensions and views to the previous levels. Level configtelemetry.Level `mapstructure:"level"` - // Address is the [address]:port that metrics exposition should be bound to. + // Deprecated: [v0.110.0] use readers configuration. Address string `mapstructure:"address"` // Readers allow configuration of metric readers to emit metrics to @@ -143,11 +156,52 @@ type TracesConfig struct { Processors []config.SpanProcessor `mapstructure:"processors"` } +func (c *Config) Unmarshal(conf *confmap.Conf) error { + if err := conf.Unmarshal(c); err != nil { + return err + } + + // If the support for "metrics::address" is disabled, nothing to do. + // TODO: when this gate is marked stable remove the whole Unmarshal definition. + if disableAddressFieldForInternalTelemetryFeatureGate.IsEnabled() { + return nil + } + + if len(c.Metrics.Address) != 0 { + host, port, err := net.SplitHostPort(c.Metrics.Address) + if err != nil { + return fmt.Errorf("failing to parse metrics address %q: %w", c.Metrics.Address, err) + } + portInt, err := strconv.Atoi(port) + if err != nil { + return fmt.Errorf("failing to extract the port from the metrics address %q: %w", c.Metrics.Address, err) + } + + // User did not overwrite readers, so we will remove the default configured reader. + if !conf.IsSet("metrics::readers") { + c.Metrics.Readers = nil + } + + c.Metrics.Readers = append(c.Metrics.Readers, config.MetricReader{ + Pull: &config.PullMetricReader{ + Exporter: config.MetricExporter{ + Prometheus: &config.Prometheus{ + Host: &host, + Port: &portInt, + }, + }, + }, + }) + } + + return nil +} + // Validate checks whether the current configuration is valid func (c *Config) Validate() error { - // Check when service telemetry metric level is not none, the metrics address should not be empty - if c.Metrics.Level != configtelemetry.LevelNone && c.Metrics.Address == "" && len(c.Metrics.Readers) == 0 { - return fmt.Errorf("collector telemetry metric address or reader should exist when metric level is not none") + // Check when service telemetry metric level is not none, the metrics readers should not be empty + if c.Metrics.Level != configtelemetry.LevelNone && len(c.Metrics.Readers) == 0 { + return fmt.Errorf("collector telemetry metrics reader should exist when metric level is not none") } return nil diff --git a/service/telemetry/config_test.go b/service/telemetry/config_test.go index 5417347c772..4bd7c81bcaa 100644 --- a/service/telemetry/config_test.go +++ b/service/telemetry/config_test.go @@ -4,15 +4,109 @@ package telemetry import ( + "path/filepath" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.opentelemetry.io/contrib/config" + "go.opentelemetry.io/collector/component/componenttest" "go.opentelemetry.io/collector/config/configtelemetry" + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/confmaptest" + "go.opentelemetry.io/collector/featuregate" ) -func TestLoadConfig(t *testing.T) { +func TestComponentConfigStruct(t *testing.T) { + require.NoError(t, componenttest.CheckConfigStruct(NewFactory().CreateDefaultConfig())) +} + +func TestUnmarshalDefaultConfig(t *testing.T) { + factory := NewFactory() + cfg := factory.CreateDefaultConfig() + require.NoError(t, confmap.New().Unmarshal(&cfg)) + assert.Equal(t, factory.CreateDefaultConfig(), cfg) +} + +func TestUnmarshalEmptyMetricReaders(t *testing.T) { + prev := disableAddressFieldForInternalTelemetryFeatureGate.IsEnabled() + require.NoError(t, featuregate.GlobalRegistry().Set(disableAddressFieldForInternalTelemetryFeatureGate.ID(), false)) + defer func() { + // Restore previous value. + require.NoError(t, featuregate.GlobalRegistry().Set(disableAddressFieldForInternalTelemetryFeatureGate.ID(), prev)) + }() + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config_empty_readers.yaml")) + require.NoError(t, err) + cfg := NewFactory().CreateDefaultConfig() + require.NoError(t, cm.Unmarshal(&cfg)) + require.Empty(t, cfg.(*Config).Metrics.Readers) +} + +func TestUnmarshalConfigDeprecatedAddress(t *testing.T) { + prev := disableAddressFieldForInternalTelemetryFeatureGate.IsEnabled() + require.NoError(t, featuregate.GlobalRegistry().Set(disableAddressFieldForInternalTelemetryFeatureGate.ID(), false)) + defer func() { + // Restore previous value. + require.NoError(t, featuregate.GlobalRegistry().Set(disableAddressFieldForInternalTelemetryFeatureGate.ID(), prev)) + }() + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config_deprecated_address.yaml")) + require.NoError(t, err) + cfg := NewFactory().CreateDefaultConfig() + require.NoError(t, cm.Unmarshal(&cfg)) + require.Len(t, cfg.(*Config).Metrics.Readers, 1) + assert.Equal(t, "localhost", *cfg.(*Config).Metrics.Readers[0].Pull.Exporter.Prometheus.Host) + assert.Equal(t, 6666, *cfg.(*Config).Metrics.Readers[0].Pull.Exporter.Prometheus.Port) +} + +func TestUnmarshalConfigDeprecatedAddressGateEnabled(t *testing.T) { + prev := disableAddressFieldForInternalTelemetryFeatureGate.IsEnabled() + require.NoError(t, featuregate.GlobalRegistry().Set(disableAddressFieldForInternalTelemetryFeatureGate.ID(), true)) + defer func() { + // Restore previous value. + require.NoError(t, featuregate.GlobalRegistry().Set(disableAddressFieldForInternalTelemetryFeatureGate.ID(), prev)) + }() + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config_deprecated_address.yaml")) + require.NoError(t, err) + cfg := NewFactory().CreateDefaultConfig() + require.NoError(t, cm.Unmarshal(&cfg)) + require.Len(t, cfg.(*Config).Metrics.Readers, 1) + assert.Equal(t, "", *cfg.(*Config).Metrics.Readers[0].Pull.Exporter.Prometheus.Host) + assert.Equal(t, 8888, *cfg.(*Config).Metrics.Readers[0].Pull.Exporter.Prometheus.Port) +} + +func TestUnmarshalConfigInvalidDeprecatedAddress(t *testing.T) { + prev := disableAddressFieldForInternalTelemetryFeatureGate.IsEnabled() + require.NoError(t, featuregate.GlobalRegistry().Set(disableAddressFieldForInternalTelemetryFeatureGate.ID(), false)) + defer func() { + // Restore previous value. + require.NoError(t, featuregate.GlobalRegistry().Set(disableAddressFieldForInternalTelemetryFeatureGate.ID(), prev)) + }() + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config_invalid_deprecated_address.yaml")) + require.NoError(t, err) + cfg := NewFactory().CreateDefaultConfig() + require.Error(t, cm.Unmarshal(&cfg)) +} + +func TestUnmarshalConfigDeprecatedAddressAndReaders(t *testing.T) { + prev := disableAddressFieldForInternalTelemetryFeatureGate.IsEnabled() + require.NoError(t, featuregate.GlobalRegistry().Set(disableAddressFieldForInternalTelemetryFeatureGate.ID(), false)) + defer func() { + // Restore previous value. + require.NoError(t, featuregate.GlobalRegistry().Set(disableAddressFieldForInternalTelemetryFeatureGate.ID(), prev)) + }() + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config_deprecated_address_and_readers.yaml")) + require.NoError(t, err) + cfg := NewFactory().CreateDefaultConfig() + require.NoError(t, cm.Unmarshal(&cfg)) + require.Len(t, cfg.(*Config).Metrics.Readers, 2) + assert.Equal(t, "localhost", *cfg.(*Config).Metrics.Readers[0].Pull.Exporter.Prometheus.Host) + assert.Equal(t, 9999, *cfg.(*Config).Metrics.Readers[0].Pull.Exporter.Prometheus.Port) + assert.Equal(t, "192.168.0.1", *cfg.(*Config).Metrics.Readers[1].Pull.Exporter.Prometheus.Host) + assert.Equal(t, 6666, *cfg.(*Config).Metrics.Readers[1].Pull.Exporter.Prometheus.Port) +} + +func TestConfigValidate(t *testing.T) { tests := []struct { name string cfg *Config @@ -22,8 +116,13 @@ func TestLoadConfig(t *testing.T) { name: "basic metric telemetry", cfg: &Config{ Metrics: MetricsConfig{ - Level: configtelemetry.LevelBasic, - Address: "127.0.0.1:3333", + Level: configtelemetry.LevelBasic, + Readers: []config.MetricReader{{ + Pull: &config.PullMetricReader{Exporter: config.MetricExporter{Prometheus: &config.Prometheus{ + Host: newPtr("127.0.0.1"), + Port: newPtr(3333), + }}}}, + }, }, }, success: true, @@ -33,32 +132,20 @@ func TestLoadConfig(t *testing.T) { cfg: &Config{ Metrics: MetricsConfig{ Level: configtelemetry.LevelBasic, - Address: "", + Readers: nil, }, }, success: false, }, - { - name: "valid metric telemetry with metric readers", - cfg: &Config{ - Metrics: MetricsConfig{ - Level: configtelemetry.LevelBasic, - Readers: []config.MetricReader{ - {Pull: &config.PullMetricReader{}}, - }, - }, - }, - success: true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { err := tt.cfg.Validate() if tt.success { - assert.NoError(t, err) + require.NoError(t, err) } else { - assert.Error(t, err) + require.Error(t, err) } }) } diff --git a/service/telemetry/factory.go b/service/telemetry/factory.go index 474c245c22d..24802271e31 100644 --- a/service/telemetry/factory.go +++ b/service/telemetry/factory.go @@ -7,6 +7,7 @@ import ( "context" "time" + "go.opentelemetry.io/contrib/config" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/trace" "go.uber.org/zap" @@ -99,8 +100,17 @@ func createDefaultConfig() component.Config { InitialFields: map[string]any(nil), }, Metrics: MetricsConfig{ - Level: configtelemetry.LevelNormal, - Address: ":8888", + Level: configtelemetry.LevelNormal, + Readers: []config.MetricReader{{ + Pull: &config.PullMetricReader{Exporter: config.MetricExporter{Prometheus: &config.Prometheus{ + Host: newPtr(""), + Port: newPtr(8888), + }}}}, + }, }, } } + +func newPtr[T int | string](str T) *T { + return &str +} diff --git a/service/telemetry/factory_test.go b/service/telemetry/factory_test.go index f777f8dbbdc..75f4196c6ce 100644 --- a/service/telemetry/factory_test.go +++ b/service/telemetry/factory_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/contrib/config" "go.uber.org/zap" "go.uber.org/zap/zapcore" @@ -30,8 +31,13 @@ func TestTelemetryConfiguration(t *testing.T) { Encoding: "console", }, Metrics: MetricsConfig{ - Level: configtelemetry.LevelBasic, - Address: "127.0.0.1:3333", + Level: configtelemetry.LevelBasic, + Readers: []config.MetricReader{{ + Pull: &config.PullMetricReader{Exporter: config.MetricExporter{Prometheus: &config.Prometheus{ + Host: newPtr("127.0.0.1"), + Port: newPtr(3333), + }}}}, + }, }, }, success: true, @@ -43,8 +49,13 @@ func TestTelemetryConfiguration(t *testing.T) { Level: zapcore.DebugLevel, }, Metrics: MetricsConfig{ - Level: configtelemetry.LevelBasic, - Address: "127.0.0.1:3333", + Level: configtelemetry.LevelBasic, + Readers: []config.MetricReader{{ + Pull: &config.PullMetricReader{Exporter: config.MetricExporter{Prometheus: &config.Prometheus{ + Host: newPtr("127.0.0.1"), + Port: newPtr(3333), + }}}}, + }, }, }, success: false, diff --git a/service/telemetry/metrics.go b/service/telemetry/metrics.go index 76b0a758b5a..a3be9169c7c 100644 --- a/service/telemetry/metrics.go +++ b/service/telemetry/metrics.go @@ -5,12 +5,9 @@ package telemetry // import "go.opentelemetry.io/collector/service/telemetry" import ( "context" - "net" "net/http" - "strconv" "sync" - "go.opentelemetry.io/contrib/config" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/noop" sdkmetric "go.opentelemetry.io/otel/sdk/metric" @@ -40,34 +37,10 @@ type meterProviderSettings struct { } func newMeterProvider(set meterProviderSettings, disableHighCardinality bool) (metric.MeterProvider, error) { - if set.cfg.Level == configtelemetry.LevelNone || (set.cfg.Address == "" && len(set.cfg.Readers) == 0) { + if set.cfg.Level == configtelemetry.LevelNone || len(set.cfg.Readers) == 0 { return noop.NewMeterProvider(), nil } - if len(set.cfg.Address) != 0 { - host, port, err := net.SplitHostPort(set.cfg.Address) - if err != nil { - return nil, err - } - portInt, err := strconv.Atoi(port) - if err != nil { - return nil, err - } - if set.cfg.Readers == nil { - set.cfg.Readers = []config.MetricReader{} - } - set.cfg.Readers = append(set.cfg.Readers, config.MetricReader{ - Pull: &config.PullMetricReader{ - Exporter: config.MetricExporter{ - Prometheus: &config.Prometheus{ - Host: &host, - Port: &portInt, - }, - }, - }, - }) - } - mp := &meterProvider{} var opts []sdkmetric.Option for _, reader := range set.cfg.Readers { diff --git a/service/telemetry/metrics_test.go b/service/telemetry/metrics_test.go index f3bb140b1f3..3446ad219d6 100644 --- a/service/telemetry/metrics_test.go +++ b/service/telemetry/metrics_test.go @@ -17,7 +17,6 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/config/configtelemetry" - "go.opentelemetry.io/collector/internal/testutil" semconv "go.opentelemetry.io/collector/semconv/v1.18.0" "go.opentelemetry.io/collector/service/internal/promtest" "go.opentelemetry.io/collector/service/internal/resource" @@ -208,8 +207,10 @@ func TestTelemetryInit(t *testing.T) { semconv.AttributeServiceInstanceID: &testInstanceID, }, Metrics: MetricsConfig{ - Level: configtelemetry.LevelDetailed, - Address: testutil.GetAvailableLocalAddress(t), + Level: configtelemetry.LevelDetailed, + Readers: []config.MetricReader{{ + Pull: &config.PullMetricReader{Exporter: config.MetricExporter{Prometheus: promtest.GetAvailableLocalAddressPrometheus(t)}}, + }}, }, } } @@ -276,5 +277,4 @@ func getMetricsFromPrometheus(t *testing.T, handler http.Handler) map[string]*io require.NoError(t, err) return parsed - } diff --git a/service/telemetry/testdata/config_deprecated_address.yaml b/service/telemetry/testdata/config_deprecated_address.yaml new file mode 100644 index 00000000000..abb68a70ad2 --- /dev/null +++ b/service/telemetry/testdata/config_deprecated_address.yaml @@ -0,0 +1,5 @@ +logs: + level: "info" +metrics: + level: "basic" + address: "localhost:6666" diff --git a/service/telemetry/testdata/config_deprecated_address_and_readers.yaml b/service/telemetry/testdata/config_deprecated_address_and_readers.yaml new file mode 100644 index 00000000000..987b9f5a7d6 --- /dev/null +++ b/service/telemetry/testdata/config_deprecated_address_and_readers.yaml @@ -0,0 +1,11 @@ +logs: + level: "info" +metrics: + level: "basic" + address: "192.168.0.1:6666" + readers: + - pull: + exporter: + prometheus: + host: "localhost" + port: 9999 diff --git a/service/telemetry/testdata/config_empty_readers.yaml b/service/telemetry/testdata/config_empty_readers.yaml new file mode 100644 index 00000000000..9c2fb6bd7b9 --- /dev/null +++ b/service/telemetry/testdata/config_empty_readers.yaml @@ -0,0 +1,5 @@ +logs: + level: "info" +metrics: + level: "basic" + readers: [] diff --git a/service/telemetry/testdata/config_invalid_deprecated_address.yaml b/service/telemetry/testdata/config_invalid_deprecated_address.yaml new file mode 100644 index 00000000000..fd81b91ad19 --- /dev/null +++ b/service/telemetry/testdata/config_invalid_deprecated_address.yaml @@ -0,0 +1,5 @@ +logs: + level: "info" +metrics: + level: "basic" + address: "1212:212121:2121"