Add log type include filter by jorbaum · Pull Request #714 · cloudfoundry/loggregator-agent-release

jorbaum · 2026-01-05T14:06:49Z

Description

Addresses #711 :

Support both ?include-source-types= and ?exclude-source-types (calling it source type instead of log type as log type is too generic and is already used in this code base)
Throw error in case both are configured. Do not throw an error on misconfigured values for the new config option (consistent with previous HTTP query param usage in binding config), but log a warning (new).
Only exclude filter shall pass unknown/nil source type, include filter will discard

I still need to verify this, but I think a feature flag is not necessary as I tried to make string parsing more efficient by:

running only when source type tag and log type filter is present
strings.IndexByte might take some time, but it is used only with a single char / and strings.IndexByte() should be more efficient than strings.Index()
Looking up content in a map logTypePrefixes rather than in each log line, which should be more efficient

Related PRs:

Disclaimer: Claude helped me in crafting this code.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Testing performed?

Unit tests
Integration tests
Acceptance tests

Checklist:

This PR is being made against the main branch, or relevant version branch
I have made corresponding changes to the documentation
I have added testing for my changes

chombium

@jorbaum Thanks for the changes.

Generally it looks good, but I have some smaller objections which should be addressed.

src/pkg/egress/syslog/filtering_drain_writer.go

src/pkg/egress/syslog/filtering_drain_writer_test.go

chombium · 2026-01-09T13:55:15Z

src/pkg/egress/syslog/syslog_connector.go

 	DrainData    DrainData `json:"type,omitempty"`
 	OmitMetadata bool
 	InternalTls  bool
+	LogFilter    *LogTypeSet


I don't think that this has to be a pointer.

I tried that at first, but in manager.go line 45 Binding is used as a map key:

sourceDrainMap map[string]map[syslog.Binding]drainHolder

My understanding is that maps in Go are not comparable, so adding LogTypeSet (which is a map[LogType]struct{}) to the Binding struct makes it non-comparable.

AFAICS the alternative to using a pointer would be to implement a custom comparable key for the map, but using a pointer seemed simpler to me.

src/pkg/ingress/bindings/binding_config.go

chombium · 2026-01-09T14:08:56Z

src/pkg/ingress/bindings/binding_config.go

+		default:
+			// Unknown log type, skip it
+			// TODO add unit test for unknown log type
+			d.log.Printf("Unknown log type '%s' in log type filter, ignoring", logType)


If there is a log message here and an app owner wrongly configures the drain, than for every app instance and every app log, a log ,message will be written.

The filter settings should be validated upfront and we should not end up in this situation. At this point we should only have valid drain configuration.
I guess we have to bring in #633 first and than this change.

My understanding is that this code does not run for every log message, but only parsing the syslog drain config. I intentionally left logging out of filtering_drain_writer.go to avoid creating a log for each app log.

After discussing with @chombium we concluded that yhis log gets created for each wrongly configured value in the log types of each syslog drain. This is emitted every few seconds on each VM within cloud foundry. While this is better than for each log line, we would still keep log load at a minimum.

To address this I now made sure to log at most one log line in this method.

src/pkg/ingress/bindings/binding_config.go

chombium · 2026-01-09T16:55:59Z

src/pkg/ingress/bindings/binding_config.go

