Skip to content

Commit

Permalink
fix: exclude_matching_metrics is excluding non-ProcessSamples (#1943
Browse files Browse the repository at this point in the history 
)

* refactor: move matcher test where other Sample types can be checked

* test: include additional sample types in test

This should make the current tests fail and uncover the source of the issue

* fix: always include non-`ProcessSample`s

* refactor: reduce duplicate checks for sample type

There were many different places where it was checked that the sample was a process sample or not, and the implementations were different each time. At the cost of a little function duplication I have removed the redundant checks and (hopefully) streamlined code

test: remove redundant test

tested function will never be applied to non-processsamples

fix: do not exclude when no exclusion rules are defined

docs: improve function names and state purpose

style: remove redundant function

* style: remove redundant parameters and logic from exclude matcher func

style: print rich representation of excluded sample

* docs: specify functions only decide for ProcessSample

* refactor: check for sample type before applying matchers

* test: remove invalid cases

the removed tests were checking the non-processsample exclusion + FF enabled behavior. Given the non-processsample check is now done elsewhere, these tests are now invalid
  • Loading branch information
@DavSanchez
DavSanchez authored Nov 6, 2024
1 parent 302f196 commit 01ed9a4
Show file tree
Hide file tree
Showing 4 changed files with 170 additions and 273 deletions.
40 changes: 24 additions & 16 deletions internal/agent/agent.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/newrelic/infrastructure-agent/pkg/entity/host"
"github.com/newrelic/infrastructure-agent/pkg/helpers/metric"
"github.com/newrelic/infrastructure-agent/pkg/metrics/sampler"
process_sample_types "github.com/newrelic/infrastructure-agent/pkg/metrics/types"
"github.com/sirupsen/logrus"

"github.com/newrelic/infrastructure-agent/pkg/ctl"
Expand Down Expand Up @@ -159,8 +160,8 @@ type context struct {
resolver hostname.ResolverChangeNotifier
EntityMap entity.KnownIDs
idLookup host.IDLookup
shouldIncludeEvent sampler.IncludeSampleMatchFn
shouldExcludeEvent sampler.ExcludeSampleMatchFn
shouldIncludeEvent sampler.IncludeProcessSampleMatchFn
shouldExcludeEvent sampler.ExcludeProcessSampleMatchFn
}

func (c *context) Context() context2.Context {
Expand Down Expand Up @@ -206,8 +207,8 @@ func NewContext(
buildVersion string,
resolver hostname.ResolverChangeNotifier,
lookup host.IDLookup,
sampleMatchFn sampler.IncludeSampleMatchFn,
sampleExcludeFn sampler.ExcludeSampleMatchFn,
sampleMatchFn sampler.IncludeProcessSampleMatchFn,
sampleExcludeFn sampler.ExcludeProcessSampleMatchFn,
) *context {
ctx, cancel := context2.WithCancel(context2.Background())

Expand Down Expand Up @@ -296,23 +297,22 @@ 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)
processSampleMatchFn := sampler.NewIncludeProcessSampleMatchFn(cfg.EnableProcessMetrics, cfg.IncludeMetricsMatchers, ffRetriever)
// by default, do not apply exclude metrics matchers, only if no include ones are present
sampleExcludeFn := func(event any) bool {
processSampleExcludeFn := 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)
processSampleExcludeFn = sampler.NewExcludeProcessSampleMatchFn(cfg.ExcludeMetricsMatchers)
// if there are not include matchers at all, we remove the matcher to exclude by default
sampleMatchFn = func(event any) bool {
processSampleMatchFn = func(event any) bool {
return false
}
}
ctx := NewContext(cfg, buildVersion, hostnameResolver, idLookupTable, sampler.IncludeSampleMatchFn(sampleMatchFn), sampleExcludeFn)
ctx := NewContext(cfg, buildVersion, hostnameResolver, idLookupTable, sampler.IncludeProcessSampleMatchFn(processSampleMatchFn), processSampleExcludeFn)

agentKey, err := idLookupTable.AgentKey()
if err != nil {
Expand Down Expand Up @@ -1192,11 +1192,11 @@ 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()).
WithField("event", fmt.Sprintf("+%v", event)).
WithField("event", fmt.Sprintf("%#v", event)).
Debug("event excluded by metric matcher")
return
}
Expand All @@ -1209,11 +1209,19 @@ 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)
// Decides wether an event will be included or not.
func (c *context) IncludeEvent(event any) bool {
switch event.(type) {
// rule is applied to process samples only
case *process_sample_types.ProcessSample, *process_sample_types.FlatProcessSample:
shouldInclude := c.shouldIncludeEvent(event)
shouldExclude := c.shouldExcludeEvent(event)

return shouldInclude || !shouldExclude
return shouldInclude || !shouldExclude
default:
// other samples are included
return true
}
}

func (c *context) Unregister(id ids.PluginID) {
Expand Down
100 changes: 1 addition & 99 deletions internal/agent/agent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -818,24 +818,6 @@ func BenchmarkStorePluginOutput(b *testing.B) {
}
}

func Test_ProcessSampling_FeatureFlagIsEnabled(t *testing.T) {
cnf := &config.Config{
IncludeMetricsMatchers: map[string][]string{"process.name": {"some-process"}},
}
someSample := struct {
evenType string
}{
evenType: "ProcessSample",
}
a, _ := NewAgent(cnf, "test", "userAgent", test.NewFFRetrieverReturning(true, true))

// when
actual := a.Context.shouldIncludeEvent(someSample)

// then
assert.Equal(t, true, actual)
}

func getBooleanPtr(val bool) *bool {
return &val
}
Expand Down Expand Up @@ -915,86 +897,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()

Expand Down Expand Up @@ -1188,7 +1090,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))
})
}
}
Expand Down
Loading

0 comments on commit 01ed9a4

Please sign in to comment.