fix: enable_process_metrics was ignored by metrics matcher (#1927)

internal/agent/agent.go

@@ -283,9 +283,36 @@ func NewAgent(
 	cloudHarvester.Initialize(cloud.WithProvider(cloud.Type(cfg.CloudProvider)))
 
 	idLookupTable := NewIdLookup(hostnameResolver, cloudHarvester, cfg.DisplayName)
+
+	// Matchers logic:
+	// * If enable_process_metrics is defined and false, no process will be sent
+	// * If enable_process_metrics is not defined ( == nil ) AND there are include_metrics_matchers,
+	// then only the metrics that match the include_metrics_matchers will be sent
+	// * If enable_process_metrics is not defined ( == nil ) AND there are exclude_metrics_matchers,
+	// exlude list is ignored and no process is sent
+	// * If enable_process_metrics == true AND there are include_metrics_matchers and exclude_metrics_matchers,
+	// exclude ones are ignored and only the ones that match the include_metrics_matchers are sent
+	// * If enable_process_metrics == true AND there are exlude_metrics_matchers ,
+	// all the processes but excluded ones will be sent
+	// * If all the cases where include_metrics_matchers and exclude_metrics_matchers are present,
+	// exclude ones will be ignored
+
 	sampleMatchFn := sampler.NewSampleMatchFn(cfg.EnableProcessMetrics, config.MetricsMap(cfg.IncludeMetricsMatchers), ffRetriever)
-	sampleExcludeFn := sampler.NewSampleMatchFn(cfg.EnableProcessMetrics, config.MetricsMap(cfg.ExcludeMetricsMatchers), ffRetriever)
-	ctx := NewContext(cfg, buildVersion, hostnameResolver, idLookupTable, sampler.IncludeSampleMatchFn(sampleMatchFn), sampler.ExcludeSampleMatchFn(sampleExcludeFn))
+	// by default, do not apply exclude metrics matchers, only if no include ones are present
+	sampleExcludeFn := func(event any) bool {
+		return true
+	}
+	if len(cfg.IncludeMetricsMatchers) == 0 &&
+		cfg.EnableProcessMetrics != nil &&
+		*cfg.EnableProcessMetrics &&
+		len(cfg.ExcludeMetricsMatchers) > 0 {
+		sampleExcludeFn = sampler.NewSampleMatchFn(cfg.EnableProcessMetrics, config.MetricsMap(cfg.ExcludeMetricsMatchers), ffRetriever)
+		// if there are not include matchers at all, we remove the matcher to exclude by default
+		sampleMatchFn = func(event any) bool {
+			return false
+		}
+	}
+	ctx := NewContext(cfg, buildVersion, hostnameResolver, idLookupTable, sampler.IncludeSampleMatchFn(sampleMatchFn), sampleExcludeFn)
 
 	agentKey, err := idLookupTable.AgentKey()
 	if err != nil {
@@ -1165,7 +1192,7 @@ func (c *context) SendEvent(event sample.Event, entityKey entity.Key) {
 	// check if event should be included
 	// include takes precedence, so the event will be included if
 	// it IS NOT EXCLUDED or if it IS INCLUDED
-	includeSample := !c.shouldExcludeEvent(event) || c.shouldIncludeEvent(event)
+	includeSample := c.includeEvent(event)
 	if !includeSample {
 		aclog.
 			WithField("entity_key", entityKey.String()).
@@ -1182,6 +1209,13 @@ func (c *context) SendEvent(event sample.Event, entityKey entity.Key) {
 	}
 }
 
+func (c *context) includeEvent(event any) bool {
+	shouldInclude := c.shouldIncludeEvent(event)
+	shouldExclude := c.shouldExcludeEvent(event)
+
+	return shouldInclude || !shouldExclude
+}
+
 func (c *context) Unregister(id ids.PluginID) {
 	c.ch <- types.NewNotApplicableOutput(id)
 }

internal/agent/agent_test.go

@@ -922,42 +922,64 @@ func Test_ProcessSamplingExcludes(t *testing.T) {
 		ProcessDisplayName: "some-process",
 	}
 
+	boolAsPointer := func(val bool) *bool {
+		return &val
+	}
+
 	type testCase struct {
-		name string
-		c    *config.Config
-		ff   feature_flags.Retriever
-		want bool
+		name          string
+		c             *config.Config
+		ff            feature_flags.Retriever
+		expectInclude bool
 	}
 	testCases := []testCase{
 		{
-			name: "Include not matching must not exclude",
-			c:    &config.Config{IncludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}}, DisableCloudMetadata: true},
-			ff:   test.NewFFRetrieverReturning(false, false),
-			want: false,
+			name:          "Include not matching must not include",
+			c:             &config.Config{EnableProcessMetrics: boolAsPointer(true), IncludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}}, DisableCloudMetadata: true},
+			ff:            test.NewFFRetrieverReturning(false, false),
+			expectInclude: false,
 		},
 		{
-			name: "Include matching should not exclude",
-			c:    &config.Config{IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, DisableCloudMetadata: true},
-			ff:   test.NewFFRetrieverReturning(false, false),
-			want: false,
+			name:          "Include matching should not exclude",
+			c:             &config.Config{EnableProcessMetrics: boolAsPointer(true), IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, DisableCloudMetadata: true},
+			ff:            test.NewFFRetrieverReturning(false, false),
+			expectInclude: true,
 		},
 		{
-			name: "Exclude matching should exclude",
-			c:    &config.Config{ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, DisableCloudMetadata: true},
-			ff:   test.NewFFRetrieverReturning(false, false),
-			want: true,
+			name:          "Exclude matching should exclude with process metrics enabled",
+			c:             &config.Config{EnableProcessMetrics: boolAsPointer(true), ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, DisableCloudMetadata: true},
+			ff:            test.NewFFRetrieverReturning(false, false),
+			expectInclude: false,
 		},
 		{
-			name: "Exclude not matching should not exclude",
-			c:    &config.Config{ExcludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}}, DisableCloudMetadata: true},
-			ff:   test.NewFFRetrieverReturning(false, false),
-			want: false,
+			name:          "Exclude matching should exclude",
+			c:             &config.Config{ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, DisableCloudMetadata: true},
+			ff:            test.NewFFRetrieverReturning(false, false),
+			expectInclude: false,
 		},
 		{
-			name: "Exclude matching should exclude even if include is configured",
-			c:    &config.Config{IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, DisableCloudMetadata: true},
-			ff:   test.NewFFRetrieverReturning(false, false),
-			want: true,
+			name:          "Exclude not matching should not exclude with process metrics enabled",
+			c:             &config.Config{EnableProcessMetrics: boolAsPointer(true), ExcludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}}, DisableCloudMetadata: true},
+			ff:            test.NewFFRetrieverReturning(false, false),
+			expectInclude: true,
+		},
+		{
+			name:          "Exclude not matching should not exclude",
+			c:             &config.Config{ExcludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}}, DisableCloudMetadata: true},
+			ff:            test.NewFFRetrieverReturning(false, false),
+			expectInclude: false,
+		},
+		{
+			name:          "Include matching should include even if exclude is configured with process metrics enabled",
+			c:             &config.Config{EnableProcessMetrics: boolAsPointer(true), IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, DisableCloudMetadata: true},
+			ff:            test.NewFFRetrieverReturning(false, false),
+			expectInclude: true,
+		},
+		{
+			name:          "Include matching should be include even when enable_process_metrics is not defined (nil)",
+			c:             &config.Config{IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, DisableCloudMetadata: true},
+			ff:            test.NewFFRetrieverReturning(false, false),
+			expectInclude: true,
 		},
 	}
 
