diff --git a/internal/agent/agent.go b/internal/agent/agent.go index fae974d21..2eeed2dc2 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -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" @@ -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 { @@ -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()) @@ -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 { @@ -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 } @@ -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) { diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index b9401df13..01a36f91b 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -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 } @@ -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() @@ -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)) }) } } diff --git a/pkg/metrics/sampler/matcher.go b/pkg/metrics/sampler/matcher.go index b934f63d4..820abfd2f 100644 --- a/pkg/metrics/sampler/matcher.go +++ b/pkg/metrics/sampler/matcher.go @@ -30,13 +30,13 @@ var ( // the metrics matcher (processor.MatcherChain) interface. type MatcherFn func(event any) bool -// IncludeSampleMatchFn func that returns whether an event/sample should be included, it satisfies +// IncludeProcessSampleMatchFn func that returns whether an event/sample should be included, it satisfies // the metrics matcher (processor.MatcherChain) interface. -type IncludeSampleMatchFn MatcherFn +type IncludeProcessSampleMatchFn MatcherFn -// ExcludeSampleMatchFn func that returns whether an event/sample should be excluded, it satisfies +// ExcludeProcessSampleMatchFn func that returns whether an event/sample should be excluded, it satisfies // the metrics matcher (processor.MatcherChain) interface. -type ExcludeSampleMatchFn MatcherFn +type ExcludeProcessSampleMatchFn MatcherFn // ExpressionMatcher is an interface every evaluator must implement type ExpressionMatcher interface { @@ -74,10 +74,6 @@ type matcher struct { } func (p matcher) Evaluate(event interface{}) bool { - if skipSample(event, typesToEvaluate) { - return true - } - actualValue := getFieldValue(event, p.PropertyName) if actualValue == nil { return false @@ -134,17 +130,6 @@ func getFieldValue(object interface{}, fieldNames []string) interface{} { } } -// determine is this is a sample that should be evaluated -// we are only checking ProcessSamples at this point, so only those should be evaluated -func skipSample(sample interface{}, typesToEvaluate map[string]bool) bool { - v := reflect.TypeOf(sample) - if v.Kind() == reflect.Ptr { - v = v.Elem() - } - // skip if not "registered" - return !typesToEvaluate[v.Name()] -} - func literalExpressionEvaluator(expected interface{}, actual interface{}) bool { return expected == actual } @@ -277,70 +262,64 @@ func (ne constantMatcher) String() string { return fmt.Sprint(ne.value) } -// NewSampleMatchFn creates new includeSampleMatchFn func, enableProcessMetrics might be nil when -// value was not set. -func NewSampleMatchFn(enableProcessMetrics *bool, metricsMatchers config.MetricsMap, ffRetriever feature_flags.Retriever) MatcherFn { - // configuration option always takes precedence over FF and matchers configuration - if enableProcessMetrics == nil { - matcher := matcherFromMetricsMatchers(metricsMatchers) - if matcher != nil { - return matcher +// NewIncludeProcessSampleMatchFn returns a function `func(sample) bool` that determinesif the sample +// should be included (true) as an event or not (false). Note that this is NOT the negation +// of `NewExcludeProcessSampleMatchFn`. +// The include decision logic only applies to ProcessSamples. Other kinds of samples are always included. +func NewIncludeProcessSampleMatchFn(enableProcessMetrics *bool, metricsMatchers config.IncludeMetricsMap, ffRetriever feature_flags.Retriever) MatcherFn { + return func(sample any) bool { + + // Continuing with the logic + // configuration option always takes precedence over FF and matchers configuration + if enableProcessMetrics == nil { + matcher := matcherFromMetricsMatchers(config.MetricsMap(metricsMatchers)) + if matcher != nil { + return matcher(sample) + } + + // configuration option is not defined and feature flag is present, FF determines, otherwise + // all process samples will be excluded. Note that this is not exactly the negation + // of `NewExcludeSampleMatchFn`. + enabled, exists := ffRetriever.GetFeatureFlag(fflag.FlagFullProcess) + + return exists && enabled } - // configuration option is not defined and feature flag is present, FF determines, otherwise - // all process samples will be excluded - return matcherFromFeatureFlag(ffRetriever) - } + if !*enableProcessMetrics { + mlog. + WithField(config.TracesFieldName, config.FeatureTrace). + Trace("EnableProcessMetrics is FALSE, process metrics will be DISABLED") - if excludeProcessMetrics(enableProcessMetrics) { - mlog. - WithField(config.TracesFieldName, config.FeatureTrace). - Trace("EnableProcessMetrics is FALSE, process metrics will be DISABLED") + return false + } - return matcherForDisabledMetrics() - } + matcher := matcherFromMetricsMatchers(config.MetricsMap(metricsMatchers)) + if matcher != nil { + return matcher(sample) + } - matcherChain := NewMatcherChain(metricsMatchers) - if matcherChain.Enabled { mlog. WithField(config.TracesFieldName, config.FeatureTrace). - Trace("EnableProcessMetrics is TRUE and rules ARE defined, process metrics will be ENABLED for matching processes") - - return matcherChain.Evaluate - } + Trace("EnableProcessMetrics is TRUE and rules are NOT defined, ALL process metrics will be ENABLED unless excluded_matching_metrics rules are defined") - mlog. - WithField(config.TracesFieldName, config.FeatureTrace). - Trace("EnableProcessMetrics is TRUE and rules are NOT defined, ALL process metrics will be ENABLED") - - return func(sample interface{}) bool { - // all process samples are included return true } } -func matcherForDisabledMetrics() MatcherFn { - return func(sample interface{}) bool { - switch sample.(type) { - case *types.ProcessSample: - mlog. - WithField(config.TracesFieldName, config.FeatureTrace). - Trace("Got a sample of type '*types.ProcessSample' so excluding sample.") - // no process samples are included - return false - case *types.FlatProcessSample: - mlog. - WithField(config.TracesFieldName, config.FeatureTrace). - Trace("Got a sample of type '*types.FlatProcessSample' so excluding sample.") - // no flat process samples are included - return false - default: - mlog. - WithField(config.TracesFieldName, config.FeatureTrace). - Tracef("Got a sample of type '%s' that should not be excluded.", reflect.TypeOf(sample).String()) - // other samples are included - return true +// NewExcludeProcessSampleMatchFn returns a function `func(sample) bool` that determinesif the sample +// should be excluded (true) as an event or not (false). Note that this is NOT the negation +// of `NewIncludeProcessSampleMatchFn`. In particular, we don't check here for the `enableProcessMetrics` +// being unset or disabled because it is checked before calling this function at `agent.NewAgent`. +// The exclude decision logic only applies to ProcessSamples. Other kinds of samples are never excluded. +func NewExcludeProcessSampleMatchFn(metricsMatchers config.ExcludeMetricsMap) MatcherFn { + return func(sample any) bool { + + matcher := matcherFromMetricsMatchers(config.MetricsMap(metricsMatchers)) + if matcher != nil { + return matcher(sample) } + + return false } } @@ -357,25 +336,3 @@ func matcherFromMetricsMatchers(metricsMatchers config.MetricsMap) MatcherFn { return nil } - -func matcherFromFeatureFlag(ffRetriever feature_flags.Retriever) MatcherFn { - return func(sample any) bool { - _, isProcessSample := sample.(*types.ProcessSample) - _, isFlatProcessSample := sample.(*types.FlatProcessSample) - - if !isProcessSample && !isFlatProcessSample { - return true - } - - enabled, exists := ffRetriever.GetFeatureFlag(fflag.FlagFullProcess) - - return exists && enabled - } -} - -func excludeProcessMetrics(enableProcessMetrics *bool) bool { - if enableProcessMetrics == nil || *enableProcessMetrics { - return false - } - return true -} diff --git a/pkg/metrics/sampler/matcher_test.go b/pkg/metrics/sampler/matcher_test.go index e5e1a0677..98ff9f91b 100644 --- a/pkg/metrics/sampler/matcher_test.go +++ b/pkg/metrics/sampler/matcher_test.go @@ -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" @@ -247,61 +248,6 @@ func Test_Evaluator_WithUnMappedFields(t *testing.T) { } } -// Test_Evaluator_WithNonProcessSamples tests that other sample types keep working as expected -func Test_Evaluator_WithNonProcessSamples(t *testing.T) { - networkSample := network.NetworkSample{InterfaceName: "eth0"} - systemSample := metrics.SystemSample{ - CPUSample: &metrics.CPUSample{ - CPUPercent: 50, - }, - } - storageSample := storage.BaseSample{ - Device: "/dev/sda1", - } - - type testCase struct { - name string - input interface{} - rules map[string][]string - want bool - } - - cases := []testCase{ - { - name: "NetworkSample", - input: networkSample, - rules: map[string][]string{ - "process.name": {"foobar"}, - }, - want: true, - }, - { - name: "SystemSample", - input: systemSample, - rules: map[string][]string{ - "process.name": {"foobar"}, - }, - want: true, - }, - { - name: "StorageSample", - input: storageSample, - rules: map[string][]string{ - "process.name": {"foobar"}, - }, - want: true, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - ec := sampler.NewMatcherChain(tc.rules) - assert.Len(t, ec.Matchers, len(tc.rules)) - assert.EqualValues(t, tc.want, ec.Evaluate(tc.input)) - }) - } -} - func Test_EvaluatorChain_WithMultipleRuleAttribute(t *testing.T) { type testCase struct { @@ -588,16 +534,6 @@ func TestNewSampleMatchFn(t *testing.T) { args args include bool }{ - { - name: "non process samples are always included", - args: args{ - enableProcessMetrics: &falseVar, - includeMetricsMatchers: emptyMatchers, - ffRetriever: testFF.EmptyFFRetriever, - sample: &fixture.NetworkSample, - }, - include: true, - }, { name: "when enableProcessMetrics is FALSE process samples are always excluded", args: args{ @@ -741,8 +677,102 @@ func TestNewSampleMatchFn(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - matchFn := sampler.NewSampleMatchFn(tt.args.enableProcessMetrics, config.MetricsMap(tt.args.includeMetricsMatchers), tt.args.ffRetriever) + matchFn := sampler.NewIncludeProcessSampleMatchFn(tt.args.enableProcessMetrics, tt.args.includeMetricsMatchers, tt.args.ffRetriever) assert.Equal(t, tt.include, matchFn(tt.args.sample)) }) } } + +//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 + agent, _ := agent.NewAgent(testCase.c, "test", "userAgent", testCase.ff) + + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, testCase.expectInclude, agent.Context.IncludeEvent(someProcessSample)) + // In all cases, events that are not ProcessSamples should always be included! + assert.True(t, agent.Context.IncludeEvent(someSystemSample)) + assert.True(t, agent.Context.IncludeEvent(someNetworkSample)) + assert.True(t, agent.Context.IncludeEvent(someStorageSample)) + }) + } +}