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

Removed metric name validation when using a CustomSampleMappingBuilder #519

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LiamClarkeNZ
Copy link

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.

…r to allow usage on arbitrarily named metrics

Signed-off-by: Liam Clarke <[email protected]>
@brian-brazil
Copy link
Contributor

No validation at all might be a bit far in the other direction, what does the graphite exporter do?

@peetzen
Copy link

peetzen commented Feb 1, 2020

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".

@brian-brazil
Copy link
Contributor

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.

@LiamClarkeNZ
Copy link
Author

LiamClarkeNZ commented Feb 13, 2020

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: [a-zA-Z0-9_-] - it then replaces every invalid character with an underscore. So dots will be converted to underscores, everything else will be left intact.

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.

@brian-brazil
Copy link
Contributor

So dots will be converted to underscores, everything else will be left intact.

So you're saying it accepts everything?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants