Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Aggregator] Support setting consumer service filters dynamically via topic updates #4297

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

adamjeanlaurent
Copy link

What this PR does / why we need it:

Adds support for setting consumer services filters at runtime via topic updates.

Previously the only way to add / remove / edit consumer servicesfilters is via the aggregator's static configuration, meaning a deployment is needed.

This change introduces the following new features,

  1. Ability to include consumer services filters in the topic definition, so if you want to add/remove/change consumer service filters in a topic during runtime, all you need to do is send a topic update.
  2. Add richer metrics for the consumer service filters, so that we can know what filters are being used and their configuration source.

I am not removing the ability to statically configure consumer service filters, as that would be a breaking change. In this new world, dynamic filter configuration overrides static filter configuration. If dynamic filter configuration is non-nil, whatever static filters exist on the CSW are overriden.

Code Changes:

  1. Update topic Protobuf definitions to include definitions for Percentage Filter, Shard Set Filter, and Storage Policy filter.
  2. Update consumer service glue code to parse and expose the new filter data from protos.
  3. Update code that process topic updates to set dynamic filters on consumer service writer.
  4. Add new metrics "filter-accept-granular" and "filter-not-accept-granular" that emit a metric every time a CS filter is used and has tags for config source (dynamic vs. static), filter type (percentage, shard set, etc.), and consumer service name, env info.
  5. Add unit tests for all of the above.

Notes for reviewer

writer_test.go is a good place to start as it has tests for the topic update flow.

Does this PR introduce a user-facing and/or backwards incompatible change?:

  TODO: write release note.....

Does this PR require updating code package or user-facing documentation?:

  TODO: update documentation?

@adamjeanlaurent adamjeanlaurent added the area:aggregator All issues pertaining to aggregator label Sep 27, 2024
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@adamjeanlaurent adamjeanlaurent changed the title [Aggregator] Support setting consumer services dynamically via topic updates [Aggregator] Support setting consumer service filterss dynamically via topic updates Sep 27, 2024
@adamjeanlaurent adamjeanlaurent changed the title [Aggregator] Support setting consumer service filterss dynamically via topic updates [Aggregator] Support setting consumer service filters dynamically via topic updates Sep 27, 2024
}
} else {
// sending no dynamic filters means we should remove all filters, unless there are static filters
// if there are no static filters, remove all filters
Copy link
Author

Choose a reason for hiding this comment

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

actually probably makes sense to instead unregister filters, then apply static filters if there are any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:aggregator All issues pertaining to aggregator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants