search

rcrowley / go-metrics

Go port of Coda Hale's Metrics library
Other
3.43k stars 493 forks source link

ChildRegistry should filter by prefix #183

Closed splittingfield closed 7 years ago

splittingfield commented 7 years ago

This commit changes the behavior of PrefixedChildRegistry to only apply the Each function to registered metrics that have the the appropriate prefix.

This commit contains two changes: 1: An originally failing test showing the behavior 2: A wrapper func around the passed in function for Each that applies the required filter

The behavior that this addresses was quite unexpected, but happy to discuss if this was the intended behavior.

splittingfield commented 7 years ago

This new addition allows PrefixedRegistries to have a prefixedRegistry as the base registry and then computes the necessary prefixes.

mihasya commented 7 years ago

@splittingfield I think I get it, but would appreciate a "here's what happened before and why it's messed up" explanation. I'm only able to do drive-by reviews these days, so any and all context helps.

splittingfield commented 7 years ago

@mihasya Sorry, I thought the failing test would have made it clear, as originally it failed until I added my changes.

Core issue was when you called Each on a prefixed registry, it was applied to ALL registered metrics, not just the one with the appropriate prefix. The second change then makes it so that if you create a PrefixedRegistry from a PrefixedRegistry, it would still behave properly. (The first fix only allowed a PrefixedRegistry with an underlying StandardRegistry to work)

splittingfield commented 7 years ago

Please note that this in fact a breaking change if people were relying on (what I at least consider to be broken) behavior around PrefixedRegistry.Each looping over all metrics.

splittingfield commented 7 years ago

@mihasya I am not certain you want to take it, but I also have a ServeHttp function that walks a prefixed registry and generates JSON without having to sync expvars (avoiding that entire handler). It makes life easier when you just want to report via a URL a subset of metrics as opposed to all of them

mihasya commented 7 years ago

2 minor comments, after that good to merge.

splittingfield commented 7 years ago

Thanks! This comments have been addressed.