client_java icon indicating copy to clipboard operation
client_java copied to clipboard

#342 - Implemented Label Value Sanitizer

Open Tigraine opened this issue 3 years ago • 2 comments
trafficstars

@brian-brazil as discussed (a few years ago 🤣 ) here https://github.com/prometheus/client_java/issues/342

This PR adds the ability to all SimpleCollector instances to manipulate labels before they are processed. This allows users to extend a label to be runtime safe or do manipulation like removing credentials etc..

Example would be:

String clientName = getClientName(); // this label is not null-safe
counter.label(clientName).inc() <-- will throw IllegalArgumentException only during runtime

Usage:

metric = Gauge.build()
            .name("nonulllabels")
            .help("help")
            .labelNames("l")
            .labelValueSanitizer(Gauge.TransformNullLabelsToEmptyString())
            .register(registry);

Instead of having to sanitise all labels before calling the instrumentation code we can make the code safe at setup and not care about downstream mis-use. Also since this is implemented through an interface additional use cases can be passed by consumers of the library.

Tigraine avatar Apr 22 '22 13:04 Tigraine

Regarding the specific case of null values: If we revisit this and not throw an IllegalArgumentException, what do you think would be a good default value? The literal string "null" as returned by String::valueOf, or an empty value?

fstab avatar May 02 '22 11:05 fstab

Regarding the specific case of null values: If we revisit this and not throw an IllegalArgumentException, what do you think would be a good default value? The literal string "null" as returned by String::valueOf, or an empty value?

Thanks for the feedback! I will update the PR to move to a Transformer as it makes a lot of sense.

I was thinking about the appropriate default value a bit and in our case 'NULL' would be a perfectly good default value - but that might not be for everyone as a default which is the main reason I came up with this PR and the concept of Sanitization.

Tigraine avatar Jun 02 '22 09:06 Tigraine