@@ -968,8 +990,205 @@ func Test_ProcessSamplingExcludes(t *testing.T) {
 		t.Run(testCase.name, func(t *testing.T) {
 			t.Parallel()
 
-			actual := a.Context.shouldExcludeEvent(someSample)
-			assert.Equal(t, testCase.want, actual)
+			assert.Equal(t, testCase.expectInclude, a.Context.includeEvent(someSample))
+		})
+	}
+}
+
+func Test_ProcessSamplingExcludesAllCases(t *testing.T) {
+	t.Parallel()
+
+	someSample := &types.ProcessSample{
+		ProcessDisplayName: "some-process",
+	}
+
+	boolAsPointer := func(val bool) *bool {
+		return &val
+	}
+
+	type testCase struct {
+		name                   string
+		processMetricsEnabled  *bool
+		IncludeMetricsMatchers map[string][]string
+		ExcludeMetricsMatchers map[string][]string
+		expectInclude          bool
+	}
+
+	//nolint:dupword
+	// Test cases
+	// EnableProcessMetrics | IncludeMetricsMatchers | ExcludeMetricsMatchers | Expected include
+	// nil | nil | nil | false
+	// nil | [process that matches ] | nil | true
+	// nil | [process that not matches ] | nil | false
+	// nil | [process that matches ] | [process that matches ] | true
+	// nil | [process that not matches ] | [process that not matche ] | false
+	// nil | nil | [process that matches ] | false
+	// nil | nil | [process that not matches ] | false
+	// false | nil | nil | false
+	// false | [process that matches ] | nil | false
+	// false | nil |  [process that not matche ] | false
+	// false | [process that matches ] | [process that matches ] | false
+	// false | [process that not matches ] | [process that not matche ] | false
+	// true | nil | nil | true
+	// true | [process that matches ] | nil | true
+	// true | [process that not matches ] | nil | false
+	// true | [process that matches ] | [process that matches ] | true
+	// true | [process that not matches ] | [process that not matche ] | false
+	// true | nil  | [process that matches ] | false
+	// true | nil  | [process that not matches ] | true
+
+	testCases := []testCase{
+		{
+			name:                   "nil | nil | nil | false",
+			processMetricsEnabled:  nil,
+			IncludeMetricsMatchers: nil,
+			ExcludeMetricsMatchers: nil,
+			expectInclude:          false,
+		},
+		{
+			name:                   "nil | [process that matches] | nil | true",
+			processMetricsEnabled:  nil,
+			IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
+			ExcludeMetricsMatchers: nil,
+			expectInclude:          true,
+		},
+		{
+			name:                   "nil | [process that not matches] | nil | false",
+			processMetricsEnabled:  nil,
+			IncludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}},
+			ExcludeMetricsMatchers: nil,
+			expectInclude:          false,
+		},
+		{
+			name:                   "nil | [process that matches] | [process that matches] | true",
+			processMetricsEnabled:  nil,
+			IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
+			ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
+			expectInclude:          true,
+		},
+		{
+			name:                   "nil | [process that not matches] | [process that not matches] | false",
+			processMetricsEnabled:  nil,
+			IncludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}},
+			ExcludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}},
+			expectInclude:          false,
+		},
+		{
+			name:                   "nil | nil | [process that matches] | false",
+			processMetricsEnabled:  nil,
+			IncludeMetricsMatchers: nil,
+			ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
+			expectInclude:          false,
+		},
+		{
+			name:                   "nil | nil | [process that not matches] | false",
+			processMetricsEnabled:  nil,
+			IncludeMetricsMatchers: nil,
+			ExcludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}},
+			expectInclude:          false,
+		},
+		{
+			name:                   "false | nil | nil | false",
+			processMetricsEnabled:  boolAsPointer(false),
+			IncludeMetricsMatchers: nil,
+			ExcludeMetricsMatchers: nil,
+			expectInclude:          false,
+		},
+		{
+			name:                   "false | [process that matches] | nil | false",
+			processMetricsEnabled:  boolAsPointer(false),
+			IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
+			ExcludeMetricsMatchers: nil,
+			expectInclude:          false,
+		},
+		{
+			name:                   "false | nil | [process that not matches] | false",
+			processMetricsEnabled:  boolAsPointer(false),
+			IncludeMetricsMatchers: nil,
+			ExcludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}},
+			expectInclude:          false,
+		},
+		{
+			name:                   "false | [process that matches] | [process that matches] | false",
+			processMetricsEnabled:  boolAsPointer(false),
+			IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
+			ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
+			expectInclude:          false,
+		},
+		{
+			name:                   "false | [process that not matches] | [process that not matches] | false",
+			processMetricsEnabled:  boolAsPointer(false),
+			IncludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}},
+			ExcludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}},
+			expectInclude:          false,
+		},
+		{
+			name:                   "true | nil | nil | true",
+			processMetricsEnabled:  boolAsPointer(true),
+			IncludeMetricsMatchers: nil,
+			ExcludeMetricsMatchers: nil,
+			expectInclude:          true,
+		},
+		{
+			name:                   "true | [process that matches] | nil | true",
+			processMetricsEnabled:  boolAsPointer(true),
+			IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
+			ExcludeMetricsMatchers: nil,
+			expectInclude:          true,
+		},
+		{
+			name:                   "true | [process that not matches] | nil | false",
+			processMetricsEnabled:  boolAsPointer(true),
+			IncludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}},
+			ExcludeMetricsMatchers: nil,
+			expectInclude:          false,
+		},
+		{
+			name:                   "true | [process that matches] | [process that matches] | true",
+			processMetricsEnabled:  boolAsPointer(true),
+			IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
+			ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
+			expectInclude:          true,
+		},
+		{
+			name:                   "true | [process that not matches] | [process that not matches] | false",
+			processMetricsEnabled:  boolAsPointer(true),
+			IncludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}},
+			ExcludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}},
+			expectInclude:          false,
+		},
+		{
+			name:                   "true | nil | [process that matches] | false",
+			processMetricsEnabled:  boolAsPointer(true),
+			IncludeMetricsMatchers: nil,
+			ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
+			expectInclude:          false,
+		},
+		{
+			name:                   "true | nil | [process that not matches] | true",
+			processMetricsEnabled:  boolAsPointer(true),
+			IncludeMetricsMatchers: nil,
+			ExcludeMetricsMatchers: map[string][]string{"process.name": {"does-not-match"}},
+			expectInclude:          true,
+		},
+	}
+
+	for _, tc := range testCases {
+		testCase := tc
+		cnf := &config.Config{
+			EnableProcessMetrics:   testCase.processMetricsEnabled,
+			IncludeMetricsMatchers: testCase.IncludeMetricsMatchers,
+			ExcludeMetricsMatchers: testCase.ExcludeMetricsMatchers,
+			DisableCloudMetadata:   true,
+		}
+
+		ff := test.NewFFRetrieverReturning(false, false)
+		a, _ := NewAgent(cnf, "test", "userAgent", ff)
+
+		t.Run(testCase.name, func(t *testing.T) {
+			t.Parallel()
+
+			assert.Equal(t, testCase.expectInclude, a.Context.includeEvent(someSample))
 		})
 	}
 }