Skip to content

Commit 671e437

Browse files
zeitlingerzeitlinger
committed
Address Prometheus translation review comments
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
1 parent c59b24d commit 671e437

5 files changed

Lines changed: 169 additions & 83 deletions

File tree

‎exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java‎

Lines changed: 112 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -155,62 +155,72 @@ MetricSnapshots convert(@Nullable Collection<MetricData> metricDataCollection) {
155155

156156
@Nullable
157157
private MetricSnapshot convert(MetricData metricData) {
158-
159-
// Note that AggregationTemporality.DELTA should never happen
160-
// because PrometheusMetricReader#getAggregationTemporality returns CUMULATIVE.
161-
162-
boolean isCounter = isMonotonicSum(metricData);
163-
MetricMetadata metadata = convertMetadata(metricData, isCounter);
164-
InstrumentationScopeInfo scope = metricData.getInstrumentationScopeInfo();
165-
switch (metricData.getType()) {
166-
case LONG_GAUGE:
167-
return convertLongGauge(
168-
metadata, scope, metricData.getLongGaugeData().getPoints(), metricData.getResource());
169-
case DOUBLE_GAUGE:
170-
return convertDoubleGauge(
171-
metadata, scope, metricData.getDoubleGaugeData().getPoints(), metricData.getResource());
172-
case LONG_SUM:
173-
SumData<LongPointData> longSumData = metricData.getLongSumData();
174-
if (longSumData.getAggregationTemporality() == AggregationTemporality.DELTA) {
175-
return null;
176-
} else if (longSumData.isMonotonic()) {
177-
return convertLongCounter(
178-
metadata, scope, longSumData.getPoints(), metricData.getResource());
179-
} else {
158+
try {
159+
// Note that AggregationTemporality.DELTA should never happen
160+
// because PrometheusMetricReader#getAggregationTemporality returns CUMULATIVE.
161+
162+
boolean isCounter = isMonotonicSum(metricData);
163+
MetricMetadata metadata = convertMetadata(metricData, isCounter);
164+
InstrumentationScopeInfo scope = metricData.getInstrumentationScopeInfo();
165+
switch (metricData.getType()) {
166+
case LONG_GAUGE:
180167
return convertLongGauge(
181-
metadata, scope, longSumData.getPoints(), metricData.getResource());
182-
}
183-
case DOUBLE_SUM:
184-
SumData<DoublePointData> doubleSumData = metricData.getDoubleSumData();
185-
if (doubleSumData.getAggregationTemporality() == AggregationTemporality.DELTA) {
186-
return null;
187-
} else if (doubleSumData.isMonotonic()) {
188-
return convertDoubleCounter(
189-
metadata, scope, doubleSumData.getPoints(), metricData.getResource());
190-
} else {
168+
metadata, scope, metricData.getLongGaugeData().getPoints(), metricData.getResource());
169+
case DOUBLE_GAUGE:
191170
return convertDoubleGauge(
192-
metadata, scope, doubleSumData.getPoints(), metricData.getResource());
193-
}
194-
case HISTOGRAM:
195-
HistogramData histogramData = metricData.getHistogramData();
196-
if (histogramData.getAggregationTemporality() == AggregationTemporality.DELTA) {
197-
return null;
198-
} else {
199-
return convertHistogram(
200-
metadata, scope, histogramData.getPoints(), metricData.getResource());
201-
}
202-
case EXPONENTIAL_HISTOGRAM:
203-
ExponentialHistogramData exponentialHistogramData =
204-
metricData.getExponentialHistogramData();
205-
if (exponentialHistogramData.getAggregationTemporality() == AggregationTemporality.DELTA) {
206-
return null;
207-
} else {
208-
return convertExponentialHistogram(
209-
metadata, scope, exponentialHistogramData.getPoints(), metricData.getResource());
210-
}
211-
case SUMMARY:
212-
return convertSummary(
213-
metadata, scope, metricData.getSummaryData().getPoints(), metricData.getResource());
171+
metadata,
172+
scope,
173+
metricData.getDoubleGaugeData().getPoints(),
174+
metricData.getResource());
175+
case LONG_SUM:
176+
SumData<LongPointData> longSumData = metricData.getLongSumData();
177+
if (longSumData.getAggregationTemporality() == AggregationTemporality.DELTA) {
178+
return null;
179+
} else if (longSumData.isMonotonic()) {
180+
return convertLongCounter(
181+
metadata, scope, longSumData.getPoints(), metricData.getResource());
182+
} else {
183+
return convertLongGauge(
184+
metadata, scope, longSumData.getPoints(), metricData.getResource());
185+
}
186+
case DOUBLE_SUM:
187+
SumData<DoublePointData> doubleSumData = metricData.getDoubleSumData();
188+
if (doubleSumData.getAggregationTemporality() == AggregationTemporality.DELTA) {
189+
return null;
190+
} else if (doubleSumData.isMonotonic()) {
191+
return convertDoubleCounter(
192+
metadata, scope, doubleSumData.getPoints(), metricData.getResource());
193+
} else {
194+
return convertDoubleGauge(
195+
metadata, scope, doubleSumData.getPoints(), metricData.getResource());
196+
}
197+
case HISTOGRAM:
198+
HistogramData histogramData = metricData.getHistogramData();
199+
if (histogramData.getAggregationTemporality() == AggregationTemporality.DELTA) {
200+
return null;
201+
} else {
202+
return convertHistogram(
203+
metadata, scope, histogramData.getPoints(), metricData.getResource());
204+
}
205+
case EXPONENTIAL_HISTOGRAM:
206+
ExponentialHistogramData exponentialHistogramData =
207+
metricData.getExponentialHistogramData();
208+
if (exponentialHistogramData.getAggregationTemporality()
209+
== AggregationTemporality.DELTA) {
210+
return null;
211+
} else {
212+
return convertExponentialHistogram(
213+
metadata, scope, exponentialHistogramData.getPoints(), metricData.getResource());
214+
}
215+
case SUMMARY:
216+
return convertSummary(
217+
metadata, scope, metricData.getSummaryData().getPoints(), metricData.getResource());
218+
}
219+
} catch (IllegalArgumentException e) {
220+
THROTTLING_LOGGER.log(
221+
Level.WARNING,
222+
"Failed to convert metric " + metricData.getName() + ". Dropping metric.",
223+
e);
214224
}
215225
return null;
216226
}
@@ -562,12 +572,19 @@ private List<AttributeKey<?>> filterAllowedResourceAttributeKeys(@Nullable Resou
562572
}
563573

