Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: exclude_matching_metrics is excluding non-ProcessSamples #1943

Merged
merged 8 commits into from
Nov 6, 2024
Next Next commit
refactor: move matcher test where other Sample types can be checked
DavSanchez committed Nov 5, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit 43ef4dd0c9733ddfd2f93c079e717f322d7d0b38
5 changes: 2 additions & 3 deletions internal/agent/agent.go
Original file line number Diff line number Diff line change
@@ -296,7 +296,6 @@ func NewAgent(
// 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)
// by default, do not apply exclude metrics matchers, only if no include ones are present
sampleExcludeFn := func(event any) bool {
@@ -1192,7 +1191,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.includeEvent(event)
includeSample := c.IncludeEvent(event)
if !includeSample {
aclog.
WithField("entity_key", entityKey.String()).
@@ -1209,7 +1208,7 @@ func (c *context) SendEvent(event sample.Event, entityKey entity.Key) {
}
}

func (c *context) includeEvent(event any) bool {
func (c *context) IncludeEvent(event any) bool {
shouldInclude := c.shouldIncludeEvent(event)
shouldExclude := c.shouldExcludeEvent(event)

82 changes: 1 addition & 81 deletions internal/agent/agent_test.go
Original file line number Diff line number Diff line change
@@ -915,86 +915,6 @@ func Test_ProcessSampling(t *testing.T) {
}
}

func Test_ProcessSamplingExcludes(t *testing.T) {
t.Parallel()

someSample := &types.ProcessSample{
ProcessDisplayName: "some-process",
}

boolAsPointer := func(val bool) *bool {
return &val
}

type testCase struct {
name string
c *config.Config
ff feature_flags.Retriever
expectInclude bool
}
testCases := []testCase{
{
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{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 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 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 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,
},
}

for _, tc := range testCases {
testCase := tc
a, _ := NewAgent(testCase.c, "test", "userAgent", testCase.ff)

t.Run(testCase.name, func(t *testing.T) {
t.Parallel()

assert.Equal(t, testCase.expectInclude, a.Context.includeEvent(someSample))
})
}
}

func Test_ProcessSamplingExcludesAllCases(t *testing.T) {
t.Parallel()

@@ -1188,7 +1108,7 @@ func Test_ProcessSamplingExcludesAllCases(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()

assert.Equal(t, testCase.expectInclude, a.Context.includeEvent(someSample))
assert.Equal(t, testCase.expectInclude, a.Context.IncludeEvent(someSample))
})
}
}
95 changes: 95 additions & 0 deletions pkg/metrics/sampler/matcher_test.go
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@ import (
"os"
"testing"

"github.com/newrelic/infrastructure-agent/internal/agent"
"github.com/newrelic/infrastructure-agent/internal/feature_flags"
testFF "github.com/newrelic/infrastructure-agent/internal/feature_flags/test"
"github.com/newrelic/infrastructure-agent/pkg/config"
@@ -746,3 +747,97 @@ func TestNewSampleMatchFn(t *testing.T) {
})
}
}

//nolint:exhaustruct
func Test_ProcessSamplingExcludes(t *testing.T) {
t.Parallel()

someProcessSample := &types.ProcessSample{
ProcessDisplayName: "some-process",
}
// someNetworkSample := network.NetworkSample{InterfaceName: "eth0"}
// someSystemSample := metrics.SystemSample{
// CPUSample: &metrics.CPUSample{
// CPUPercent: 50,
// },
// }
// someStorageSample := storage.BaseSample{
// Device: "/dev/sda1",
// }

boolAsPointer := func(val bool) *bool {
return &val
}

type testCase struct {
name string
c *config.Config
ff feature_flags.Retriever
expectInclude bool
}
testCases := []testCase{
{
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: testFF.NewFFRetrieverReturning(false, false),
expectInclude: false,
},
{
name: "Include matching should not exclude",
c: &config.Config{EnableProcessMetrics: boolAsPointer(true), IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, DisableCloudMetadata: true},
ff: testFF.NewFFRetrieverReturning(false, false),
expectInclude: 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: testFF.NewFFRetrieverReturning(false, false),
expectInclude: false,
},
{
name: "Exclude matching should exclude",
c: &config.Config{ExcludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}}, DisableCloudMetadata: true},
ff: testFF.NewFFRetrieverReturning(false, false),
expectInclude: false,
},
{
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: testFF.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: testFF.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: testFF.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: testFF.NewFFRetrieverReturning(false, false),
expectInclude: true,
},
}

for _, tc := range testCases {
testCase := tc
a, _ := agent.NewAgent(testCase.c, "test", "userAgent", testCase.ff)

t.Run(testCase.name, func(t *testing.T) {
t.Parallel()

assert.Equal(t, testCase.expectInclude, a.Context.IncludeEvent(someProcessSample))
// In all cases, events that are not ProcessSamples should always be included!
// assert.True(t, a.Context.IncludeEvent(someSystemSample))
// assert.True(t, a.Context.IncludeEvent(someNetworkSample))
// assert.True(t, a.Context.IncludeEvent(someStorageSample))
})
}
}