diff --git a/.chloggen/fix-metric-validation.yaml b/.chloggen/fix-metric-validation.yaml new file mode 100644 index 0000000000000..65408e879b687 --- /dev/null +++ b/.chloggen/fix-metric-validation.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: attributesprocessor + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "Validate metrics configuration parameters before processing" + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [36077] + +# (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: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# 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: [] diff --git a/internal/filter/filterconfig/config.go b/internal/filter/filterconfig/config.go index eb0f2a53191f8..521372aa85bc3 100644 --- a/internal/filter/filterconfig/config.go +++ b/internal/filter/filterconfig/config.go @@ -126,9 +126,11 @@ type MatchProperties struct { } var ( - ErrMissingRequiredField = errors.New(`at least one of "attributes", "libraries", or "resources" field must be specified`) - ErrInvalidLogField = errors.New("services, span_names, and span_kinds are not valid for log records") - ErrMissingRequiredLogField = errors.New(`at least one of "attributes", "libraries", "span_kinds", "resources", "log_bodies", "log_severity_texts" or "log_severity_number" field must be specified`) + ErrMissingRequiredSpanField = errors.New(`at least one of "attributes", "libraries", or "resources" field must be specified`) + ErrInvalidLogField = errors.New("services, span_names, span_kinds and metric_names are not valid for log records") + ErrMissingRequiredLogField = errors.New(`at least one of "attributes", "libraries", "span_kinds", "resources", "log_bodies", "log_severity_texts" or "log_severity_number" field must be specified`) + ErrMissingRequiredMetricField = errors.New(`at least one of "metric_names" or "resources" field must be specified`) + ErrInvalidMetricField = errors.New(`"span_names", "span_kinds", "log_bodies", "log_severity_texts", "log_severity_number", "services", "attributes" and "libraries" are not valid for metrics`) spanKinds = map[string]bool{ traceutil.SpanKindStr(ptrace.SpanKindInternal): true, @@ -153,9 +155,13 @@ func (mp *MatchProperties) ValidateForSpans() error { return errors.New("log_severity_number should not be specified for trace spans") } + if len(mp.MetricNames) > 0 { + return errors.New("metric_names should not be specified for trace spans") + } + if len(mp.Services) == 0 && len(mp.SpanNames) == 0 && len(mp.Attributes) == 0 && len(mp.Libraries) == 0 && len(mp.Resources) == 0 && len(mp.SpanKinds) == 0 { - return ErrMissingRequiredField + return ErrMissingRequiredSpanField } if len(mp.SpanKinds) > 0 && mp.MatchType == "strict" { @@ -176,7 +182,7 @@ func (mp *MatchProperties) ValidateForSpans() error { // ValidateForLogs validates properties for logs. func (mp *MatchProperties) ValidateForLogs() error { - if len(mp.SpanNames) > 0 || len(mp.Services) > 0 || len(mp.SpanKinds) > 0 { + if len(mp.SpanNames) > 0 || len(mp.Services) > 0 || len(mp.SpanKinds) > 0 || len(mp.MetricNames) > 0 { return ErrInvalidLogField } @@ -190,6 +196,26 @@ func (mp *MatchProperties) ValidateForLogs() error { return nil } +// ValidateForMetrics validates properties for metrics. +func (mp *MatchProperties) ValidateForMetrics() error { + if len(mp.LogBodies) > 0 || + len(mp.LogSeverityTexts) > 0 || + len(mp.SpanNames) > 0 || + len(mp.Services) > 0 || + len(mp.SpanKinds) > 0 || + len(mp.Attributes) > 0 || + len(mp.Libraries) > 0 || + mp.LogSeverityNumber != nil { + return ErrInvalidMetricField + } + + if len(mp.MetricNames) == 0 && len(mp.Resources) == 0 { + return ErrMissingRequiredMetricField + } + + return nil +} + // Attribute specifies the attribute key and optional value to match against. type Attribute struct { // Key specifies the attribute key. @@ -261,9 +287,13 @@ type MetricMatchProperties struct { ResourceAttributes []Attribute `mapstructure:"resource_attributes"` } -func CreateMetricMatchPropertiesFromDefault(properties *MatchProperties) *MetricMatchProperties { +func CreateMetricMatchPropertiesFromDefault(properties *MatchProperties) (*MetricMatchProperties, error) { if properties == nil { - return nil + return nil, nil + } + + if err := properties.ValidateForMetrics(); err != nil { + return nil, err } return &MetricMatchProperties{ @@ -271,5 +301,5 @@ func CreateMetricMatchPropertiesFromDefault(properties *MatchProperties) *Metric RegexpConfig: properties.Config.RegexpConfig, MetricNames: properties.MetricNames, ResourceAttributes: properties.Resources, - } + }, nil } diff --git a/internal/filter/filterconfig/config_test.go b/internal/filter/filterconfig/config_test.go index 91a1e757f1e4c..af6d485966728 100644 --- a/internal/filter/filterconfig/config_test.go +++ b/internal/filter/filterconfig/config_test.go @@ -2,3 +2,297 @@ // SPDX-License-Identifier: Apache-2.0 package filterconfig + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterset" + "github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterset/regexp" +) + +func Test_ValidateWithSpans(t *testing.T) { + tests := []struct { + name string + config *MatchProperties + wantErr bool + }{ + { + name: "valid", + config: &MatchProperties{ + SpanKinds: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + }, + wantErr: false, + }, + { + name: "invalid", + config: &MatchProperties{ + SpanKinds: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + MetricNames: []string{"name"}, + }, + wantErr: true, + }, + { + name: "invalid empty", + config: &MatchProperties{}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.wantErr { + require.Error(t, tt.config.ValidateForSpans()) + } else { + require.NoError(t, tt.config.ValidateForSpans()) + } + }) + } +} + +func Test_ValidateWithMetrics(t *testing.T) { + tests := []struct { + name string + config *MatchProperties + wantErr bool + }{ + { + name: "valid", + config: &MatchProperties{ + MetricNames: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + }, + wantErr: false, + }, + { + name: "invalid", + config: &MatchProperties{ + SpanKinds: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + MetricNames: []string{"name"}, + }, + wantErr: true, + }, + { + name: "invalid empty", + config: &MatchProperties{}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.wantErr { + require.Error(t, tt.config.ValidateForMetrics()) + } else { + require.NoError(t, tt.config.ValidateForMetrics()) + } + }) + } +} + +func Test_ValidateWithLogs(t *testing.T) { + tests := []struct { + name string + config *MatchProperties + wantErr bool + }{ + { + name: "valid", + config: &MatchProperties{ + LogBodies: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + }, + wantErr: false, + }, + { + name: "invalid", + config: &MatchProperties{ + SpanKinds: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + LogBodies: []string{"name"}, + }, + wantErr: true, + }, + { + name: "invalid empty", + config: &MatchProperties{}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.wantErr { + require.Error(t, tt.config.ValidateForLogs()) + } else { + require.NoError(t, tt.config.ValidateForLogs()) + } + }) + } +} + +func Test_CreateMetricMatchPropertiesFromDefault(t *testing.T) { + tests := []struct { + name string + config *MatchProperties + want *MetricMatchProperties + wantErr bool + }{ + { + name: "nil", + config: nil, + wantErr: false, + want: nil, + }, + { + name: "invalid log bodies", + config: &MatchProperties{ + MetricNames: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + LogBodies: []string{"body"}, + }, + want: nil, + wantErr: true, + }, + { + name: "invalid log severity texts", + config: &MatchProperties{ + MetricNames: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + LogSeverityTexts: []string{"text"}, + }, + want: nil, + wantErr: true, + }, + { + name: "invalid log severity number", + config: &MatchProperties{ + MetricNames: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + LogSeverityNumber: &LogSeverityNumberMatchProperties{}, + }, + want: nil, + wantErr: true, + }, + { + name: "invalid span names", + config: &MatchProperties{ + MetricNames: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + SpanNames: []string{"text"}, + }, + want: nil, + wantErr: true, + }, + { + name: "invalid span kinds", + config: &MatchProperties{ + MetricNames: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + SpanKinds: []string{"text"}, + }, + want: nil, + wantErr: true, + }, + { + name: "invalid services", + config: &MatchProperties{ + MetricNames: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + Services: []string{"text"}, + }, + want: nil, + wantErr: true, + }, + { + name: "invalid attributes", + config: &MatchProperties{ + MetricNames: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + Attributes: []Attribute{ + {Key: "key", Value: "val"}, + }, + }, + want: nil, + wantErr: true, + }, + { + name: "invalid span kinds", + config: &MatchProperties{ + MetricNames: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + }, + Libraries: []InstrumentationLibrary{ + {Name: "name"}, + }, + }, + want: nil, + wantErr: true, + }, + { + name: "valid", + config: &MatchProperties{ + MetricNames: []string{"kind"}, + Config: filterset.Config{ + MatchType: filterset.Regexp, + RegexpConfig: ®exp.Config{ + CacheMaxNumEntries: 1, + }, + }, + }, + want: &MetricMatchProperties{ + MetricNames: []string{"kind"}, + MatchType: MetricRegexp, + RegexpConfig: ®exp.Config{ + CacheMaxNumEntries: 1, + }, + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.wantErr { + res, err := CreateMetricMatchPropertiesFromDefault(tt.config) + require.Error(t, err) + require.Nil(t, res) + } else { + res, err := CreateMetricMatchPropertiesFromDefault(tt.config) + require.NoError(t, err) + require.Equal(t, tt.want, res) + } + }) + } +} diff --git a/internal/filter/filterspan/filterspan_test.go b/internal/filter/filterspan/filterspan_test.go index b06171543c1a3..3e78b609d64f0 100644 --- a/internal/filter/filterspan/filterspan_test.go +++ b/internal/filter/filterspan/filterspan_test.go @@ -38,14 +38,14 @@ func TestSpan_validateMatchesConfiguration_InvalidConfig(t *testing.T) { { name: "empty_property", property: &filterconfig.MatchProperties{}, - errorString: filterconfig.ErrMissingRequiredField.Error(), + errorString: filterconfig.ErrMissingRequiredSpanField.Error(), }, { name: "empty_service_span_names_and_attributes", property: &filterconfig.MatchProperties{ Services: []string{}, }, - errorString: filterconfig.ErrMissingRequiredField.Error(), + errorString: filterconfig.ErrMissingRequiredSpanField.Error(), }, { name: "log_properties", diff --git a/processor/attributesprocessor/README.md b/processor/attributesprocessor/README.md index f70818718dc24..b6cacedbc1b1a 100644 --- a/processor/attributesprocessor/README.md +++ b/processor/attributesprocessor/README.md @@ -193,8 +193,8 @@ must be specified with a non-empty value for a valid configuration. The `log_bod - For logs, one of `log_bodies`, `log_severity_texts`, `log_severity_number`, `attributes`, `resources` or `libraries` must be specified with a non-empty value for a valid configuration. The `span_names`, `span_kinds`, `metric_names` and `services` fields are invalid. -- For metrics, `metric_names` must be specified with a valid non-empty value for -a valid configuration. The `span_names`, `span_kinds`, `resources`, `log_bodies`, `log_severity_texts`, +- For metrics, one of `metric_names` or `resources` must be specified with a valid non-empty value for +a valid configuration. The `span_names`, `span_kinds`, `log_bodies`, `log_severity_texts`, `log_severity_number`, `services`, `attributes` and `libraries` fields are invalid. diff --git a/processor/attributesprocessor/config_test.go b/processor/attributesprocessor/config_test.go index 5d8ed1487762b..99a536386b580 100644 --- a/processor/attributesprocessor/config_test.go +++ b/processor/attributesprocessor/config_test.go @@ -4,6 +4,7 @@ package attributesprocessor import ( + "context" "path/filepath" "testing" @@ -12,6 +13,8 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/confmap/confmaptest" "go.opentelemetry.io/collector/confmap/xconfmap" + "go.opentelemetry.io/collector/consumer/consumertest" + "go.opentelemetry.io/collector/processor/processortest" "github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal/attraction" "github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterconfig" @@ -217,3 +220,23 @@ func TestLoadConfig(t *testing.T) { }) } } + +func TestSpanConfigUsedWithmetrics(t *testing.T) { + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) + require.NoError(t, err) + + factory := NewFactory() + cfg := factory.CreateDefaultConfig() + + sub, err := cm.Sub("attributes/servicesmetrics") + + require.NoError(t, err) + require.NoError(t, sub.Unmarshal(cfg)) + + assert.NoError(t, xconfmap.Validate(cfg)) + + sink := consumertest.MetricsSink{} + + _, err = NewFactory().CreateMetrics(context.Background(), processortest.NewNopSettings(metadata.Type), cfg, &sink) + require.Error(t, err) +} diff --git a/processor/attributesprocessor/factory.go b/processor/attributesprocessor/factory.go index 5844db4744d2f..b30358ffcf204 100644 --- a/processor/attributesprocessor/factory.go +++ b/processor/attributesprocessor/factory.go @@ -98,9 +98,18 @@ func createMetricsProcessor( return nil, err } + includeMatchProperties, err := filterconfig.CreateMetricMatchPropertiesFromDefault(oCfg.Include) + if err != nil { + return nil, err + } + excludeMatchProperties, err := filterconfig.CreateMetricMatchPropertiesFromDefault(oCfg.Exclude) + if err != nil { + return nil, err + } + skipExpr, err := filtermetric.NewSkipExpr( - filterconfig.CreateMetricMatchPropertiesFromDefault(oCfg.Include), - filterconfig.CreateMetricMatchPropertiesFromDefault(oCfg.Exclude), + includeMatchProperties, + excludeMatchProperties, ) if err != nil { return nil, err diff --git a/processor/attributesprocessor/testdata/config.yaml b/processor/attributesprocessor/testdata/config.yaml index 8f0f7dbb09a8d..5979fbc213222 100644 --- a/processor/attributesprocessor/testdata/config.yaml +++ b/processor/attributesprocessor/testdata/config.yaml @@ -354,3 +354,13 @@ attributes/from_context: # for more information about which attributes are available. from_context: auth.subject action: insert + +attributes/servicesmetrics: + include: + match_type: strict + services: + - foo + actions: + - key: service.name + value: bar + action: insert diff --git a/processor/filterprocessor/metrics_test.go b/processor/filterprocessor/metrics_test.go index a5bbef2474cfd..422261d2a3a2c 100644 --- a/processor/filterprocessor/metrics_test.go +++ b/processor/filterprocessor/metrics_test.go @@ -1068,7 +1068,12 @@ func Test_ResourceSkipExpr_With_Bridge(t *testing.T) { tCtx := ottlresource.NewTransformContext(resource, pmetric.NewResourceMetrics()) - boolExpr, err := newSkipResExpr(filterconfig.CreateMetricMatchPropertiesFromDefault(tt.condition.Include), filterconfig.CreateMetricMatchPropertiesFromDefault(tt.condition.Exclude)) + includeMatchProperties, err := filterconfig.CreateMetricMatchPropertiesFromDefault(tt.condition.Include) + assert.NoError(t, err) + excludeMatchProperties, err := filterconfig.CreateMetricMatchPropertiesFromDefault(tt.condition.Exclude) + assert.NoError(t, err) + + boolExpr, err := newSkipResExpr(includeMatchProperties, excludeMatchProperties) require.NoError(t, err) expectedResult, err := boolExpr.Eval(context.Background(), tCtx) assert.NoError(t, err)