564574
private String convertLabelName(String key) {
565-
if (translationStrategy.shouldEscape()) {
575+
if (shouldEscape(translationStrategy)) {
566576
return convertLegacyLabelName(key);
567577
}
568578
return key;
569579
}
570580

581+
/**
582+
* Normalize an attribute name to the legacy Prometheus label scheme.
583+
*
584+
* <p>This mirrors {@code prometheus/otlptranslator}'s invalid-character collapsing rules, but we
585+
* intentionally do not preserve {@code __...__} names because Prometheus Java rejects user label
586+
* names that begin with {@code __}.
587+
*/
571588
private static String convertLegacyLabelName(String key) {
572589
if (key.isEmpty()) {
573590
throw new IllegalArgumentException("label name is empty");
@@ -594,7 +611,7 @@ private static String convertLegacyLabelName(String key) {
594611
}
595612

596613
String normalized = result.toString();
597-
if (!normalized.isEmpty() && Character.isDigit(normalized.charAt(0))) {
614+
if (Character.isDigit(normalized.charAt(0))) {
598615
normalized = "key_" + normalized;
599616
}
600617
if (containsOnlyUnderscores(normalized)) {
@@ -629,12 +646,22 @@ private static String convertLegacyMetricName(String name) {
629646
}
630647

631648
StringBuilder result = new StringBuilder(name.length());
649+
boolean previousWasUnderscore = false;
632650
for (int i = 0; i < name.length(); ) {
633651
int codePoint = name.codePointAt(i);
634652
if (isValidLegacyMetricChar(codePoint, i)) {
635-
result.appendCodePoint(codePoint);
636-
} else {
653+
if (codePoint == '_') {
654+
if (!previousWasUnderscore) {
655+
result.append('_');
656+
previousWasUnderscore = true;
657+
}
658+
} else {
659+
result.appendCodePoint(codePoint);
660+
previousWasUnderscore = false;
661+
}
662+
} else if (!previousWasUnderscore) {
637663
result.append('_');
664+
previousWasUnderscore = true;
638665
}
639666
i += Character.charCount(codePoint);
640667
}
@@ -667,16 +694,19 @@ private static MetricMetadata convertMetadataEscapedWithSuffixes(MetricData metr
667694
String name = convertLegacyMetricName(metricData.getName());
668695
String help = metricData.getDescription();
669696
Unit unit = PrometheusUnitsHelper.convertUnit(metricData.getUnit());
670-
name = stripRepeatedUnderscores(stripReservedMetricSuffixes(name));
697+
name = stripReservedMetricSuffixes(name);
671698
if (unit != null && !name.endsWith(unit.toString())) {
672699
name = name + "_" + unit;
673700
}
674-
return new MetricMetadata(stripRepeatedUnderscores(name), help, unit);
701+
validateNormalizedMetricName(metricData.getName(), name);
702+
return new MetricMetadata(name, help, unit);
675703
}
676704

677705
private static MetricMetadata convertMetadataEscapedWithoutSuffixes(MetricData metricData) {
678-
String rawName = stripRepeatedUnderscores(convertLegacyMetricName(metricData.getName()));
706+
String rawName = convertLegacyMetricName(metricData.getName());
679707
String name = stripReservedMetricSuffixes(rawName);
708+
validateNormalizedMetricName(metricData.getName(), rawName);
709+
validateNormalizedMetricName(metricData.getName(), name);
680710
return new MetricMetadata(name, rawName, rawName, metricData.getDescription(), null);
681711
}
682712

@@ -718,13 +748,6 @@ private static String stripReservedMetricSuffixes(String name) {
718748
return name;
719749
}
720750

721-
private static String stripRepeatedUnderscores(String name) {
722-
while (name.contains("__")) {
723-
name = name.replace("__", "_");
724-
}
725-
return name;
726-
}
727-
728751
private void putOrMerge(Map<String, MetricSnapshot> snapshotsByName, MetricSnapshot snapshot) {
729752
String name = getMergeKey(snapshot.getMetadata());
730753
if (snapshotsByName.containsKey(name)) {
@@ -738,12 +761,32 @@ private void putOrMerge(Map<String, MetricSnapshot> snapshotsByName, MetricSnaps
738761
}
739762

740763
private String getMergeKey(MetricMetadata metadata) {
741-
if (translationStrategy.shouldEscape()) {
764+
if (shouldEscape(translationStrategy)) {
742765
return metadata.getPrometheusName();
743766
}
744767
return metadata.getName();
745768
}
746769

770+
private static boolean shouldEscape(TranslationStrategy translationStrategy) {
771+
return translationStrategy == TranslationStrategy.UNDERSCORE_ESCAPING_WITH_SUFFIXES
772+
|| translationStrategy == TranslationStrategy.UNDERSCORE_ESCAPING_WITHOUT_SUFFIXES;
773+
}
774+
775+
private static void validateNormalizedMetricName(String originalName, String normalizedName) {
776+
if (normalizedName.isEmpty()) {
777+
throw new IllegalArgumentException(
778+
"normalization for metric \"" + originalName + "\" resulted in empty name");
779+
}
780+
if (containsOnlyUnderscores(normalizedName)) {
781+
throw new IllegalArgumentException(
782+
"normalization for metric \""
783+
+ originalName
784+
+ "\" resulted in invalid name \""
785+
+ normalizedName
786+
+ "\"");
787+
}
788+
}
789+
747790
/**
748791
* OpenTelemetry may use the same metric name multiple times but in different instrumentation
749792
* scopes. In that case, we try to merge the metrics. They will have different {@code

‎exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusUnitsHelper.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,8 @@ private static Unit unitOrNull(String name) {
109109
}
110110
}
111111

112+
// These helpers are adapted from Prometheus naming sanitization. We keep a local copy because
113+
// the exporter still needs unit normalization behavior that fits the Prometheus Java model API.
112114
private static String sanitizeUnitName(String unitName) {
113115
if (unitName.isEmpty()) {
114116
throw new IllegalArgumentException("Cannot convert an empty string to a valid unit name.");

‎exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/TranslationStrategy.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,4 @@ public enum TranslationStrategy {
3030

3131
/** Metric and label names are passed through without translation. */
3232
NO_TRANSLATION;
33-
34-
boolean shouldEscape() {
35-
return this == UNDERSCORE_ESCAPING_WITH_SUFFIXES
36-
|| this == UNDERSCORE_ESCAPING_WITHOUT_SUFFIXES;
37-
}
3833
}

‎exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/internal/PrometheusComponentProvider.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ private static TranslationStrategy parseTranslationStrategy(String value) {
8989
case "no_translation/development":
9090
return TranslationStrategy.NO_TRANSLATION;
9191
default:
92-
throw new IllegalArgumentException("Unsupported translation_strategy: " + value);
92+
throw new DeclarativeConfigException("Unsupported translation_strategy: " + value);
9393
}
9494
}
9595
}

‎exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverterTest.java‎

Lines changed: 54 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
import static io.opentelemetry.api.common.AttributeKey.valueKey;
1717
import static org.assertj.core.api.Assertions.assertThat;
1818
import static org.assertj.core.api.Assertions.assertThatCode;
19-
import static org.assertj.core.api.Assertions.assertThatThrownBy;
2019

2120
import io.opentelemetry.api.common.Attributes;
2221
import io.opentelemetry.api.common.KeyValue;
@@ -242,6 +241,24 @@ private static Stream<Arguments> translationStrategyArgs() {
242241
TranslationStrategy.NO_TRANSLATION, "sample.name", "sample.name", "sample.name"));
243242
}
244243

244+
@Test
245+
void metricMetadata_underscoreEscapingCollapsesRepeatedUnderscores() {
246+
MetricSnapshots snapshots =
247+
converter.convert(
248+
Collections.singletonList(
249+
createSampleMetricData("sample__name", "By", MetricDataType.LONG_SUM)));
250+
251+
MetricMetadata metadata =
252+
snapshots.stream()
253+
.filter(snapshot -> snapshot instanceof CounterSnapshot)
254+
.findFirst()
255+
.orElseThrow(AssertionError::new)
256+
.getMetadata();
257+
assertThat(metadata.getName()).isEqualTo("sample_name_bytes");
258+
assertThat(metadata.getExpositionBaseName()).isEqualTo("sample_name_bytes");
259+
assertThat(metadata.getOriginalName()).isEqualTo("sample_name_bytes");
260+
}
261+
245262
@ParameterizedTest
246263
@MethodSource("legacyLabelNameTranslationArgs")
247264
void labelNameTranslation_underscoreEscaping(String labelName, String expectedLabelName) {
@@ -271,13 +288,42 @@ private static Stream<Arguments> legacyLabelNameTranslationArgs() {
271288
}
272289

273290
@Test
274-
void labelNameTranslation_legacyRejectsInvalidNormalizedName() {
275-
assertThatThrownBy(
276-
() ->
277-
convertAttributeLabels(
278-
"ようこそ", TranslationStrategy.UNDERSCORE_ESCAPING_WITH_SUFFIXES))
279-
.isInstanceOf(IllegalArgumentException.class)
280-
.hasMessage("normalization for label name \"ようこそ\" resulted in invalid name" + " \"_\"");
291+
void labelNameTranslation_legacyDropsMetricWithInvalidNormalizedName() {
292+
Otel2PrometheusConverter converter =
293+
new Otel2PrometheusConverter(
294+
/* otelScopeLabelsEnabled= */ false,
295+
/* targetInfoMetricEnabled= */ false,
296+
TranslationStrategy.UNDERSCORE_ESCAPING_WITH_SUFFIXES,
297+
/* allowedResourceAttributesFilter= */ null);
298+
299+
MetricSnapshots snapshots =
300+
converter.convert(
301+
Collections.singletonList(
302+
createSampleMetricData(
303+
"sample",
304+
"1",
305+
MetricDataType.LONG_SUM,
306+
Attributes.of(stringKey("ようこそ"), "value"),
307+
Resource.empty())));
308+
309+
assertThat(snapshots).isEmpty();
310+
}
311+
312+
@Test
313+
void metricNameTranslation_legacyDropsMetricWithInvalidNormalizedName() {
314+
Otel2PrometheusConverter converter =
315+
new Otel2PrometheusConverter(
316+
/* otelScopeLabelsEnabled= */ false,
317+
/* targetInfoMetricEnabled= */ false,
318+
TranslationStrategy.UNDERSCORE_ESCAPING_WITHOUT_SUFFIXES,
319+
/* allowedResourceAttributesFilter= */ null);
320+
321+
MetricSnapshots snapshots =
322+
converter.convert(
323+
Collections.singletonList(
324+
createSampleMetricData("ようこそ", "1", MetricDataType.LONG_SUM)));
325+
326+
assertThat(snapshots).isEmpty();
281327
}
282328

283329
@ParameterizedTest

0 commit comments

Comments
 (0)