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

Graphite: Replace names with non-alphanumeric chars with '-' #936

Merged
merged 1 commit into from Apr 11, 2016

Conversation

Projects
None yet
4 participants
@nickbabcock
Contributor

nickbabcock commented Apr 11, 2016

Fix for #637, that supersedes #731

The fix won't allow any characters that will interfere with Graphite's display or functions.

Instead of using fancy regex, this PR uses code ported from Guava's CharMatcher and Apache's StringUtils.

This PR was made against master because this may break other's graphite metric names, even if some of them aren't 100% valid (eg. foo:bar will be logged fine, but some graphite functions will break). Any potential breakage should be able to be fixed through whisper-merge. If this is wanted for 3.2.0, I'd be more than happy to send another PR against that.

@ryantenney ryantenney merged commit cade4ad into dropwizard:master Apr 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ryantenney ryantenney added this to the 4.0.0 milestone Apr 11, 2016

@ryantenney

This comment has been minimized.

Member

ryantenney commented Apr 11, 2016

Thank you! I've just created the branch 3.2-development, and it'd be great to see this make it into 3.2 as well.

@ArturGajowy

This comment has been minimized.

ArturGajowy commented May 8, 2018

FYI this change has been reverted by #1099, sadly without referencing this PR

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented May 8, 2018

Yup, it was reverted because it broke everything 😄

Things may be different now, and it may now be possible to sanitize individual portions of a metric name -- but I haven't checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment