Allow filtering metrics using labels #77
Conversation
This will allow for a more elegant solution to hashicorp/consul#4042
There was a problem hiding this comment.
This looks great to me!
I have a couple of super nitpicky -comment only change requests but i think they are important enough to warrant actually requesting the change 😄 .
I'd like someone else familiar with the code base to review too. I'll try to get that organised 🔜 .
metrics.go
Outdated
| @@ -146,20 +172,54 @@ func (m *Metrics) UpdateFilter(allow, block []string) { | |||
| } | |||
| } | |||
|
|
|||
| // labelIsAllowed return true if a label has to be included | |||
There was a problem hiding this comment.
nitpick "should be included" rather than "has to be included" seems to describe the intention better.
Also, can we document that this must only be called internally by a method that holds m.filterLock in at least Read mode? It is as far as I can see now but it's good to document preconditions like that.
There was a problem hiding this comment.
DONE in next patch
metrics.go
Outdated
| } | ||
|
|
||
| // filterLabels return only allowed labels | ||
| func (m *Metrics) filterLabels(labels []Label) []Label { |
There was a problem hiding this comment.
Same comment about requiring caller holds lock.
There was a problem hiding this comment.
DONE in next patch
start.go
Outdated
| filter *iradix.Tree | ||
| allowedLabels map[string]int | ||
| blockedLabels map[string]int | ||
| filterLock sync.RWMutex |
There was a problem hiding this comment.
nitpick: I'd comment that all of filter, allowedLabels, blockedLabels are protected by filterLock as that's not implied by the name (no need to rename IMO just worth clarifying).
There was a problem hiding this comment.
DONE in next patch
|
@banks Suggestion to go forward with thoses patches ? Kind Regards |
|
I think @kyhavlov had some suggestions he was going to make here. We’ve had a busy couple of weeks but I’ll ping him today to take a look. |
|
Could someone have a look, so I can continue on hashicorp/consul#4042 ? Kind regards |
|
Looks good to me 👍 |
|
Hey @pierresouchay sorry for the delayed response. I think the idea behind this seems sound, but it feels weird to be blocking labels globally by name without it being attached to series name at all. Do you think it makes sense to make the label filters specific to a given series name for more fine-grained blocking? |
|
@kyhavlov Well for our use-case (aka adding labels in Consul everywhere), it will be more pain than ease, on a API point of view, it make more sense. What would you like me to do ?
Tell me what you want in that case. Kind regards |
There was a problem hiding this comment.
I talked about this a little more with @banks and we agreed it should be ok to merge as-is so long as we document the feature as a "global label filter" and make sure its function is clear. I had a couple really minor comments, and other than that it looks good 👍
start.go
Outdated
| @@ -23,17 +23,21 @@ type Config struct { | |||
|
|
|||
| AllowedPrefixes []string // A list of metric prefixes to allow, with '.' as the separator | |||
| BlockedPrefixes []string // A list of metric prefixes to block, with '.' as the separator | |||
| AllowedLabels []string // A list of metric labels to allow, with '.' as the separator | |||
| BlockedLabels []string // A list of metric prefixes to block, with '.' as the separator | |||
There was a problem hiding this comment.
Should be A list of metric labels...
start.go
Outdated
| lastNumGC uint32 | ||
| sink MetricSink | ||
| filter *iradix.Tree | ||
| allowedLabels map[string]int |
There was a problem hiding this comment.
minor nitpick, but this could be map[string]bool or map[string]struct{} when it's just being used as a set
|
@kyhavlov Thank you for the review, PR updated with requested fixes |
|
I made the fixes, would you consider merging it so I can go forward on hashicorp/consul#4042 ? Thanks a lot Kind regards |
There was a problem hiding this comment.
Sorry @pierresouchay for the death march this turned into.
I think I now have access to merge this, but just noted that the suggestion for documenting the globalness of label filtering wasn't done yet. I think this is a reasonable trade off for not actually implementing something more complex for now.
I don't think there are any actual docs other than Godoc to update so just the comment on the method.
Thanks for persevering here!
| m.UpdateFilterAndLabels(allow, block, m.AllowedLabels, m.BlockedLabels) | ||
| } | ||
|
|
||
| // UpdateFilterAndLabels overwrites the existing filter with the given rules. |
There was a problem hiding this comment.
Can we make this doc comment explicit what the behaviour of label filter is? i.g. apply globally to all metrics and apply orthogonally to the allow/block prefixes. I'd also include the use-case for them (globally configuring whether potentially high-cardinality metrics are included) to make it obvious why they work like this.
I think that satisfied the last point Kyle mentioned.
There was a problem hiding this comment.
@banks DONE: I added more text in README.md as well
This will allow for a more elegant solution to
hashicorp/consul#4042
TODO: