From d1c4cee765e2515bdb62b6a39df23f194fa95e93 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Tue, 16 Apr 2024 12:51:56 -0400 Subject: [PATCH] Replace pulling metrics port with using direct config (#2856) * checkpoint: with working tests, minimal * chlog * issue * missing change --- .chloggen/2603-part-one.yaml | 16 ++++ apis/v1beta1/config.go | 77 +++++++++++++++---- apis/v1beta1/config_test.go | 26 +++++++ apis/v1beta1/zz_generated.deepcopy.go | 47 +++++++++++ .../collector/adapters/config_to_ports.go | 28 ++----- .../adapters/config_to_ports_test.go | 69 ++++++++--------- internal/manifests/collector/container.go | 8 +- internal/manifests/collector/service.go | 13 +--- 8 files changed, 193 insertions(+), 91 deletions(-) create mode 100755 .chloggen/2603-part-one.yaml diff --git a/.chloggen/2603-part-one.yaml b/.chloggen/2603-part-one.yaml new file mode 100755 index 0000000000..1ed85a6a45 --- /dev/null +++ b/.chloggen/2603-part-one.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action) +component: collector + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Changes metric port logic to use intermediary struct. + +# One or more tracking issues related to the change +issues: [2603] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: diff --git a/apis/v1beta1/config.go b/apis/v1beta1/config.go index a34a8cacfd..04695cefa9 100644 --- a/apis/v1beta1/config.go +++ b/apis/v1beta1/config.go @@ -30,10 +30,10 @@ type AnyConfig struct { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *AnyConfig) DeepCopyInto(out *AnyConfig) { - *out = *in - if in.Object != nil { - in, out := &in.Object, &out.Object +func (c *AnyConfig) DeepCopyInto(out *AnyConfig) { + *out = *c + if c.Object != nil { + in, out := &c.Object, &out.Object *out = make(map[string]interface{}, len(*in)) for key, val := range *in { (*out)[key] = val @@ -42,12 +42,12 @@ func (in *AnyConfig) DeepCopyInto(out *AnyConfig) { } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AnyConfig. -func (in *AnyConfig) DeepCopy() *AnyConfig { - if in == nil { +func (c *AnyConfig) DeepCopy() *AnyConfig { + if c == nil { return nil } out := new(AnyConfig) - in.DeepCopyInto(out) + c.DeepCopyInto(out) return out } @@ -88,7 +88,7 @@ type Config struct { } // Yaml encodes the current object and returns it as a string. -func (c Config) Yaml() (string, error) { +func (c *Config) Yaml() (string, error) { var buf bytes.Buffer yamlEncoder := yaml.NewEncoder(&buf) yamlEncoder.SetIndent(2) @@ -98,16 +98,8 @@ func (c Config) Yaml() (string, error) { return buf.String(), nil } -type Service struct { - Extensions *[]string `json:"extensions,omitempty" yaml:"extensions,omitempty"` - // +kubebuilder:pruning:PreserveUnknownFields - Telemetry *AnyConfig `json:"telemetry,omitempty" yaml:"telemetry,omitempty"` - // +kubebuilder:pruning:PreserveUnknownFields - Pipelines AnyConfig `json:"pipelines" yaml:"pipelines"` -} - // Returns null objects in the config. -func (c Config) nullObjects() []string { +func (c *Config) nullObjects() []string { var nullKeys []string if nulls := hasNullValue(c.Receivers.Object); len(nulls) > 0 { nullKeys = append(nullKeys, addPrefix("receivers.", nulls)...) @@ -135,6 +127,57 @@ func (c Config) nullObjects() []string { return nullKeys } +type Service struct { + Extensions *[]string `json:"extensions,omitempty" yaml:"extensions,omitempty"` + // +kubebuilder:pruning:PreserveUnknownFields + Telemetry *AnyConfig `json:"telemetry,omitempty" yaml:"telemetry,omitempty"` + // +kubebuilder:pruning:PreserveUnknownFields + Pipelines AnyConfig `json:"pipelines" yaml:"pipelines"` +} + +// MetricsConfig comes from the collector. +type MetricsConfig struct { + // Level is the level of telemetry metrics, the possible values are: + // - "none" indicates that no telemetry data should be collected; + // - "basic" is the recommended and covers the basics of the service telemetry. + // - "normal" adds some other indicators on top of basic. + // - "detailed" adds dimensions and views to the previous levels. + Level string `json:"level,omitempty" yaml:"level,omitempty"` + + // Address is the [address]:port that metrics exposition should be bound to. + Address string `json:"address,omitempty" yaml:"address,omitempty"` +} + +// Telemetry is an intermediary type that allows for easy access to the collector's telemetry settings. +type Telemetry struct { + Metrics MetricsConfig `json:"metrics,omitempty" yaml:"metrics,omitempty"` + + // Resource specifies user-defined attributes to include with all emitted telemetry. + // Note that some attributes are added automatically (e.g. service.version) even + // if they are not specified here. In order to suppress such attributes the + // attribute must be specified in this map with null YAML value (nil string pointer). + Resource map[string]*string `json:"resource,omitempty" yaml:"resource,omitempty"` +} + +// GetTelemetry serves as a helper function to access the fields we care about in the underlying telemetry struct. +// This exists to avoid needing to worry extra fields in the telemetry struct. +func (s *Service) GetTelemetry() *Telemetry { + if s.Telemetry == nil { + return nil + } + // Convert map to JSON bytes + jsonData, err := json.Marshal(s.Telemetry) + if err != nil { + return nil + } + t := &Telemetry{} + // Unmarshal JSON into the provided struct + if err := json.Unmarshal(jsonData, t); err != nil { + return nil + } + return t +} + func hasNullValue(cfg map[string]interface{}) []string { var nullKeys []string for k, v := range cfg { diff --git a/apis/v1beta1/config_test.go b/apis/v1beta1/config_test.go index ac067f9ea1..207a61133a 100644 --- a/apis/v1beta1/config_test.go +++ b/apis/v1beta1/config_test.go @@ -184,3 +184,29 @@ service: assert.Equal(t, expected, yamlCollector) } + +func TestGetTelemetryFromYAML(t *testing.T) { + collectorYaml, err := os.ReadFile("./testdata/otelcol-demo.yaml") + require.NoError(t, err) + + cfg := &Config{} + err = go_yaml.Unmarshal(collectorYaml, cfg) + require.NoError(t, err) + telemetry := &Telemetry{ + Metrics: MetricsConfig{ + Level: "detailed", + Address: "0.0.0.0:8888", + }, + } + assert.Equal(t, telemetry, cfg.Service.GetTelemetry()) +} + +func TestGetTelemetryFromYAMLIsNil(t *testing.T) { + collectorYaml, err := os.ReadFile("./testdata/otelcol-couchbase.yaml") + require.NoError(t, err) + + cfg := &Config{} + err = go_yaml.Unmarshal(collectorYaml, cfg) + require.NoError(t, err) + assert.Nil(t, cfg.Service.GetTelemetry()) +} diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index e109e8ff7a..80cddf7dd0 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -174,6 +174,21 @@ func (in *MetricSpec) DeepCopy() *MetricSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *MetricsConfig) DeepCopyInto(out *MetricsConfig) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new MetricsConfig. +func (in *MetricsConfig) DeepCopy() *MetricsConfig { + if in == nil { + return nil + } + out := new(MetricsConfig) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *MetricsConfigSpec) DeepCopyInto(out *MetricsConfigSpec) { *out = *in @@ -804,3 +819,35 @@ func (in *TargetAllocatorStatus) DeepCopy() *TargetAllocatorStatus { in.DeepCopyInto(out) return out } + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Telemetry) DeepCopyInto(out *Telemetry) { + *out = *in + out.Metrics = in.Metrics + if in.Resource != nil { + in, out := &in.Resource, &out.Resource + *out = make(map[string]*string, len(*in)) + for key, val := range *in { + var outVal *string + if val == nil { + (*out)[key] = nil + } else { + inVal := (*in)[key] + in, out := &inVal, &outVal + *out = new(string) + **out = **in + } + (*out)[key] = outVal + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Telemetry. +func (in *Telemetry) DeepCopy() *Telemetry { + if in == nil { + return nil + } + out := new(Telemetry) + in.DeepCopyInto(out) + return out +} diff --git a/internal/manifests/collector/adapters/config_to_ports.go b/internal/manifests/collector/adapters/config_to_ports.go index 3a0bc5a6fd..0a8b183d4f 100644 --- a/internal/manifests/collector/adapters/config_to_ports.go +++ b/internal/manifests/collector/adapters/config_to_ports.go @@ -22,7 +22,6 @@ import ( "strings" "github.com/go-logr/logr" - "github.com/mitchellh/mapstructure" corev1 "k8s.io/api/core/v1" "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" @@ -153,29 +152,12 @@ func ConfigToPorts(logger logr.Logger, config map[interface{}]interface{}) ([]v1 } // ConfigToMetricsPort gets the port number for the metrics endpoint from the collector config if it has been set. -func ConfigToMetricsPort(logger logr.Logger, config map[interface{}]interface{}) (int32, error) { - // we don't need to unmarshal the whole config, just follow the keys down to - // the metrics address. - type metricsCfg struct { - Address string - } - type telemetryCfg struct { - Metrics metricsCfg - } - type serviceCfg struct { - Telemetry telemetryCfg - } - type cfg struct { - Service serviceCfg - } - - var cOut cfg - err := mapstructure.Decode(config, &cOut) - if err != nil { - return 0, err +func ConfigToMetricsPort(config v1beta1.Service) (int32, error) { + if config.GetTelemetry() == nil { + // telemetry isn't set, use the default + return 8888, nil } - - _, port, netErr := net.SplitHostPort(cOut.Service.Telemetry.Metrics.Address) + _, port, netErr := net.SplitHostPort(config.GetTelemetry().Metrics.Address) if netErr != nil && strings.Contains(netErr.Error(), "missing port in address") { return 8888, nil } else if netErr != nil { diff --git a/internal/manifests/collector/adapters/config_to_ports_test.go b/internal/manifests/collector/adapters/config_to_ports_test.go index d76091379c..160d998487 100644 --- a/internal/manifests/collector/adapters/config_to_ports_test.go +++ b/internal/manifests/collector/adapters/config_to_ports_test.go @@ -25,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" logf "sigs.k8s.io/controller-runtime/pkg/log" + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/adapters" "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/parser" "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/parser/receiver" @@ -210,34 +211,33 @@ func TestParserFailed(t *testing.T) { assert.NoError(t, err) assert.True(t, mockParserCalled) } - func TestConfigToMetricsPort(t *testing.T) { - t.Run("custom port specified", func(t *testing.T) { - config := map[interface{}]interface{}{ - "service": map[interface{}]interface{}{ - "telemetry": map[interface{}]interface{}{ - "metrics": map[interface{}]interface{}{ - "address": "0.0.0.0:9090", - }, - }, - }, - } - - port, err := adapters.ConfigToMetricsPort(logger, config) - assert.NoError(t, err) - assert.Equal(t, int32(9090), port) - }) for _, tt := range []struct { - desc string - config map[interface{}]interface{} + desc string + expectedPort int32 + config v1beta1.Service }{ + { + "custom port", + 9090, + v1beta1.Service{ + Telemetry: &v1beta1.AnyConfig{ + Object: map[string]interface{}{ + "metrics": map[string]interface{}{ + "address": "0.0.0.0:9090", + }, + }, + }, + }, + }, { "bad address", - map[interface{}]interface{}{ - "service": map[interface{}]interface{}{ - "telemetry": map[interface{}]interface{}{ - "metrics": map[interface{}]interface{}{ + 8888, + v1beta1.Service{ + Telemetry: &v1beta1.AnyConfig{ + Object: map[string]interface{}{ + "metrics": map[string]interface{}{ "address": "0.0.0.0", }, }, @@ -246,10 +246,11 @@ func TestConfigToMetricsPort(t *testing.T) { }, { "missing address", - map[interface{}]interface{}{ - "service": map[interface{}]interface{}{ - "telemetry": map[interface{}]interface{}{ - "metrics": map[interface{}]interface{}{ + 8888, + v1beta1.Service{ + Telemetry: &v1beta1.AnyConfig{ + Object: map[string]interface{}{ + "metrics": map[string]interface{}{ "level": "detailed", }, }, @@ -258,24 +259,22 @@ func TestConfigToMetricsPort(t *testing.T) { }, { "missing metrics", - map[interface{}]interface{}{ - "service": map[interface{}]interface{}{ - "telemetry": map[interface{}]interface{}{}, - }, + 8888, + v1beta1.Service{ + Telemetry: &v1beta1.AnyConfig{}, }, }, { "missing telemetry", - map[interface{}]interface{}{ - "service": map[interface{}]interface{}{}, - }, + 8888, + v1beta1.Service{}, }, } { t.Run(tt.desc, func(t *testing.T) { // these are acceptable failures, we return to the collector's default metric port - port, err := adapters.ConfigToMetricsPort(logger, tt.config) + port, err := adapters.ConfigToMetricsPort(tt.config) assert.NoError(t, err) - assert.Equal(t, int32(8888), port) + assert.Equal(t, tt.expectedPort, port) }) } } diff --git a/internal/manifests/collector/container.go b/internal/manifests/collector/container.go index 1ffd81cbe0..4f5c1b5ef6 100644 --- a/internal/manifests/collector/container.go +++ b/internal/manifests/collector/container.go @@ -49,7 +49,7 @@ func Container(cfg config.Config, logger logr.Logger, otelcol v1beta1.OpenTeleme } // build container ports from service ports - ports, err := getConfigContainerPorts(logger, configYaml) + ports, err := getConfigContainerPorts(logger, configYaml, otelcol.Spec.Config) if err != nil { logger.Error(err, "container ports config") } @@ -169,9 +169,9 @@ func Container(cfg config.Config, logger logr.Logger, otelcol v1beta1.OpenTeleme } } -func getConfigContainerPorts(logger logr.Logger, cfg string) (map[string]corev1.ContainerPort, error) { +func getConfigContainerPorts(logger logr.Logger, cfgYaml string, conf v1beta1.Config) (map[string]corev1.ContainerPort, error) { ports := map[string]corev1.ContainerPort{} - c, err := adapters.ConfigFromString(cfg) + c, err := adapters.ConfigFromString(cfgYaml) if err != nil { logger.Error(err, "couldn't extract the configuration") return ports, err @@ -202,7 +202,7 @@ func getConfigContainerPorts(logger logr.Logger, cfg string) (map[string]corev1. } } - metricsPort, err := adapters.ConfigToMetricsPort(logger, c) + metricsPort, err := adapters.ConfigToMetricsPort(conf.Service) if err != nil { logger.Info("couldn't determine metrics port from configuration, using 8888 default value", "error", err) metricsPort = 8888 diff --git a/internal/manifests/collector/service.go b/internal/manifests/collector/service.go index 7b834bc751..7306943402 100644 --- a/internal/manifests/collector/service.go +++ b/internal/manifests/collector/service.go @@ -64,18 +64,7 @@ func MonitoringService(params manifests.Params) (*corev1.Service, error) { labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{}) labels[monitoringLabel] = valueExists - out, err := params.OtelCol.Spec.Config.Yaml() - if err != nil { - return nil, err - } - - c, err := adapters.ConfigFromString(out) - if err != nil { - params.Log.Error(err, "couldn't extract the configuration") - return nil, err - } - - metricsPort, err := adapters.ConfigToMetricsPort(params.Log, c) + metricsPort, err := adapters.ConfigToMetricsPort(params.OtelCol.Spec.Config.Service) if err != nil { return nil, err }