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

Jump to bottom

Implement deletion based on partially matching labels #1013

Merged
merged 14 commits into from Apr 21, 2022
Merged

Implement deletion based on partially matching labels #1013

merged 14 commits into from Apr 21, 2022

Conversation

stone-z
Copy link
Contributor

@stone-z stone-z commented Mar 25, 2022 •
edited

Towards #834. This is an approach which I think feels user-friendly and accomplishes what folks were asking for. Open to suggestions of course, I'm new here.

This PR introduces a new deletion method:

  • DeletePartialMatch(labels Labels) - deletes a metric if all of the given labels and associated values are present in that metric.

It returns the number of metrics which were deleted.

@stone-z
WIP partial match
34486f9 
Signed-off-by: Zach Stone <zach@giantswarm.io>
@stone-z
Cleanup
b8653c0 
Signed-off-by: Zach Stone <zach@giantswarm.io>
@stone-z
Comments
0820be3 
Signed-off-by: Zach Stone <zach@giantswarm.io>
@stone-z
Tests and comments
387c4f0 
Signed-off-by: Zach Stone <zach@giantswarm.io>
@stone-z
Handle properly deleting multiple metrics, update tests
fa775cf 
Signed-off-by: Zach Stone <zach@giantswarm.io>
@stone-z
Comments
9a8cb32 
Signed-off-by: Zach Stone <zach@giantswarm.io>
@stone-z stone-z mentioned this pull request Mar 25, 2022
Closed
bwplotka
bwplotka reviewed Mar 27, 2022
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice! It looks quite good to me however, I personally did not have a code that needs functionality like this. This is why it would be amazing to have opinions from potential users: @taiyang-li @pstibrany

I added a small comment though, I think the DeletePartialMatchLabelValues(values..) semantics is too open. I wonder if DeletePartialMatchLabelValues(name, values ...) make more sense (:

prometheus/vec.go Outdated Show resolved Hide resolved
@stone-z stone-z mentioned this pull request Mar 28, 2022
Closed
@stone-z
Try using curry values
7e947b6 
Signed-off-by: Zach Stone <zach@giantswarm.io>
@stone-z
Skip curry value to demo
e79ef55 
Signed-off-by: Zach Stone <zach@giantswarm.io>
stone-z
stone-z commented Mar 28, 2022
View reviewed changes
prometheus/vec_test.go Outdated Show resolved Hide resolved
beorn7
beorn7 reviewed Mar 30, 2022
View reviewed changes
prometheus/vec.go Outdated Show resolved Hide resolved
@stone-z
Fix curry deletion, remove outdated comment.
5514642 
Signed-off-by: Zach Stone <zach@giantswarm.io>
@stone-z
Fix logic for deletion of metrics from prior to currying
e56280b 
Signed-off-by: Zach Stone <zach@giantswarm.io>
beorn7
beorn7 requested changes Apr 5, 2022
View reviewed changes
prometheus/vec_test.go Outdated Show resolved Hide resolved
prometheus/vec_test.go Outdated Show resolved Hide resolved
prometheus/vec_test.go Outdated Show resolved Hide resolved
prometheus/vec_test.go Outdated Show resolved Hide resolved
prometheus/vec_test.go Outdated Show resolved Hide resolved
prometheus/vec_test.go Outdated Show resolved Hide resolved
@taiyang-li
Copy link

Nice! It looks quite good to me however, I personally did not have a code that needs functionality like this. This is why it would be amazing to have opinions from potential users: @taiyang-li @pstibrany

I added a small comment though, I think the DeletePartialMatchLabelValues(values..) semantics is too open. I wonder if DeletePartialMatchLabelValues(name, values ...) make more sense (:

This is the exact function I needed .

@stone-z
Don't match curried values. Update tests.
27d7f47 
Signed-off-by: Zach Stone <zach@giantswarm.io>
@stone-z
Remove unneccesasry helper and todo comments
89fe389 
Signed-off-by: Zach Stone <zach@giantswarm.io>
@stone-z
Comment about partial matching curried labels
6533de3 
Signed-off-by: Zach Stone <zach@giantswarm.io>
@stone-z
Copy link
Contributor Author

stone-z commented Apr 11, 2022

Ok, I've updated this PR with what I think @beorn7 had in mind. I am open to returning an error if a user passes a curried label to DeletePartialMatch, but I checked and this would actually be different from Delete and DeleteLabelValues, which just return booleans. So I'm happy with this if the maintainers are as well. Thanks for your patience 🙏

@beorn7
Copy link
Member

beorn7 commented Apr 13, 2022

I'm aware, and this is on my review queue, but the queue is very long and progressing slowly. I'll get to this eventually, but feel free to make the call without me.

bwplotka
bwplotka approved these changes Apr 13, 2022
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thanks 💪🏽

Great!

@bwplotka
Copy link
Member

bwplotka commented Apr 13, 2022 •
edited

I think it's safe to merge as the API got smaller and very sensible, but waiting for your eyes @beorn7 if you want to check it still. I am happy with this (:

@stone-z
Simplify curried value check
8168be3 
Signed-off-by: Zach Stone <zach@giantswarm.io>
beorn7
beorn7 approved these changes Apr 19, 2022
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Semantics looks OK to me now.

kakkoyun
kakkoyun approved these changes Apr 21, 2022
View reviewed changes
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM

@kakkoyun kakkoyun merged commit 4dcf02e into prometheus:main Apr 21, 2022
@stone-z stone-z deleted the delete-partial-match branch April 21, 2022 07:51
@pracucci pracucci mentioned this pull request Apr 27, 2022
Closed
@stone-z stone-z mentioned this pull request Jun 8, 2022
Merged
3 tasks
@GMHDBJD GMHDBJD mentioned this pull request Sep 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kakkoyun kakkoyun kakkoyun approved these changes

@beorn7 beorn7 beorn7 approved these changes

@bwplotka bwplotka bwplotka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@stone-z @taiyang-li @beorn7 @bwplotka @kakkoyun