Graphite metrics should clean metrics names

[JensRantil]

Graphite does not allow metrics/sensors with names containing ', but Metrics happily submits them. This makes Graphite log a bunch of errors not being able to create the whisper files yada yada.

I see three different solutions to this:

• Patch Metrics to not submit these metrics if they have the wrong naming, logging the metrics with the bad naming.
• Automagically clean the sensor names (removing weird characters etc.) on submission to Graphite/carbon.
• Keep things the way they are. Possibly adding a static helper method that can be used to clean sensor names from garbage. I am currently wrapping a bunch of sensor names in:

public class MetricsUtils {

    /**
     * Clean graphite metrics names.
     * <p>
     * This was created because I was seeing a lot of stacktraces in Carbon log due to broken metric names.
     * 
     * @param proposal
     *            the proposed metrics' name.
     * @return cleaned metric's name
     */
    public static String cleanMetricName(String proposal) {
        return proposal.replace("'", "");
    }

}

Input welcome.

[ryantenney]

Are there characters other than ' which need to be sanitized from metric names?

[jmason]

Newlines for sure; they are used as a line separator in the text protocol: http://graphite.readthedocs.org/en/latest/feeding-carbon.html

I'd suggest sanitising anything that isn't healthy in a UNIX filename, so any control chars too.

[thattolleyguy]

Is there any status on the desired requirements of this issue? I'd love to help out, I'm just not sure what steps to take.

[felixbarny]

This is my static helper class. I got the regex somewhere from the graphite source code.
You can use and modify as you like. It's under Apache 2.0 license as its part of the stagemonitor project.

https://github.com/stagemonitor/stagemonitor/blob/master/stagemonitor-core/src/main/java/org/stagemonitor/core/util/GraphiteSanitizer.java

public final class GraphiteSanitizer {

    public static final Pattern DISALLOWED_CHARS = Pattern.compile("[^a-zA-Z0-9!#\\$%&\"'\\*\\+\\-:;<=>\\?@\\[\\\\\\]\\^_`\\|~]");

    private GraphiteSanitizer() {
    }

    /**
     * Graphite only supports alphanumeric characters + !#$%&"'*+-.:;<=>?@[\]^_`|~ so each metric name segment has to
     * be cleared from other chars.
     * <p/>
     * <pre>
     * metric path/segment delimiter
     *         _|_
     *        |   |
     * metrics.cpu.utilisation  <- metric name
     *    |     |      |
     *    -------------
     *          |
     *  metric name segments
     * </pre>
     *
     * @param metricNameSegment the metric name segment (see diagram above for a explanation of what a metric name segment is)
     * @return the metricNameSegment that contains only characters that graphite can handle
     */
    public static String sanitizeGraphiteMetricSegment(String metricNameSegment) {
        return DISALLOWED_CHARS.matcher(metricNameSegment.replace('.', ':').replace(' ', '-').replace('/', '|')).replaceAll("_");
    }
}

[thattolleyguy]

So do people think it would be better to remove the offending metrics or sanitize the names? Should we support both with an option to determine which? If so, what should the default be?

[JensRantil]

I believe the default should be to sanitize. There are few worse things than missing a metric when debugging.

On Fri, Dec 19, 2014 at 5:46 AM, Tyler Tolley notifications@github.com
wrote:

So do people think it would be better to remove the offending metrics or sanitize the names? Should we support both with an option to determine which? If so, what should the default be?

Reply to this email directly or view it on GitHub:
#637 (comment)

[JensRantil]

FYI, I just ran into a bug(?) triggered in Graphite when a metric had a parenthesis in it. Filename was /whisper_root/package/path(something)/1_mean.wsp. And I was plotting aliasByNode(package.*.1_mean, 1). We are now internally stripping all parenthesis.

[felixbarny]

Graphite only supports alphanumeric characters + !#$%&"'*+-.:;<=>?@[]^_`|~

[JensRantil]

@felixbarny Thanks! Do you have a reference for this list? As stated in the initial description, ' is broke my Graphite.

[felixbarny]

I've found it somewhere in Graphite's source code. Can't remember where though...

[ryantenney]

I've gone with the following regex: [^a-z0-9!#\$%&"'*+-.:;<=>\?@[]^_`|~]+

If there are no objections then I will be releasing 3.1.1 today.

[felixbarny]

I think the regex is missing the backslash character (\\)

[ryantenney]

So it is. Thanks!

[ryantenney]

Forward slash is legal then?

[felixbarny]

No, that's one of the most illegal chars ;)
But look at my sanitizeGraphiteMetricSegment method. I'm replacing it separately by | to that URLs don't look like 💩 . For example GET /index.html -> GET-|index:html as supposed to GETindexhtml or GET--index-html but thats a matter of taste I guess...

[nickbabcock]

With #936 merged, should this be closed?

[ryantenney]

Yep looks like it.