Introduce inferred-context for filterprocessor by jaronoff97 · Pull Request #39688 · open-telemetry/opentelemetry-collector-contrib

jaronoff97 · 2025-04-28T15:50:35Z

Description

This PR adds the ability to do context inference for conditions for the filter processor. New config fields are introduced for the supported telemetry types and an isolated code path is introduced for ease of review and separation of concerns

Link to tracking issue

Fixes #37904

Testing

Unit, ran locally

Documentation

TODO

atoulme · 2025-05-09T18:27:17Z

Looks OK to my untrained eye, asking for codeowner review.

edmocosta

Thanks for working on it, @jaronoff97, it's great to have standardized settings across processors :)

These are early comments; I'm still going through the PR.

processor/filterprocessor/config.go

edmocosta · 2025-05-15T16:40:41Z

processor/filterprocessor/config.go

 // Validate checks if the processor configuration is valid
 func (cfg *Config) Validate() error {
+	// TODO: The old format should be deprecated and removed.
+	if cfg.TraceConditions != nil && (cfg.Traces.SpanConditions != nil || cfg.Traces.SpanEventConditions != nil) {


Can we please add some tests for those mixed-style conditions?

processor/filterprocessor/config.go

edmocosta · 2025-05-19T08:08:53Z

processor/filterprocessor/logs.go

 	flp.telemetry = fpt

+	if len(cfg.LogConditions) > 0 {
+		pc, collectionErr := common.NewLogParserCollection(set.TelemetrySettings, common.WithLogParser(filterottl.StandardLogFuncs()))


We're missing passing the default error_mode options (common.With*ErrorMode) for the parser collections (other signals too). In addition to that, can we please also port the existing error mode unit tests from the transform processor?

do you want me to port these tests for the config or these tests for the processors, or both?

I meant the second set of tests that are specific for error mode.

processor/filterprocessor/logs.go

edmocosta · 2025-05-19T09:20:17Z

processor/filterprocessor/internal/common/processor.go

+	return Scope
+}
+
+func (s scopeConditions) ConsumeTraces(ctx context.Context, td ptrace.Traces) error {


It seems we've no coverage for the scope context (all signals). Can we please add some tests for that?

edmocosta · 2025-05-19T09:39:27Z

processor/filterprocessor/config.go

+	if cfg.TraceConditions != nil && (cfg.Traces.SpanConditions != nil || cfg.Traces.SpanEventConditions != nil) {
+		return errors.New("cannot use inferred context trace conditions and old format at the same time")
+	}
+	if cfg.MetricConditions != nil && (cfg.Metrics.MetricConditions != nil || cfg.Metrics.DataPointConditions != nil) {


I'm trying to wrap my head around on how this processor config should work, but based on the existing settings and validations (docs is missing), it should use either the include and exclude settings, OR the OTTL conditions, but not both.

That said, I think we're missing a validation here, so it does not allow the include and exclude keys when <sinal>_conditions are configured.

edmocosta · 2025-05-19T09:44:49Z

processor/filterprocessor/logs.go

+		if errors != nil {
+			return nil, errors
+		}
+	}


We should early return here as skipExpr won't be evaluated when the signal condition is set.

edmocosta · 2025-05-19T09:53:50Z

processor/filterprocessor/logs.go

 )

 type filterLogProcessor struct {
+	consumers []common.LogsConsumer


It would be great if we could add some tests for all signals processors using their context inferrer settings (like we do for the transformprocessor (example), so we can ensure that filtering all possible inferred contexts works.

I'm still think we need to add tests for the high level processor code (all signals). I might be wrong, but I don't think the current implementation is working as expected with the new context inferred style.

At the moment, we're skipping processing the data for all signals when the skipExpr is nil, leading configurations with only context inferred conditions to be no-op.

github-actions · 2025-06-03T05:21:17Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

jaronoff97 · 2025-06-06T15:04:14Z

@edmocosta i still want to work at this, ive been very busy the past week 😢

jaronoff97 · 2025-07-08T21:05:11Z

still going through rn to add tests!

jaronoff97 · 2025-07-08T23:31:08Z

@edmocosta i think this should be ready for another review. One thing i'd like your take on – I think we could probably condense the individual <signal>_conditions into a single ottl_conditions and just parse them dynamically with a single parser collection rather than having one per telemetry type, this would simplify the configuration a lot as well as a lot of the code. What do you think?

edmocosta · 2025-07-09T09:51:34Z

processor/filterprocessor/internal/contextfilter/logs_test.go

+			switch {
+			case tt.filterEverything && !tt.wantErr:
+				assert.Equal(t, processorhelper.ErrSkipProcessingData, consumeErr)
+			case tt.wantErr:


Do we need this condition and the above && !tt.wantErr? It seems we're early returning it when tt.wantErr == true at line 96. If we want to assert both operations ParseContextConditions and ConsumeLogs errors, we probably should be more specific on the expected error string.

edmocosta · 2025-07-09T11:18:03Z

processor/filterprocessor/internal/contextfilter/logs_test.go

+		errorMode     ottl.ErrorMode
+		conditions    []common.ContextConditions
+		want          func(td plog.Logs)
+		wantErr       bool


It seems we're not using this wantErr field on the tests scenarios. if so, we could just drop it and keep only the wantErrorWith.

edmocosta · 2025-07-09T11:25:31Z

processor/filterprocessor/internal/contextfilter/logs_test.go

+			// Apply each consumer sequentially
+			for _, consumer := range consumers {
+				if err := consumer.ConsumeLogs(context.Background(), finalLogs); err != nil {
+					if errors.Is(err, processorhelper.ErrSkipProcessingData) {


I didn't fully understand this block, shouldn't it be something like the following if the idea is to ignore ErrSkipProcessingData errors? am I missing something?

if err := consumer.ConsumeLogs(context.Background(), finalLogs); err != nil { if !errors.Is(err, processorhelper.ErrSkipProcessingData) { consumeErr = err break } }

If that's the case, we can also drop the if condition at line 236 and assert.NoError(t, consumeErr) it directly.

edmocosta · 2025-07-09T12:41:29Z

processor/filterprocessor/logs.go

 )

 type filterLogProcessor struct {
+	consumers []common.LogsConsumer


I'm still think we need to add tests for the high level processor code (all signals). I might be wrong, but I don't think the current implementation is working as expected with the new context inferred style.

At the moment, we're skipping processing the data for all signals when the skipExpr is nil, leading configurations with only context inferred conditions to be no-op.

edmocosta · 2025-07-09T13:04:54Z

@edmocosta i think this should be ready for another review. One thing i'd like your take on – I think we could probably condense the individual <signal>_conditions into a single ottl_conditions and just parse them dynamically with a single parser collection rather than having one per telemetry type, this would simplify the configuration a lot as well as a lot of the code. What do you think?

That's interesting, but I'm not completely sure if that would work, we need to run some tests to double check that.
My concern are the shared contexts, such as resource and scope, which are common for all other signals. In that case, if one condition only use those shared contexts, e.g. `resource.attributes["bar"] == "foo"', It would apply & filter data for all signals, which maybe is not desired I guess 🤔

github-actions · 2025-07-24T05:22:15Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2025-08-08T05:23:10Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2025-08-27T05:20:55Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2025-09-24T05:20:51Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2025-10-10T05:20:48Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2025-10-25T05:20:22Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@jaronoff97

This commit continues the work from PR open-telemetry#39688 by @jaronoff97 - Carried over the original implementation - Fixed error_mode ignore test cases - Fixed issues with resource filtering Co-authored-by: jaronoff97 <jacobaronoff45@gmail.com>

necessary filter processor steps

f4adf6e

github-actions bot added the processor/filter Filter processor label Apr 28, 2025

github-actions bot requested review from TylerHelmuth and boostchicken April 28, 2025 15:50

jaronoff97 and others added 5 commits April 28, 2025 11:53

mutlierr

cc7e7bb

fix links

a831d1b

add in some sample tests

8f8f4ba

Merge branch 'main' into jaronoff97-37904-filter-processor-changes

29f3864

make it usable

b693746

jaronoff97 marked this pull request as ready for review May 6, 2025 16:27

jaronoff97 requested a review from a team as a code owner May 6, 2025 16:27

github-actions bot assigned djaglowski May 6, 2025

fix lint

4578e3e

atoulme added the waiting-for-code-owners label May 9, 2025

TylerHelmuth requested review from edmocosta and evan-bradley May 12, 2025 17:13

github-actions bot mentioned this pull request May 13, 2025

Weekly Report: 2025-05-06 - 2025-05-13 #40023

Closed

edmocosta reviewed May 19, 2025

View reviewed changes

djaglowski removed their assignment May 19, 2025

This was referenced May 20, 2025

Weekly Report: 2025-05-13 - 2025-05-20 #40138

Closed

Weekly Report: 2025-05-20 - 2025-05-27 #40297

Closed

github-actions bot mentioned this pull request Jun 3, 2025

Weekly Report: 2025-05-27 - 2025-06-03 #40424

Closed

github-actions bot added the Stale label Jun 3, 2025

edmocosta removed the Stale label Jun 3, 2025

edmocosta self-assigned this Jun 3, 2025

github-actions bot mentioned this pull request Jun 10, 2025

Weekly Report: 2025-06-03 - 2025-06-10 #40577

Closed

github-actions bot mentioned this pull request Jun 17, 2025

Weekly Report: 2025-06-10 - 2025-06-17 #40753

Closed

jaronoff97 added 2 commits July 8, 2025 13:31

not yet

377070d

Add some more tests

71f82cd

jaronoff97 added 2 commits July 8, 2025 19:22

validate tests

98047ef

fix lint

0d02373

edmocosta reviewed Jul 9, 2025

View reviewed changes

github-actions bot mentioned this pull request Jul 15, 2025

Weekly Report: 2025-07-08 - 2025-07-15 #41291

Closed

github-actions bot added the Stale label Jul 24, 2025

edmocosta removed the Stale label Jul 24, 2025

github-actions bot added the Stale label Aug 8, 2025

edmocosta removed the Stale label Aug 12, 2025

github-actions bot added the Stale label Aug 27, 2025

edmocosta removed the Stale label Aug 27, 2025

evan-bradley added waiting for author and removed waiting-for-code-owners labels Sep 9, 2025

github-actions bot added the Stale label Sep 24, 2025

edmocosta removed the Stale label Sep 25, 2025

github-actions bot added the Stale label Oct 10, 2025

github-actions bot closed this Oct 25, 2025

edmocosta mentioned this pull request Nov 3, 2025

Allow to filter whole resource in the filter processor #43968

Merged

kaisecheng mentioned this pull request Nov 26, 2025

[processor/filter] Introduce inferred-context #44543

Merged

Conversation

jaronoff97 commented Apr 28, 2025

Description

Link to tracking issue

Testing

Documentation

Uh oh!

atoulme commented May 9, 2025

Uh oh!

edmocosta left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 3, 2025

Uh oh!

jaronoff97 commented Jun 6, 2025

Uh oh!

jaronoff97 commented Jul 8, 2025

Uh oh!

jaronoff97 commented Jul 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edmocosta commented Jul 9, 2025

Uh oh!

github-actions bot commented Jul 24, 2025

Uh oh!

github-actions bot commented Aug 8, 2025

Uh oh!

github-actions bot commented Aug 27, 2025

Uh oh!

github-actions bot commented Sep 24, 2025

Uh oh!

github-actions bot commented Oct 10, 2025

Uh oh!

github-actions bot commented Oct 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

edmocosta left a comment •

edited

Loading