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

Query Filtering -- Querier #5482

Merged

Conversation

MasslessParticle
Copy link
Contributor

This is the first PR for query filtering for deletes. It adds a delete store to the querier. The querier uses that to lookup any delete request for the current user, finds the ones that overlap with the query range, and forwards them to downstream ingesters and stores.

This is implemented for both Logs and Metrics.

- Get deletes from the index gateway and attach them to store and ingester requests
for _, del := range d {
for _, selector := range del.Selectors {
if int64(del.StartTime) <= end && int64(del.EndTime) >= start {
deletes = append(deletes, &logproto.Delete{
Copy link
Contributor

Choose a reason for hiding this comment

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

In a future PR it would be nice to have this as a metrics somewhere so we can see if the different instances are in sync.

Copy link
Contributor

@MichelHollands MichelHollands left a comment

Choose a reason for hiding this comment

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

LGTM

@MasslessParticle MasslessParticle marked this pull request as draft March 4, 2022 14:32
@MasslessParticle
Copy link
Contributor Author

This relies on #5481, converting to a draft PR so I can make the necessary changes when it's merged

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

This is looking really good. When are you planning to remove draft?

@MasslessParticle
Copy link
Contributor Author

There's nothing technically stopping it. I think it's going to conflict with #5481 quite a bit, though.

@MasslessParticle MasslessParticle marked this pull request as ready for review March 11, 2022 18:01
@MasslessParticle
Copy link
Contributor Author

MasslessParticle commented Mar 11, 2022

@owen-d I proposed some some changes to the API PR above and I don't think it's going to interfere with this PR much at all. I think we're good to review/merge

@owen-d owen-d merged commit 88372c9 into grafana:main Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants