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.

Sign up for GitHub

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

#342 - Implemented Label Value Sanitizer #776

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

#342 - Implemented Label Value Sanitizer #776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions simpleclient/src/main/java/io/prometheus/client/LabelValueSanitizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package io.prometheus.client;

public interface LabelValueSanitizer {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you say this can also be used for other use cases, not just "sanitization". So maybe the interface should have a more generic name covering all use cases, like LabelValueTransformer?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @fstab ... LabelValueTransformer seems like it could be used more generically.

Copy link
Collaborator

@dhoard dhoard Jul 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be future use cases/places where the paradigm could be used to transform other (non-label) String values, so I feel it would make more sense to rename the interface to StringTransformer.

String[] sanitize(String... labelValue);
Copy link
Member

@fstab fstab May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to sanitize only a single label value, not the whole array. For example, you could register a sanitizer like this:

metric = Gauge.build()
        .name("nonulllabels")
        .help("help")
        .labelNames("path", "status")
        .labelValueSanitizer("path", myPathSanitizer)
        .register(registry);

and then the myPathSanitizer would only be called for the path label value, but not for an array of all label values.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since labelNames takes a String array, I think it makes sense for the class to manipulate the labels also takes a String array.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on what use case you have in mind.

What I have in mind is that you have a specific label that you want to transform (for example stripping a unique user ID from an HTTP path). For that, it would be good to implement a specific pathTransformer and apply it to the path label. Advantage is: You don't need to magically know at which position in the array the path label is. And if path is passed as a label to multiple different metrics, you can reuse the transformer, even if the path is on different positions for each metric.

The array as parameter only makes sense if you want to do exactly the same transformation for all labels. This would work for replacing null with "null", but for every other use case I think it's better if you know which label you want to transform.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both might actually be interesting.
Maybe having a Transformer be applied to a specific label or * to apply to all?
I could for example think of a case where you'd want to have your infrastructure automatically prevent users from using excessively long labels (truncating them automatically).

}
45 changes: 45 additions & 0 deletions simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public abstract class SimpleCollector<Child> extends Collector {
protected final String help;
protected final String unit;
protected final List<String> labelNames;
protected final LabelValueSanitizer labelValueSanitizer;

protected final ConcurrentMap<List<String>, Child> children = new ConcurrentHashMap<List<String>, Child>();
protected Child noLabelsChild;
Expand All @@ -64,6 +65,7 @@ public Child labels(String... labelValues) {
if (labelValues.length != labelNames.size()) {
throw new IllegalArgumentException("Incorrect number of labels.");
}
labelValues = labelValueSanitizer.sanitize(labelValues);
for (String label: labelValues) {
if (label == null) {
throw new IllegalArgumentException("Label cannot be null.");
Expand Down Expand Up @@ -174,12 +176,42 @@ protected SimpleCollector(Builder b) {
for (String n: labelNames) {
checkMetricLabelName(n);
}
labelValueSanitizer = b.labelValueSanitizer;

if (!b.dontInitializeNoLabelsChild) {
initializeNoLabelsChild();
}
}

/**
* Default Sanitizer - Will not perform any manipulation on labels
*/
static LabelValueSanitizer NoOpLabelValueSanitizer() {
return new LabelValueSanitizer() {
@Override
public String[] sanitize(String... labelValue) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change labelValue to labelValues since it's an array.

return labelValue;
}
};
}

/**
* Transforms null labels to an empty string to guard against IllegalArgument runtime exceptions
*/
static LabelValueSanitizer TransformNullLabelsToEmptyString() {
return new LabelValueSanitizer() {
@Override
public String[] sanitize(String... labelValue) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change labelValue to labelValues since it's an array.

for (int i = 0; i < labelValue.length; i++) {
if (labelValue[i] == null) {
labelValue[i] = "";
}
}
return labelValue;
}
};
}

Comment on lines +198 to +214
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the sanitizer takes only a single string as argument instead of an array of strings, a good handler for null values would be String::valueOf. As this already exists in the Java runtime, there's no need to provide our own implementation in SimpleCollector.

/**
* Builders let you configure and then create collectors.
*/
Expand All @@ -191,6 +223,7 @@ public abstract static class Builder<B extends Builder<B, C>, C extends SimpleCo
String unit = "";
String help = "";
String[] labelNames = new String[]{};
LabelValueSanitizer labelValueSanitizer = NoOpLabelValueSanitizer();
// Some metrics require additional setup before the initialization can be done.
boolean dontInitializeNoLabelsChild;

Expand Down Expand Up @@ -239,6 +272,18 @@ public B labelNames(String... labelNames) {
return (B)this;
}

/**
* Sanitize labels. Optional, defaults to no manipulation of labels.
* Useful to scrub credentials from labels
* or avoid Null values causing runtime exceptions
* @param handler
* @return new array of labels to use
*/
public B labelValueSanitizer(LabelValueSanitizer handler) {
this.labelValueSanitizer = handler;
return (B)this;
}

/**
* Return the constructed collector.
* <p>
Expand Down
35 changes: 35 additions & 0 deletions simpleclient/src/test/java/io/prometheus/client/SimpleCollectorWithLabelSanitizerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package io.prometheus.client;

import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

import static org.junit.Assert.*;
import static org.junit.rules.ExpectedException.none;


public class SimpleCollectorWithLabelSanitizerTest {

CollectorRegistry registry;
Gauge metric;

@Rule
public final ExpectedException thrown = none();

@Before
public void setUp() {
registry = new CollectorRegistry();
metric = Gauge.build().name("nonulllabels").help("help").labelNames("l").labelValueSanitizer(Gauge.TransformNullLabelsToEmptyString()).register(registry);
}

private Double getValue(String labelValue) {
return registry.getSampleValue("nonulllabels", new String[]{"l"}, new String[]{labelValue});
}

@Test
public void testNullLabelDoesntThrowWithLabelSanitizer() {
metric.labels(new String[]{null}).inc();
assertEquals(1.0, getValue("").doubleValue(), .001);
}
}