Skip to content

Prometheus label conversion refactored to align with spec #7291

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import static java.util.Objects.requireNonNull;

import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.AttributeType;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.SpanContext;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
Expand Down Expand Up @@ -472,7 +473,9 @@

Map<String, String> labelNameToValue = new HashMap<>();
attributes.forEach(
(key, value) -> labelNameToValue.put(sanitizeLabelName(key.getKey()), value.toString()));
(key, value) ->
labelNameToValue.put(
sanitizeLabelName(key.getKey()), toLabelValue(key.getType(), value)));

for (int i = 0; i < additionalAttributes.length; i += 2) {
labelNameToValue.putIfAbsent(
Expand Down Expand Up @@ -642,4 +645,78 @@
// Simple helper for a log message.
return snapshot.getClass().getSimpleName().replace("Snapshot", "").toLowerCase(Locale.ENGLISH);
}

private static String toLabelValue(AttributeType type, Object attributeValue) {
if (AttributeType.STRING.equals(type)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

switch here? Many cases could be grouped into one

return attributeValue.toString();
} else if (AttributeType.BOOLEAN.equals(type)) {
return attributeValue.toString();
} else if (AttributeType.LONG.equals(type)) {
return attributeValue.toString();
} else if (AttributeType.DOUBLE.equals(type)) {
return attributeValue.toString();
} else if (AttributeType.STRING_ARRAY.equals(type)) {
if (attributeValue instanceof List) {
return ((List<?>) attributeValue)
.stream()
.map(String.class::cast)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about calling toString here instead? Just to avoid the class cast error in case something goes wrong

.map(Otel2PrometheusConverter::toJsonValidStr)
.collect(Collectors.toList())
.toString();
} else {
LOGGER.log(

Check warning on line 667 in exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java

View check run for this annotation

Codecov / codecov/patch

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java#L667

Added line #L667 was not covered by tests
Level.WARNING,
"Unexpected label value for AttributeType.STRING_ARRAY, toString() is being used as fallback value...");
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also log attributeValue.getClass(), and are the three trailing dots a convention in this codebase?

return attributeValue.toString();

Check warning on line 670 in exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java

View check run for this annotation

Codecov / codecov/patch

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java#L670

Added line #L670 was not covered by tests
}
} else if (AttributeType.BOOLEAN_ARRAY.equals(type)) {
return attributeValue.toString();
} else if (AttributeType.LONG_ARRAY.equals(type)) {
return attributeValue.toString();
} else if (AttributeType.DOUBLE_ARRAY.equals(type)) {
return attributeValue.toString();
} else {
throw new IllegalStateException(("Unrecognized AttributeType: " + type));

Check warning on line 679 in exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java

View check run for this annotation

Codecov / codecov/patch

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java#L679

Added line #L679 was not covered by tests
}
}

public static String toJsonValidStr(String str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is only relevant for STRING and STRING_ARRAY, right? How about calling it only where is needed, in the relevant case in toLabelValue?

StringBuilder sb = new StringBuilder();
sb.append('"');
for (int i = 0; i < str.length(); i++) {
char c = str.charAt(i);

switch (c) {
case '"':
sb.append("\\\"");
break;

Check warning on line 692 in exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java

View check run for this annotation

Codecov / codecov/patch

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java#L691-L692

Added lines #L691 - L692 were not covered by tests
case '\\':
sb.append("\\\\");
break;
case '\b':
sb.append("\\b");
break;
case '\f':
sb.append("\\f");
break;
case '\n':
sb.append("\\n");
break;
case '\r':
sb.append("\\r");
break;
case '\t':
sb.append("\\t");
break;
default:
if (c <= 0x1F) {
sb.append(String.format(Locale.ROOT, "\\u%04X", (int) c));
} else {
sb.append(c);
}
}
}
sb.append('"');
return sb.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;

import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.AttributeType;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.metrics.data.AggregationTemporality;
Expand Down Expand Up @@ -137,6 +139,71 @@ void prometheusNameCollisionTest_Issue6277() {
assertThatCode(() -> converter.convert(metricData)).doesNotThrowAnyException();
}

@Test
void labelValueSerialization_Primitives() {
Attributes attributes =
Attributes.builder()
.put(AttributeKey.stringKey("stringKey"), "stringValue")
.put(AttributeKey.booleanKey("booleanKey"), true)
.put(AttributeKey.longKey("longKey"), Long.MAX_VALUE)
.put(AttributeKey.doubleKey("doubleKey"), 0.12345)
.build();
MetricData metricData =
createSampleMetricData("sample", "1", MetricDataType.LONG_SUM, attributes, null);

MetricSnapshots snapshots = converter.convert(Collections.singletonList(metricData));

assertThat(snapshots.get(0).getDataPoints().get(0).getLabels().get("stringKey"))
.isEqualTo("stringValue");
assertThat(snapshots.get(0).getDataPoints().get(0).getLabels().get("booleanKey"))
.isEqualTo("true");
assertThat(snapshots.get(0).getDataPoints().get(0).getLabels().get("longKey"))
.isEqualTo("9223372036854775807");
assertThat(snapshots.get(0).getDataPoints().get(0).getLabels().get("doubleKey"))
.isEqualTo("0.12345");
}

@Test
void labelValueSerialization_NonPrimitives() {
Attributes attributes =
Attributes.builder()
.put(
AttributeKey.stringArrayKey("stringKey"),
Arrays.asList("stringValue1", "\\\\\\+\b+\f+\n+\r+\t+" + (char) 0))
.put(AttributeKey.booleanArrayKey("booleanKey"), Arrays.asList(true, false))
.put(AttributeKey.longArrayKey("longKey"), Arrays.asList(12345L, 6789L))
.put(AttributeKey.doubleArrayKey("doubleKey"), Arrays.asList(0.12345, 0.6789))
.build();
MetricData metricData =
createSampleMetricData("sample", "1", MetricDataType.LONG_SUM, attributes, null);

MetricSnapshots snapshots = converter.convert(Collections.singletonList(metricData));

assertThat(snapshots.get(0).getDataPoints().get(0).getLabels().get("stringKey"))
.isEqualTo("[\"stringValue1\", \"\\\\\\\\\\\\+\\b+\\f+\\n+\\r+\\t+\\u0000\"]");
assertThat(snapshots.get(0).getDataPoints().get(0).getLabels().get("booleanKey"))
.isEqualTo("[true, false]");
assertThat(snapshots.get(0).getDataPoints().get(0).getLabels().get("longKey"))
.isEqualTo("[12345, 6789]");
assertThat(snapshots.get(0).getDataPoints().get(0).getLabels().get("doubleKey"))
.isEqualTo("[0.12345, 0.6789]");
}

@Test
void labelValueSerialization_Should_Handle_All_AttributeTypes() {
assertThat(Stream.of(AttributeType.values()).map(Enum::name))
.isEqualTo(
Arrays.asList(
"STRING",
"BOOLEAN",
"LONG",
"DOUBLE",
"STRING_ARRAY",
"BOOLEAN_ARRAY",
"LONG_ARRAY",
"DOUBLE_ARRAY"));
}

private static Stream<Arguments> resourceAttributesAdditionArgs() {
List<Arguments> arguments = new ArrayList<>();

Expand Down
Loading