+	set := make(syslog.LogTypeSet, len(logTypes))
+
+	for _, logType := range logTypes {
+		logType = strings.ToLower(strings.TrimSpace(logType))


The query string in the URL is case sensitive. We should only accept lowercase and not convert eVerY POssibLE string to lower case.

Did so.

Still, I wonder if we should be strict here about this. Not caring about casing might bring us some benefits as this might be something that users accidentally misconfigure. It should not hurt much to support both I think.

src/pkg/ingress/bindings/binding_config_test.go

chombium · 2026-01-09T17:05:15Z

@ZPascal Wdyt about this PR? It addresses #711

jorbaum · 2026-01-12T08:04:26Z

@chombium I left a bunch of replies for points that are not yet clear to me. All the rest look good.

Thanks for the thorough review! You found a bunch of nice things.

jorbaum

Worked in some comments.

src/pkg/egress/syslog/filtering_drain_writer.go

jorbaum · 2026-01-12T16:02:28Z

src/pkg/ingress/bindings/binding_config.go

+	set := make(syslog.LogTypeSet, len(logTypes))
+
+	for _, logType := range logTypes {
+		logType = strings.ToLower(strings.TrimSpace(logType))


Did so.

Still, I wonder if we should be strict here about this. Not caring about casing might bring us some benefits as this might be something that users accidentally misconfigure. It should not hurt much to support both I think.

src/pkg/ingress/bindings/binding_config.go

jorbaum

Worked in most comments. Still 2 left to clarify.

jorbaum · 2026-01-13T10:21:47Z

src/pkg/ingress/bindings/binding_config.go

+		default:
+			// Unknown log type, skip it
+			// TODO add unit test for unknown log type
+			d.log.Printf("Unknown log type '%s' in log type filter, ignoring", logType)


After discussing with @chombium we concluded that yhis log gets created for each wrongly configured value in the log types of each syslog drain. This is emitted every few seconds on each VM within cloud foundry. While this is better than for each log line, we would still keep log load at a minimum.

To address this I now made sure to log at most one log line in this method.

src/pkg/ingress/bindings/binding_config_test.go

jorbaum · 2026-01-19T15:30:27Z

Feel free to take another look at it.

I noticed that as my changes depend on `RawQuery` not being empty, but actually they were.

src/pkg/egress/syslog/log.go

Also rename remaining instances of log type to source type

chombium

Hi @jorbaum. Thanks for the improvements. Unfortunately, I've found few more problematic things.

chombium · 2026-02-17T13:38:03Z

src/pkg/egress/syslog/source.go

+		return true
+	}
+
+	if sourceTypeTag == "" {


The source_type is always set. Why is this check needed?

chombium · 2026-02-17T13:46:04Z

src/pkg/egress/syslog/source.go

+type SourceType string
+
+const (
+	SOURCE_API    SourceType = "API"


Add log somewhere in the name of the constants, so it is clear that these are for logs. Something like LOG_SOURCE_STG for example

chombium · 2026-02-17T13:50:47Z

src/pkg/egress/syslog/filtering_drain_writer_test.go

 		Expect(err).To(HaveOccurred())
 	})
+
+	Context("when source_type tag is missing", func() {


Can a source type be missing for log envelopes?
Please read the Loggregator API spec for Log Envelopes.

chombium · 2026-02-17T14:18:21Z

src/pkg/egress/syslog/filtering_drain_writer_test.go

+			}
+		})
+
+		It("omits logs when source type include filter is configured with LOGS", func() {


This sounds odd: It omits logs when the include filter is configured.

Does the test test if the wanted logs are sent (included)? If an include filter is set , only these logs should be included

chombium · 2026-02-17T14:19:07Z

src/pkg/egress/syslog/filtering_drain_writer_test.go

+			Expect(fakeWriter.received).To(Equal(0))
+		})
+
+		It("sends logs when source type exclude filter is configured with LOGS", func() {


The same as above. The naming of the test is odd.

chombium · 2026-02-17T14:30:19Z

src/pkg/egress/syslog/source.go

+}
+
+// AllSourceTypes returns all valid source types
+func AllSourceTypes() []SourceType {


Where is this function used?

chombium · 2026-02-17T14:38:50Z

src/pkg/ingress/bindings/binding_config.go

+	set := make(syslog.SourceTypeSet, len(sourceTypes))
+
+	for _, sourceType := range sourceTypes {
+		sourceType = strings.TrimSpace(sourceType)


Do not trim, leading and trailing space will be url encoded as %20 or +. Nevertheless, if there is a space in the url that will be misformed url, an invalid Syslog Drain in our case.

chombium · 2026-02-17T14:41:39Z

src/pkg/ingress/bindings/binding_config.go

+	for _, sourceType := range sourceTypes {
+		sourceType = strings.TrimSpace(sourceType)
+		t, _ := syslog.ParseSourceType(sourceType)
+		set.Add(t)


What if a source type is invalid? Shouldn't that throw an error and mark the drain as invalid?

chombium · 2026-02-17T14:46:22Z

src/pkg/ingress/bindings/binding_config.go

 }

+func (d *DrainParamParser) getLogFilter(u *url.URL) *syslog.LogFilter {
+	includeSourceTypes := u.Query().Get("include-source-types")


IMO it would be better if we add log in the names of the parameters, so that they have clear descriptive names. Maybe something like include-log-source-types and exclude-log-source-types will be better...

chombium · 2026-02-17T14:52:27Z

src/pkg/ingress/bindings/binding_config_test.go

+			},
+			{
+				name:     "include-source-types=app",
+				url:      "https://test.org/drain?include-source-types=app",


it should be types=APP. The urls are case sensitive. As the values of the source types are defined uppercase, I'm not sure if lower case should be accepted for the values.

cf-foundation-community-automation bot added this to Application Runtime Platform Working Group Jan 5, 2026

cf-foundation-community-automation bot moved this to Inbox in Application Runtime Platform Working Group Jan 5, 2026

jorbaum added 4 commits January 8, 2026 10:41

Add log type include filter

c100636

Support exclude and include

d21490c

Error when using both include and exclude types

4fb3117

Introduce logging to binding config

dcc096a

jorbaum force-pushed the add-log-type-filter branch from 43b0941 to 9fcc372 Compare January 8, 2026 09:41

Add some more unit tests

bccd92c

jorbaum force-pushed the add-log-type-filter branch from 9fcc372 to bccd92c Compare January 8, 2026 09:43

chombium requested changes Jan 9, 2026

View reviewed changes

Fix wrong word

f5b43ae

jorbaum added 2 commits January 12, 2026 16:43

Create LogType as string, move to package

8487400

Prefix log type constants with LOG_

708301d

jorbaum commented Jan 12, 2026

View reviewed changes

jorbaum added 4 commits January 13, 2026 10:11

Rename value to sourceType

abaf216

Only emit one warning on wrong drain config

b78377d

Rename DrainParameterParser in test

2029984

Add tests for the include and exclude filters

6425efd

jorbaum commented Jan 13, 2026

View reviewed changes

jorbaum added 3 commits January 19, 2026 16:21

Move new log filter test to table test

20364ed

Move next test to table

c98468d

Move drain data for old parameter test to table

5614b36

jorbaum added 2 commits February 3, 2026 13:30

Move log filter validation to binding_fetcher

5e102fd

Dereference pointer to make an actual copy

56b93f2

I noticed that as my changes depend on `RawQuery` not being empty, but actually they were.

jorbaum commented Feb 4, 2026

View reviewed changes

src/pkg/egress/syslog/log.go Outdated Show resolved Hide resolved

jorbaum force-pushed the add-log-type-filter branch from 382854c to 21d9380 Compare February 11, 2026 14:24

Rename log type to source type where possible

e4d6fb6

jorbaum force-pushed the add-log-type-filter branch from 21d9380 to 108740d Compare February 11, 2026 14:26

jorbaum added 2 commits February 11, 2026 15:33

Only exclude filter shall pass unknown source type

5f4105e

Also rename remaining instances of log type to source type

Add SourceType PROXY, HEALTH, SYS and STATS

ef16bed

jorbaum force-pushed the add-log-type-filter branch from 108740d to ef16bed Compare February 11, 2026 14:33

jorbaum marked this pull request as ready for review February 16, 2026 10:18

jorbaum requested a review from a team as a code owner February 16, 2026 10:18

This was referenced Feb 16, 2026

Document new log-source-type syslog drain config cloudfoundry/docs-dev-guide#576

Merged

Document more source types cloudfoundry/docs-dev-guide#577

Draft

chombium requested changes Feb 17, 2026

View reviewed changes

Conversation

jorbaum commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Testing performed?

Checklist:

Uh oh!

chombium left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorbaum Jan 12, 2026 • 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!

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!

Uh oh!

Uh oh!

Uh oh!

chombium commented Jan 9, 2026

Uh oh!

jorbaum commented Jan 12, 2026

Uh oh!

jorbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jorbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jorbaum commented Jan 19, 2026

Uh oh!

Uh oh!

chombium left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

jorbaum commented Jan 5, 2026 •

edited

Loading

jorbaum Jan 12, 2026 •

edited

Loading