From b1e075f507d1be98d24c3872d5bf0d6c49fb703d Mon Sep 17 00:00:00 2001 From: Dmitrii Anoshin Date: Fri, 12 Apr 2024 15:11:31 -0700 Subject: [PATCH] [cmd/mdatagen] [chore] Small fixes for new resource attributes filter (#9950) - Make sure we always pass a string to the filter.Match even if the attribute value has a different type. Otherwise, it panics. - Make sure we show the if_configured warning if the user sets include/exclude without enabled. - Simplify generated tests Follow up to https://github.com/open-telemetry/opentelemetry-collector/pull/9660 --- .../internal/metadata/generated_metrics.go | 18 +++--- .../metadata/generated_metrics_test.go | 59 ++++++++++--------- .../internal/metadata/testdata/config.yaml | 46 +++------------ cmd/mdatagen/templates/config.go.tmpl | 14 +++-- cmd/mdatagen/templates/metrics.go.tmpl | 18 +++--- cmd/mdatagen/templates/metrics_test.go.tmpl | 57 +++++++++--------- .../templates/testdata/config.yaml.tmpl | 30 ++-------- 7 files changed, 99 insertions(+), 143 deletions(-) diff --git a/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics.go b/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics.go index c5a52864edb..6a4c7753c2f 100644 --- a/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics.go +++ b/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics.go @@ -357,7 +357,7 @@ func NewMetricsBuilder(mbc MetricsBuilderConfig, settings receiver.CreateSetting if !mbc.ResourceAttributes.StringResourceAttrDisableWarning.enabledSetByUser { settings.Logger.Warn("[WARNING] Please set `enabled` field explicitly for `string.resource.attr_disable_warning`: This resource_attribute will be disabled by default soon.") } - if mbc.ResourceAttributes.StringResourceAttrRemoveWarning.enabledSetByUser { + if mbc.ResourceAttributes.StringResourceAttrRemoveWarning.enabledSetByUser || mbc.ResourceAttributes.StringResourceAttrRemoveWarning.Include != nil || mbc.ResourceAttributes.StringResourceAttrRemoveWarning.Exclude != nil { settings.Logger.Warn("[WARNING] `string.resource.attr_remove_warning` should not be configured: This resource_attribute is deprecated and will be removed soon.") } if mbc.ResourceAttributes.StringResourceAttrToBeRemoved.Enabled { @@ -495,16 +495,14 @@ func (mb *MetricsBuilder) EmitForResource(rmo ...ResourceMetricsOption) { for _, op := range rmo { op(rm) } - for name, val := range rm.Resource().Attributes().AsRaw() { - if filter, ok := mb.resourceAttributeIncludeFilter[name]; ok { - if !filter.Matches(val) { - return - } + for attr, filter := range mb.resourceAttributeIncludeFilter { + if val, ok := rm.Resource().Attributes().Get(attr); ok && !filter.Matches(val.AsString()) { + return } - if filter, ok := mb.resourceAttributeExcludeFilter[name]; ok { - if filter.Matches(val) { - return - } + } + for attr, filter := range mb.resourceAttributeExcludeFilter { + if val, ok := rm.Resource().Attributes().Get(attr); ok && filter.Matches(val.AsString()) { + return } } diff --git a/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics_test.go b/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics_test.go index 89e34908623..6860fe3c866 100644 --- a/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics_test.go +++ b/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics_test.go @@ -14,40 +14,43 @@ import ( "go.opentelemetry.io/collector/receiver/receivertest" ) -type testConfigCollection int +type metricsSet int const ( - testSetDefault testConfigCollection = iota - testSetAll - testSetNone - testSetFilterInclude - testSetFilterExclude + metricsSetDefault metricsSet = iota + metricsSetAll + metricsSetNone ) func TestMetricsBuilder(t *testing.T) { tests := []struct { - name string - configSet testConfigCollection + name string + metricsSet metricsSet + resAttrsConfigured bool + expectEmpty bool }{ { - name: "default", - configSet: testSetDefault, + name: "default", }, { - name: "all_set", - configSet: testSetAll, + name: "all_set", + metricsSet: metricsSetAll, + resAttrsConfigured: true, }, { - name: "none_set", - configSet: testSetNone, + name: "none_set", + metricsSet: metricsSetNone, + resAttrsConfigured: true, + expectEmpty: true, }, { - name: "filter_set_include", - configSet: testSetFilterInclude, + name: "filter_set_include", + resAttrsConfigured: true, }, { - name: "filter_set_exclude", - configSet: testSetFilterExclude, + name: "filter_set_exclude", + resAttrsConfigured: true, + expectEmpty: true, }, } for _, test := range tests { @@ -60,31 +63,31 @@ func TestMetricsBuilder(t *testing.T) { mb := NewMetricsBuilder(loadMetricsBuilderConfig(t, test.name), settings, WithStartTime(start)) expectedWarnings := 0 - if test.configSet == testSetDefault { + if test.metricsSet == metricsSetDefault { assert.Equal(t, "[WARNING] Please set `enabled` field explicitly for `default.metric`: This metric will be disabled by default soon.", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } - if test.configSet == testSetDefault || test.configSet == testSetAll || test.configSet == testSetFilterInclude || test.configSet == testSetFilterExclude { + if test.metricsSet == metricsSetDefault || test.metricsSet == metricsSetAll { assert.Equal(t, "[WARNING] `default.metric.to_be_removed` should not be enabled: This metric is deprecated and will be removed soon.", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } - if test.configSet == testSetAll || test.configSet == testSetNone || test.configSet == testSetFilterInclude || test.configSet == testSetFilterExclude { + if test.metricsSet == metricsSetAll || test.metricsSet == metricsSetNone { assert.Equal(t, "[WARNING] `optional.metric` should not be configured: This metric is deprecated and will be removed soon.", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } - if test.configSet == testSetAll || test.configSet == testSetNone || test.configSet == testSetFilterInclude || test.configSet == testSetFilterExclude { + if test.metricsSet == metricsSetAll || test.metricsSet == metricsSetNone { assert.Equal(t, "[WARNING] `optional.metric.empty_unit` should not be configured: This metric is deprecated and will be removed soon.", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } - if test.configSet == testSetDefault { + if test.metricsSet == metricsSetDefault { assert.Equal(t, "[WARNING] Please set `enabled` field explicitly for `string.resource.attr_disable_warning`: This resource_attribute will be disabled by default soon.", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } - if test.configSet == testSetAll || test.configSet == testSetNone || test.configSet == testSetFilterInclude || test.configSet == testSetFilterExclude { + if test.resAttrsConfigured { assert.Equal(t, "[WARNING] `string.resource.attr_remove_warning` should not be configured: This resource_attribute is deprecated and will be removed soon.", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } - if test.configSet == testSetDefault || test.configSet == testSetAll || test.configSet == testSetFilterInclude || test.configSet == testSetFilterExclude { + if test.metricsSet == metricsSetDefault || test.metricsSet == metricsSetAll { assert.Equal(t, "[WARNING] `string.resource.attr_to_be_removed` should not be enabled: This resource_attribute is deprecated and will be removed soon.", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } @@ -124,7 +127,7 @@ func TestMetricsBuilder(t *testing.T) { res := rb.Emit() metrics := mb.Emit(WithResource(res)) - if test.configSet == testSetNone || test.configSet == testSetFilterExclude { + if test.expectEmpty { assert.Equal(t, 0, metrics.ResourceMetrics().Len()) return } @@ -134,10 +137,10 @@ func TestMetricsBuilder(t *testing.T) { assert.Equal(t, res, rm.Resource()) assert.Equal(t, 1, rm.ScopeMetrics().Len()) ms := rm.ScopeMetrics().At(0).Metrics() - if test.configSet == testSetDefault { + if test.metricsSet == metricsSetDefault { assert.Equal(t, defaultMetricsCount, ms.Len()) } - if test.configSet == testSetAll { + if test.metricsSet == metricsSetAll { assert.Equal(t, allMetricsCount, ms.Len()) } validatedMetrics := make(map[string]bool) diff --git a/cmd/mdatagen/internal/samplereceiver/internal/metadata/testdata/config.yaml b/cmd/mdatagen/internal/samplereceiver/internal/metadata/testdata/config.yaml index 3d437f4302f..24a348ce4b5 100644 --- a/cmd/mdatagen/internal/samplereceiver/internal/metadata/testdata/config.yaml +++ b/cmd/mdatagen/internal/samplereceiver/internal/metadata/testdata/config.yaml @@ -58,84 +58,54 @@ none_set: string.resource.attr_to_be_removed: enabled: false filter_set_include: - metrics: - default.metric: - enabled: true - default.metric.to_be_removed: - enabled: true - metric.input_type: - enabled: true - optional.metric: - enabled: true - optional.metric.empty_unit: - enabled: true resource_attributes: map.resource.attr: - enabled: true + include: + - regexp: ".*" optional.resource.attr: - enabled: true include: - regexp: ".*" slice.resource.attr: - enabled: true + include: + - regexp: ".*" string.enum.resource.attr: - enabled: true include: - regexp: ".*" string.resource.attr: - enabled: true include: - regexp: ".*" string.resource.attr_disable_warning: - enabled: true include: - regexp: ".*" string.resource.attr_remove_warning: - enabled: true include: - regexp: ".*" string.resource.attr_to_be_removed: - enabled: true include: - regexp: ".*" filter_set_exclude: - metrics: - default.metric: - enabled: true - default.metric.to_be_removed: - enabled: true - metric.input_type: - enabled: true - optional.metric: - enabled: true - optional.metric.empty_unit: - enabled: true resource_attributes: map.resource.attr: - enabled: true + exclude: + - regexp: ".*" optional.resource.attr: - enabled: true exclude: - strict: "optional.resource.attr-val" slice.resource.attr: - enabled: true + exclude: + - regexp: ".*" string.enum.resource.attr: - enabled: true exclude: - strict: "one" string.resource.attr: - enabled: true exclude: - strict: "string.resource.attr-val" string.resource.attr_disable_warning: - enabled: true exclude: - strict: "string.resource.attr_disable_warning-val" string.resource.attr_remove_warning: - enabled: true exclude: - strict: "string.resource.attr_remove_warning-val" string.resource.attr_to_be_removed: - enabled: true exclude: - strict: "string.resource.attr_to_be_removed-val" diff --git a/cmd/mdatagen/templates/config.go.tmpl b/cmd/mdatagen/templates/config.go.tmpl index 32025863d9d..e4a9f52f8fc 100644 --- a/cmd/mdatagen/templates/config.go.tmpl +++ b/cmd/mdatagen/templates/config.go.tmpl @@ -2,12 +2,12 @@ package {{ .Package }} -{{ if or .Metrics .ResourceAttributes -}} -import "go.opentelemetry.io/collector/confmap" -{{- end }} -{{ if .ResourceAttributes -}} -import "go.opentelemetry.io/collector/filter" -{{- end }} +import ( + "go.opentelemetry.io/collector/confmap" + {{ if and .Metrics .ResourceAttributes -}} + "go.opentelemetry.io/collector/filter" + {{- end }} +) {{ if .Metrics -}} // MetricConfig provides common config for a particular metric. @@ -51,8 +51,10 @@ func DefaultMetricsConfig() MetricsConfig { // ResourceAttributeConfig provides common config for a particular resource attribute. type ResourceAttributeConfig struct { Enabled bool `mapstructure:"enabled"` + {{- if .Metrics }} Include []filter.Config `mapstructure:"include"` Exclude []filter.Config `mapstructure:"exclude"` + {{- end }} enabledSetByUser bool } diff --git a/cmd/mdatagen/templates/metrics.go.tmpl b/cmd/mdatagen/templates/metrics.go.tmpl index 97d7cd5b9f6..dff435b8c7d 100644 --- a/cmd/mdatagen/templates/metrics.go.tmpl +++ b/cmd/mdatagen/templates/metrics.go.tmpl @@ -184,7 +184,7 @@ func NewMetricsBuilder(mbc MetricsBuilderConfig, settings receiver.CreateSetting } {{- end }} {{- if $attr.Warnings.IfConfigured }} - if mbc.ResourceAttributes.{{ $name.Render }}.enabledSetByUser { + if mbc.ResourceAttributes.{{ $name.Render }}.enabledSetByUser || mbc.ResourceAttributes.{{ $name.Render }}.Include != nil || mbc.ResourceAttributes.{{ $name.Render }}.Exclude != nil { settings.Logger.Warn("[WARNING] `{{ $name }}` should not be configured: {{ $attr.Warnings.IfConfigured }}") } {{- end }} @@ -284,16 +284,14 @@ func (mb *MetricsBuilder) EmitForResource(rmo ...ResourceMetricsOption) { op(rm) } {{ if .ResourceAttributes -}} - for name, val := range rm.Resource().Attributes().AsRaw() { - if filter, ok := mb.resourceAttributeIncludeFilter[name]; ok { - if !filter.Matches(val) { - return - } + for attr, filter := range mb.resourceAttributeIncludeFilter { + if val, ok := rm.Resource().Attributes().Get(attr); ok && !filter.Matches(val.AsString()) { + return } - if filter, ok := mb.resourceAttributeExcludeFilter[name]; ok { - if filter.Matches(val) { - return - } + } + for attr, filter := range mb.resourceAttributeExcludeFilter { + if val, ok := rm.Resource().Attributes().Get(attr); ok && filter.Matches(val.AsString()) { + return } } {{- end }} diff --git a/cmd/mdatagen/templates/metrics_test.go.tmpl b/cmd/mdatagen/templates/metrics_test.go.tmpl index dfce2fd97ae..bfcdc2c70d5 100644 --- a/cmd/mdatagen/templates/metrics_test.go.tmpl +++ b/cmd/mdatagen/templates/metrics_test.go.tmpl @@ -14,40 +14,43 @@ import ( ) -type testConfigCollection int +type metricsSet int const ( - testSetDefault testConfigCollection = iota - testSetAll - testSetNone - testSetFilterInclude - testSetFilterExclude + metricsSetDefault metricsSet = iota + metricsSetAll + metricsSetNone ) func TestMetricsBuilder(t *testing.T) { tests := []struct { - name string - configSet testConfigCollection + name string + metricsSet metricsSet + resAttrsConfigured bool + expectEmpty bool }{ { - name: "default", - configSet: testSetDefault, + name: "default", }, { - name: "all_set", - configSet: testSetAll, + name: "all_set", + metricsSet: metricsSetAll, + resAttrsConfigured: true, }, { - name: "none_set", - configSet: testSetNone, + name: "none_set", + metricsSet: metricsSetNone, + resAttrsConfigured: true, + expectEmpty: true, }, { - name: "filter_set_include", - configSet: testSetFilterInclude, + name: "filter_set_include", + resAttrsConfigured: true, }, { - name: "filter_set_exclude", - configSet: testSetFilterExclude, + name: "filter_set_exclude", + resAttrsConfigured: true, + expectEmpty: true, }, } for _, test := range tests { @@ -62,19 +65,19 @@ func TestMetricsBuilder(t *testing.T) { expectedWarnings := 0 {{- range $name, $metric := .Metrics }} {{- if and $metric.Enabled $metric.Warnings.IfEnabled }} - if test.configSet == testSetDefault || test.configSet == testSetAll || test.configSet == testSetFilterInclude || test.configSet == testSetFilterExclude { + if test.metricsSet == metricsSetDefault || test.metricsSet == metricsSetAll { assert.Equal(t, "[WARNING] `{{ $name }}` should not be enabled: {{ $metric.Warnings.IfEnabled }}", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } {{- end }} {{- if $metric.Warnings.IfEnabledNotSet }} - if test.configSet == testSetDefault { + if test.metricsSet == metricsSetDefault { assert.Equal(t, "[WARNING] Please set `enabled` field explicitly for `{{ $name }}`: {{ $metric.Warnings.IfEnabledNotSet }}", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } {{- end }} {{- if $metric.Warnings.IfConfigured }} - if test.configSet == testSetAll || test.configSet == testSetNone || test.configSet == testSetFilterInclude || test.configSet == testSetFilterExclude { + if test.metricsSet == metricsSetAll || test.metricsSet == metricsSetNone { assert.Equal(t, "[WARNING] `{{ $name }}` should not be configured: {{ $metric.Warnings.IfConfigured }}", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } @@ -82,19 +85,19 @@ func TestMetricsBuilder(t *testing.T) { {{- end }} {{- range $name, $attr := .ResourceAttributes }} {{- if and $attr.Enabled $attr.Warnings.IfEnabled }} - if test.configSet == testSetDefault || test.configSet == testSetAll || test.configSet == testSetFilterInclude || test.configSet == testSetFilterExclude { + if test.metricsSet == metricsSetDefault || test.metricsSet == metricsSetAll { assert.Equal(t, "[WARNING] `{{ $name }}` should not be enabled: {{ $attr.Warnings.IfEnabled }}", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } {{- end }} {{- if $attr.Warnings.IfEnabledNotSet }} - if test.configSet == testSetDefault { + if test.metricsSet == metricsSetDefault { assert.Equal(t, "[WARNING] Please set `enabled` field explicitly for `{{ $name }}`: {{ $attr.Warnings.IfEnabledNotSet }}", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } {{- end }} {{- if $attr.Warnings.IfConfigured }} - if test.configSet == testSetAll || test.configSet == testSetNone || test.configSet == testSetFilterInclude || test.configSet == testSetFilterExclude { + if test.resAttrsConfigured { assert.Equal(t, "[WARNING] `{{ $name }}` should not be configured: {{ $attr.Warnings.IfConfigured }}", observedLogs.All()[expectedWarnings].Message) expectedWarnings++ } @@ -131,7 +134,7 @@ func TestMetricsBuilder(t *testing.T) { {{- end }} metrics := mb.Emit(WithResource(res)) - if test.configSet == testSetNone || test.configSet == testSetFilterExclude { + if test.expectEmpty { assert.Equal(t, 0, metrics.ResourceMetrics().Len()) return } @@ -141,10 +144,10 @@ func TestMetricsBuilder(t *testing.T) { assert.Equal(t, res, rm.Resource()) assert.Equal(t, 1, rm.ScopeMetrics().Len()) ms := rm.ScopeMetrics().At(0).Metrics() - if test.configSet == testSetDefault { + if test.metricsSet == metricsSetDefault { assert.Equal(t, defaultMetricsCount, ms.Len()) } - if test.configSet == testSetAll { + if test.metricsSet == metricsSetAll { assert.Equal(t, allMetricsCount, ms.Len()) } validatedMetrics := make(map[string]bool) diff --git a/cmd/mdatagen/templates/testdata/config.yaml.tmpl b/cmd/mdatagen/templates/testdata/config.yaml.tmpl index e35f6a88894..862aa0aaf1f 100644 --- a/cmd/mdatagen/templates/testdata/config.yaml.tmpl +++ b/cmd/mdatagen/templates/testdata/config.yaml.tmpl @@ -29,41 +29,23 @@ none_set: enabled: false {{- end }} {{- end }} +{{- if and .Metrics .ResourceAttributes }} filter_set_include: - {{- if .Metrics }} - metrics: - {{- range $name, $_ := .Metrics }} - {{ $name }}: - enabled: true - {{- end }} - {{- end }} - {{- if .ResourceAttributes }} resource_attributes: {{- range $name, $attr := .ResourceAttributes }} {{ $name }}: - enabled: true - {{- if eq $attr.Type.String "Str" }} include: - regexp: ".*" - {{- end }} {{- end }} - {{- end }} filter_set_exclude: - {{- if .Metrics }} - metrics: - {{- range $name, $_ := .Metrics }} - {{ $name }}: - enabled: true - {{- end }} - {{- end }} - {{- if .ResourceAttributes }} resource_attributes: {{- range $name, $attr := .ResourceAttributes }} {{ $name }}: - enabled: true - {{- if eq $attr.Type.String "Str" }} exclude: + {{- if eq $attr.Type.String "Str" }} - strict: {{ $attr.TestValue }} - {{- end }} + {{- else }} + - regexp: ".*" + {{- end }} {{- end }} - {{- end }} +{{- end }}