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.

Sign up for GitHub

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

Jump to bottom

Add filter parameter to rebound so lines can be deleted by the compactor #5879

Merged

sandeepsukhani merged 19 commits into grafana:main from MichelHollands:add_line_filtering_to_compactor

Apr 29, 2022

Contributor

MichelHollands commented Apr 12, 2022 •

edited

Loading

Add filter functionality to the delete functionality of the compactor. This is hidden behind the "filter-and-delete" value of the deletion_mode config setting.

A filter function is added to the intervals sent to the Rebound function. This removes lines that match the given DeleteRequest.

Checklist

Documentation added
Tests updated
Add an entry in the CHANGELOG.md about the changes.

Signed-off-by: Michel Hollands michel.hollands@grafana.com

pull-request-size bot added size/L size/XL and removed size/L labels

MichelHollands marked this pull request as ready for review

April 12, 2022 11:38

MichelHollands requested a review from a team as a code owner

April 12, 2022 11:38

MichelHollands closed this

MichelHollands reopened this

MichelHollands marked this pull request as draft

April 12, 2022 11:38

MichelHollands marked this pull request as ready for review

April 14, 2022 14:54

MasslessParticle reviewed

View reviewed changes

pkg/storage/stores/shipper/compactor/deletion/delete_request.go Outdated Show resolved Hide resolved

MasslessParticle reviewed

View reviewed changes

pkg/storage/stores/shipper/compactor/deletion/delete_request.go Outdated Show resolved Hide resolved

MasslessParticle reviewed

View reviewed changes

pkg/storage/stores/shipper/compactor/compactor.go Outdated Show resolved Hide resolved

MasslessParticle reviewed

View reviewed changes

pkg/loki/modules.go Outdated Show resolved Hide resolved

MasslessParticle reviewed

View reviewed changes

cmd/loki/loki-docker-config.yaml Outdated Show resolved Hide resolved

MasslessParticle requested changes

View reviewed changes

Contributor

MasslessParticle left a comment

This looks awesome. I think there are just a couple of QOL and efficiency changes

MichelHollands force-pushed the add_line_filtering_to_compactor branch from 89ce10e to 6fa08a4 Compare

April 19, 2022 09:40

MichelHollands changed the title ~~Add filter parameter to rebound~~ Add filter parameter to rebound so lines can be deleted by the compactor

MichelHollands requested a review from MasslessParticle

April 19, 2022 10:18

MasslessParticle approved these changes

View reviewed changes

Contributor

MasslessParticle left a comment

A nit about commented out code and a question about when to add the endpoints.

Otherwise LGTM

pkg/storage/stores/shipper/compactor/deletion/validation_test.go Outdated Show resolved Hide resolved

pkg/loki/modules.go Show resolved Hide resolved

MichelHollands added 11 commits

April 22, 2022 09:01


          Add filter parameter to rebound

a312280

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Fix linting issues

0f62656

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Add filter function to delete request

5c370d7

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Fix linting issues

5906acc

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Enable api for filter and delete mode

f4b8060

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Add settings for retention

c460772

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Use labels to check and add test

3f8144e

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Simplify filter function

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Also enable filter mode

810363e

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Remove test settings in config file for docker

3b9dea1

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Add extra (unused) param for ProcessString in filter

94ab384

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

MichelHollands added 4 commits

April 22, 2022 09:05


          Empty commit to trigger CI again

668edba

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Update changelog

f4704f6

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Fix flapping test

e3bc524

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Remove commented out unit tests and add some more

48166fb

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

MichelHollands force-pushed the add_line_filtering_to_compactor branch from 79636d6 to 48166fb Compare

April 22, 2022 08:07

sandeepsukhani reviewed

View reviewed changes

Contributor

sandeepsukhani left a comment

Overall changes look good to me. Left a few minor suggestions.

pkg/storage/stores/shipper/compactor/deletion/delete_request.go Show resolved Hide resolved

pkg/storage/stores/shipper/compactor/deletion/delete_request_test.go Show resolved Hide resolved

pkg/storage/stores/shipper/compactor/retention/retention.go Outdated Show resolved Hide resolved

MichelHollands added 2 commits

April 26, 2022 08:40


          Add extra test case for delete request without line filter

c49b283

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          Use chunk bounds

414eb48

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

sandeepsukhani reviewed

View reviewed changes

Contributor

sandeepsukhani left a comment

Sorry, I missed a case and found a small problem. Can you please take care of that as well? Please let me know if it is not clear or if I got something wrong.

pkg/storage/stores/shipper/compactor/deletion/delete_request.go Show resolved Hide resolved

MichelHollands added 2 commits

April 28, 2022 09:28


          check if the log selector has a filter if the whole chunk is selected

a840002

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>


          fix lint issue: use correct go-kit import

6f7ba86

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

sandeepsukhani approved these changes

View reviewed changes

Contributor

sandeepsukhani left a comment

Just some very minor non-blocking nits.
Nice work, the changes look great!

pkg/storage/stores/shipper/compactor/deletion/delete_request_test.go

+              					tc.expectedResp.nonDeletedIntervals[idx].Interval.End,
+              					nonDeletedIntervals[idx].Interval.End,
+              				)
+              			}

Contributor

sandeepsukhani Apr 29, 2022

Let us also add a nil vs non nil check when the delete request is with lblWithFilter?

pkg/util/filter/filter_function.go

		@@ -0,0 +1,3 @@
		package filter

		type Func func(string) bool

Contributor

sandeepsukhani Apr 29, 2022

Is it possible to either move this somewhere relevant like chunks or deletion package? It looks out of place to me.
The other option is to add more context to it if moving it is not possible due to circular dependency.

sandeepsukhani merged commit 477a0e7 into grafana:main

MichelHollands mentioned this pull request

Add comments to the Func definition #6056

Closed

4 tasks

This was referenced Jun 28, 2022

dannykopping/remove cache stats dannykopping/loki#13

Closed

dannykopping/remove cache stats dannykopping/loki#14

Draft

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

Labels