Skip to content

Add log type include filter#714

Open
jorbaum wants to merge 28 commits intocloudfoundry:mainfrom
jorbaum:add-log-type-filter
Open

Add log type include filter#714
jorbaum wants to merge 28 commits intocloudfoundry:mainfrom
jorbaum:add-log-type-filter

Conversation

@jorbaum
Copy link
Contributor

@jorbaum jorbaum commented Jan 5, 2026

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

Copy link
Contributor

@chombium chombium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorbaum Thanks for the changes.

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

@chombium
Copy link
Contributor

chombium commented Jan 9, 2026

@ZPascal Wdyt about this PR? It addresses #711

@jorbaum
Copy link
Contributor Author

jorbaum commented Jan 12, 2026

@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.

Copy link
Contributor Author

@jorbaum jorbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worked in some comments.

Copy link
Contributor Author

@jorbaum jorbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worked in most comments. Still 2 left to clarify.

@jorbaum
Copy link
Contributor Author

jorbaum commented Jan 19, 2026

Feel free to take another look at it.

@jorbaum jorbaum force-pushed the add-log-type-filter branch 3 times, most recently from 108740d to ef16bed Compare February 11, 2026 14:33
@jorbaum jorbaum marked this pull request as ready for review February 16, 2026 10:18
@jorbaum jorbaum requested a review from a team as a code owner February 16, 2026 10:18
Copy link
Contributor

@chombium chombium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jorbaum
Copy link
Contributor Author

jorbaum commented Feb 18, 2026

Thanks for the review. I replied to a bunch of comments for some clarifying remarks. Also a quick discussion about uppercase/lowercase config values would help me.

Copy link
Contributor Author

@jorbaum jorbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed hopefully all comments.

I got a bunch of things to clarify. Biggest question that is left is likely if we should recommend users uppercase or lowercase values.

@jorbaum jorbaum force-pushed the add-log-type-filter branch from ef16bed to ecab32a Compare February 20, 2026 13:38
Copy link
Contributor

@chombium chombium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorbaum It looks much better now and we're close to mergnig.
Please fix the small things which I've commented on.

I will have to test everything before merging.

}
})

It("filters out the log with missing source_type when include filter is configured", func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filter works based on the values of the source_type tag and here the code defines the behavior of what should happen.

Is this behavior documented somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it is not. The best place for that is likely https://docs.cloudfoundry.org/devguide/services/log-management.html ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the place. Please open a PR. Btw, why is the description of the parameters already in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the place. Please open a PR.

Opened up a new PR at cloudfoundry/docs-dev-guide#584

Btw, why is the description of the parameters already in the docs?

The previous PR has been merged a bit sooner than expected.

Copy link
Contributor Author

@jorbaum jorbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should all be addressed now. CI is failing due to new golangci-lint changes. Those have been addressed in the two newest commits of #717 .

}
})

It("filters out the log with missing source_type when include filter is configured", func() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the place. Please open a PR.

Opened up a new PR at cloudfoundry/docs-dev-guide#584

Btw, why is the description of the parameters already in the docs?

The previous PR has been merged a bit sooner than expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants