Skip to content

Commit 957e2e7

Browse files
zeitlingerzeitlinger
committed
Address Jack's Prometheus translation review nits
- Replace IllegalArgumentException control flow in PrometheusUnitsHelper.sanitizeUnitName with @nullable return; drop the try/catch in unitOrNull. - Extract doConvert helper so convert is just the IAE try/catch boundary. - Inline the getMergeKey ternary at the putOrMerge call site. - Reorder convertMetadataEscapedWithSuffixes for readability and use the explicit 5-arg MetricMetadata constructor. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
1 parent 671e437 commit 957e2e7

2 files changed

Lines changed: 79 additions & 83 deletions

File tree

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

Lines changed: 69 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -156,71 +156,73 @@ MetricSnapshots convert(@Nullable Collection<MetricData> metricDataCollection) {
156156
@Nullable
157157
private MetricSnapshot convert(MetricData metricData) {
158158
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:
167-
return convertLongGauge(
168-
metadata, scope, metricData.getLongGaugeData().getPoints(), metricData.getResource());
169-
case DOUBLE_GAUGE:
170-
return convertDoubleGauge(
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-
}
159+
return doConvert(metricData);
219160
} catch (IllegalArgumentException e) {
220161
THROTTLING_LOGGER.log(
221162
Level.WARNING,
222163
"Failed to convert metric " + metricData.getName() + ". Dropping metric.",
223164
e);
165+
return null;
166+
}
167+
}
168+
169+
@Nullable
170+
private MetricSnapshot doConvert(MetricData metricData) {
171+
// Note that AggregationTemporality.DELTA should never happen
172+
// because PrometheusMetricReader#getAggregationTemporality returns CUMULATIVE.
173+
174+
boolean isCounter = isMonotonicSum(metricData);
175+
MetricMetadata metadata = convertMetadata(metricData, isCounter);
176+
InstrumentationScopeInfo scope = metricData.getInstrumentationScopeInfo();
177+
switch (metricData.getType()) {
178+
case LONG_GAUGE:
179+
return convertLongGauge(
180+
metadata, scope, metricData.getLongGaugeData().getPoints(), metricData.getResource());
181+
case DOUBLE_GAUGE:
182+
return convertDoubleGauge(
183+
metadata, scope, metricData.getDoubleGaugeData().getPoints(), metricData.getResource());
184+
case LONG_SUM:
185+
SumData<LongPointData> longSumData = metricData.getLongSumData();
186+
if (longSumData.getAggregationTemporality() == AggregationTemporality.DELTA) {
187+
return null;
188+
} else if (longSumData.isMonotonic()) {
189+
return convertLongCounter(
190+
metadata, scope, longSumData.getPoints(), metricData.getResource());
191+
} else {
192+
return convertLongGauge(
193+
metadata, scope, longSumData.getPoints(), metricData.getResource());
194+
}
195+
case DOUBLE_SUM:
196+
SumData<DoublePointData> doubleSumData = metricData.getDoubleSumData();
197+
if (doubleSumData.getAggregationTemporality() == AggregationTemporality.DELTA) {
198+
return null;
199+
} else if (doubleSumData.isMonotonic()) {
200+
return convertDoubleCounter(
201+
metadata, scope, doubleSumData.getPoints(), metricData.getResource());
202+
} else {
203+
return convertDoubleGauge(
204+
metadata, scope, doubleSumData.getPoints(), metricData.getResource());
205+
}
206+
case HISTOGRAM:
207+
HistogramData histogramData = metricData.getHistogramData();
208+
if (histogramData.getAggregationTemporality() == AggregationTemporality.DELTA) {
209+
return null;
210+
} else {
211+
return convertHistogram(
212+
metadata, scope, histogramData.getPoints(), metricData.getResource());
213+
}
214+
case EXPONENTIAL_HISTOGRAM:
215+
ExponentialHistogramData exponentialHistogramData =
216+
metricData.getExponentialHistogramData();
217+
if (exponentialHistogramData.getAggregationTemporality() == AggregationTemporality.DELTA) {
218+
return null;
219+
} else {
220+
return convertExponentialHistogram(
221+
metadata, scope, exponentialHistogramData.getPoints(), metricData.getResource());
222+
}
223+
case SUMMARY:
224+
return convertSummary(
225+
metadata, scope, metricData.getSummaryData().getPoints(), metricData.getResource());
224226
}
225227
return null;
226228
}
@@ -691,15 +693,15 @@ private MetricMetadata convertMetadata(MetricData metricData, boolean isCounter)
691693
}
692694

693695
private static MetricMetadata convertMetadataEscapedWithSuffixes(MetricData metricData) {
694-
String name = convertLegacyMetricName(metricData.getName());
696+
String originalName = metricData.getName();
697+
String name = stripReservedMetricSuffixes(convertLegacyMetricName(originalName));
695698
String help = metricData.getDescription();
696699
Unit unit = PrometheusUnitsHelper.convertUnit(metricData.getUnit());
697-
name = stripReservedMetricSuffixes(name);
698700
if (unit != null && !name.endsWith(unit.toString())) {
699701
name = name + "_" + unit;
700702
}
701-
validateNormalizedMetricName(metricData.getName(), name);
702-
return new MetricMetadata(name, help, unit);
703+
validateNormalizedMetricName(originalName, name);
704+
return new MetricMetadata(name, name, name, help, unit);
703705
}
704706

705707
private static MetricMetadata convertMetadataEscapedWithoutSuffixes(MetricData metricData) {
@@ -749,7 +751,9 @@ private static String stripReservedMetricSuffixes(String name) {
749751
}
750752

751753
private void putOrMerge(Map<String, MetricSnapshot> snapshotsByName, MetricSnapshot snapshot) {
752-
String name = getMergeKey(snapshot.getMetadata());
754+
MetricMetadata metadata = snapshot.getMetadata();
755+
String name =
756+
shouldEscape(translationStrategy) ? metadata.getPrometheusName() : metadata.getName();
753757
if (snapshotsByName.containsKey(name)) {
754758
MetricSnapshot merged = merge(snapshotsByName.get(name), snapshot);
755759
if (merged != null) {
@@ -760,13 +764,6 @@ private void putOrMerge(Map<String, MetricSnapshot> snapshotsByName, MetricSnaps
760764
}
761765
}
762766

763-
private String getMergeKey(MetricMetadata metadata) {
764-
if (shouldEscape(translationStrategy)) {
765-
return metadata.getPrometheusName();
766-
}
767-
return metadata.getName();
768-
}
769-
770767
private static boolean shouldEscape(TranslationStrategy translationStrategy) {
771768
return translationStrategy == TranslationStrategy.UNDERSCORE_ESCAPING_WITH_SUFFIXES
772769
|| translationStrategy == TranslationStrategy.UNDERSCORE_ESCAPING_WITHOUT_SUFFIXES;

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

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -97,23 +97,23 @@ static Unit convertUnit(String otelUnit) {
9797

9898
@Nullable
9999
private static Unit unitOrNull(String name) {
100-
try {
101-
String sanitized = sanitizeUnitName(name);
102-
sanitized = stripReservedUnitSuffixes(sanitized);
103-
if (sanitized.isEmpty()) {
104-
return null;
105-
}
106-
return new Unit(sanitized);
107-
} catch (IllegalArgumentException e) {
100+
String sanitized = sanitizeUnitName(name);
101+
if (sanitized == null) {
102+
return null;
103+
}
104+
sanitized = stripReservedUnitSuffixes(sanitized);
105+
if (sanitized.isEmpty()) {
108106
return null;
109107
}
108+
return new Unit(sanitized);
110109
}
111110

112111
// These helpers are adapted from Prometheus naming sanitization. We keep a local copy because
113112
// the exporter still needs unit normalization behavior that fits the Prometheus Java model API.
113+
@Nullable
114114
private static String sanitizeUnitName(String unitName) {
115115
if (unitName.isEmpty()) {
116-
throw new IllegalArgumentException("Cannot convert an empty string to a valid unit name.");
116+
return null;
117117
}
118118
String sanitizedName = replaceIllegalCharsInUnitName(unitName);
119119
while (sanitizedName.startsWith("_") || sanitizedName.startsWith(".")) {
@@ -123,8 +123,7 @@ private static String sanitizeUnitName(String unitName) {
123123
sanitizedName = sanitizedName.substring(0, sanitizedName.length() - 1);
124124
}
125125
if (sanitizedName.isEmpty()) {
126-
throw new IllegalArgumentException(
127-
"Cannot convert '" + unitName + "' into a valid unit name.");
126+
return null;
128127
}
129128
return sanitizedName;
130129
}

0 commit comments

Comments
 (0)