Removed metric name validation when using a CustomSampleMappingBuilder#519
Removed metric name validation when using a CustomSampleMappingBuilder#519LiamClarkeNZ wants to merge 1 commit intoprometheus:mainfrom
Conversation
…r to allow usage on arbitrarily named metrics Signed-off-by: Liam Clarke <liam.clarke@adscale.co.nz>
f226c1d to
a2e759f
Compare
|
No validation at all might be a bit far in the other direction, what does the graphite exporter do? |
|
Hi @brian-brazil, After looking into it and running into the same issues as @LiamClarkeNZ I feel like the whole validation pattern is not necessary. If one would want to validate something, then it would be if the "regular expression pattern" is useful. For example not allowing my.metric.**.something as ** would resolve to two regex groups with the second being empty. I think the intention was to make sure the metric name follows a specific "graphite" pattern, but this would only make sense after all mappings have been applied, not on the match string. Removing it sounds fine to me and I suggest renaming the GraphiteNamePattern class to something else without "Graphite". |
|
What does the graphite exporter do? Whatever we have here should be in line with that, and if that's not handling these cases correctly we'll need to fix that too. |
|
I can answer that, sorry, lost interest in this, ended up hand-rolling my own variant that allowed for much more flexibility, am considering submitting a pull request, but being honest, given the history of my attempts at pull requests on Prometheus projects, not overly enthused. The Graphite bridge believes that a valid metric name matches the following regex character class: Far more lenient than the Dropwizard code. My 2c is that the Dropwizard code shouldn't be trying to enforce metric names in a class that's intended to remap existing metric names, hence removing all validation, because why does a class specifically designed for remapping to Prometheus valid metric names care what your previous crazy metric name was. |
So you're saying it accepts everything? |
The current implementation enforces Graphite metric naming patterns, but not all usages of Dropwizard metrics use these patterns, so I've removed the validation to allow a more widespread use.