To be honest I disagree with the way we do sanitizing here and would …

[Se7soz]

To be honest I disagree with the way we do sanitizing here and would even remove it as graphite supports many special characters and it depends on the developer to choose better naming conventions to his/her metrics .. when I upgraded the library to use the new version all my dashboards didn't work as it uses '.' in metric names which in this case has been converted to '-' developers should be able to track metrics in the following formats (e.g., search.total, search.error, search.done) and as soon as these characters are fine with graphite then why would we need to strip them or normalize them?! .. I just added some of the special characters that I'm sure are accepted by graphite and I think we should support whatever graphite supports to be backward compatible .. especially when users upgrade to the new version

[Se7soz]

If we are going to leave the sanitization happening in metrics .. so I think it should be fare to allow developers pass the ACC characters in the GraphiteReporter .. Users don't have to change their WORKING dashboards when they upgrade to a newer version .. all the changes should cope with previous versions and allow ways to control the logic being done.

[ryanrupp]

Replacing dots with a dash does seem like a bug here as Graphite uses dot delimited naming sequence for path segments (and changing these to slashes would break wildcard queries and such that are across the path segment, see render API note about wildcards/path segments here.

I'm not sure which Graphite sender you're using but it looks like the sanitize method is protected so you can override it in the meantime if you're configuring the reporter directly.

Side note but I also noticed the TCP based graphite sender wasn't actually updated to use this new sanitizer, see here. Going back to the change I think it came up because of other characters like apostrophes mentioned here - #637 - but yeah I don't think periods should be replaced.

[ryanrupp]

Also the value gets sanitized the same way so I'm guessing that creates a problem for decimals here for instance :)

[arteam]

I agree with @ryanrupp and will put this pull request on hold, unless we have an evidence the sanitizing is broken.

Side note but I also noticed the TCP based graphite sender wasn't actually updated to use this new sanitizer

Thanks! I will submit a PR to address that.

[Se7soz]

@arteam I'm not sure if we really need the sanitize functionality .. that means we need users of our library to override the sanitize method to remove special characters they have declared? may be I'm missing something .. would you please clarify the main purpose of having it .. also graphite works fine with at least the characters I put in this PR .. please let me know :)

[ryanrupp]

@arteam to clarify my comment I think the present behavior released in 3.2 has a bug so this PR may be valid (or at least some sort of changes so dots in metric names do not get replaced with slashes). Specifically, this test case here is invalid. It should leave the periods alone e.g.:

softly.assertThat(GraphiteSanitize.sanitize("Foo.bar.baz", '-')).isEqualTo("Foo.bar.baz");

I haven't tested this locally but the values (in addition to names) are getting sanitized too so:

12.34 --> 12-34

which would be bad. Basically dots/periods should be left alone as they're expected to be used in a metric name (in both metric conventions and graphite conventions). The only time it would make sense to sanitize a period would be if it was within one of the "components" of a metric name e.g.:

// Generates a String foo.bar.baz
// Debatable if it's really foo.bar-baz in Graphite land
MetricRegistry.name("foo", "bar.baz");

However with the 3.x API in Metrics that cannot be detected (since a String is just being produced).

[arteam]

Thanks for clearing this up. I will try to test 3.2.0 with on a real Graphite installation.

[nickbabcock]

(Original feature implementer here): Oof, that does seem like a pretty bad bug!

My intent was for the sanitize feature to be used on individual parts of a named metric. Akin to:

MetricRegistry.name("A", "B", sanitize("St. Foo's of Bar"))

This is especially important with IPs/hosts:

MetricRegistry.name("A", "B", sanitize("127.0.0.1"))

One would expect: "A.B.127-0-0-1"

EDIT: Maybe bring back the whitespace remover from here and allow user to GraphiteSanitize#sanitize for individual metric names.

EDIT: This looks to affect all graphite metrics except those transmitted over TCP

[arteam]

I think the best remedy is too fallback to removing whitespaces how we do in the Graphite TCP sender.
This should remove the bug and make all senders consistent.

[Se7soz]

Closing this one then ;)

[arteam]

Metrics 3.2.1 has been released with the less aggressive sanitizer.