Skip to content

Commit

Permalink
Correct Prometheus formatting error with tags; remove training unders…
Browse files Browse the repository at this point in the history
…core in name when units are not present (helidon-io#3453)

Signed-off-by: [email protected] <[email protected]>
  • Loading branch information
tjquinno authored Sep 30, 2021
1 parent 308b6fb commit 6bfe88f
Show file tree
Hide file tree
Showing 4 changed files with 199 additions and 7 deletions.
12 changes: 6 additions & 6 deletions metrics/metrics/src/main/java/io/helidon/metrics/MetricImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ void appendPrometheusElement(StringBuilder sb,
boolean withHelpType,
String typeName,
Derived derived) {
appendPrometheusElement(sb, name.units(), () -> name.nameStatUnits(statName), withHelpType, typeName, derived.value(),
appendPrometheusElement(sb, name, () -> name.nameStatUnits(statName), withHelpType, typeName, derived.value(),
derived.sample());
}

Expand All @@ -307,12 +307,12 @@ void appendPrometheusElement(StringBuilder sb,
boolean withHelpType,
String typeName,
Sample.Labeled sample) {
appendPrometheusElement(sb, name.units(), () -> name.nameStatUnits(statName), withHelpType, typeName, sample.value(),
appendPrometheusElement(sb, name, () -> name.nameStatUnits(statName), withHelpType, typeName, sample.value(),
sample);
}

private void appendPrometheusElement(StringBuilder sb,
Units units,
PrometheusName name,
Supplier<String> nameToUse,
boolean withHelpType,
String typeName,
Expand All @@ -321,11 +321,11 @@ private void appendPrometheusElement(StringBuilder sb,
if (withHelpType) {
prometheusType(sb, nameToUse.get(), typeName);
}
Object convertedValue = units.convert(value);
sb.append(nameToUse.get())
Object convertedValue = name.units().convert(value);
sb.append(nameToUse.get() + name.prometheusTags())
.append(" ")
.append(convertedValue)
.append(prometheusExemplar(sample, units))
.append(prometheusExemplar(sample, name.units()))
.append("\n");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ String nameUnits(Units units) {
* @return name with stat name with units
*/
String nameStatUnits(String statName) {
return nameStat(statName) + "_" + prometheusUnit;
return nameStat(statName) + (prometheusUnit.isBlank() ? "" : "_" + prometheusUnit);
}

String nameStat(String statName) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,29 +22,39 @@
import java.text.NumberFormat;
import java.text.ParseException;
import java.util.AbstractMap;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import java.util.function.Predicate;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import javax.json.Json;
import javax.json.JsonObject;
import javax.json.JsonObjectBuilder;
import javax.json.JsonValue;

import org.eclipse.microprofile.metrics.Histogram;
import org.eclipse.microprofile.metrics.Metadata;
import org.eclipse.microprofile.metrics.MetricID;
import org.eclipse.microprofile.metrics.MetricType;
import org.eclipse.microprofile.metrics.MetricUnits;
import org.eclipse.microprofile.metrics.Snapshot;
import org.eclipse.microprofile.metrics.Tag;
import org.hamcrest.collection.IsMapContaining;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;

import static io.helidon.metrics.HelidonMetricsMatcher.withinTolerance;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.isEmptyOrNullString;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
import static org.junit.jupiter.api.Assertions.assertAll;
Expand Down Expand Up @@ -92,6 +102,19 @@ class HelidonHistogramTest {
+ "application_file_sizes_bytes{quantile=\"0.99\"} 98000\n"
+ "application_file_sizes_bytes{quantile=\"0.999\"} 99000\n";

private static final Tag[] HISTO_INT_TAGS = new Tag[] {
new Tag("tag1", "val1"),
new Tag("tag2", "val2")};

private static final Map<String, String> HISTO_INT_TAGS_AS_MAP =
Arrays.stream(HISTO_INT_TAGS).collect(Collectors.toMap(Tag::getTagName, Tag::getTagValue));

private static final Map<String, String> HISTO_INT_TAGS_AS_MAP_PROM =
Arrays.stream(HISTO_INT_TAGS).collect(Collectors.toMap(Tag::getTagName, tag -> "\"" + tag.getTagValue() + "\""));

// name{tag="val",tag="val"} where the braces and tags within are optional
private static final Pattern PROMETHEUS_KEY_PATTERN = Pattern.compile("([^{]+)(?:\\{([^}]+)})?+");

/**
* Parses a {@code Stream| of text lines (presumably in Prometheus/OpenMetrics format) into a {@code Stream}
* of {@code Map.Entry}, with the key the value name and the value a {@code Number}
Expand Down Expand Up @@ -120,6 +143,7 @@ private static Stream<Map.Entry<String, Number>> parsePrometheusText(String prom
private static Metadata meta;
private static HelidonHistogram histoInt;
private static MetricID histoIntID;
private static MetricID histoIntIDWithTags;
private static HelidonHistogram delegatingHistoInt;
private static HelidonHistogram histoLong;
private static HelidonHistogram delegatingHistoLong;
Expand All @@ -137,6 +161,7 @@ static void initClass() {

histoInt = HelidonHistogram.create("application", meta);
histoIntID = new MetricID("file_sizes");
histoIntIDWithTags = new MetricID(histoIntID.getName(), HISTO_INT_TAGS);
delegatingHistoInt = HelidonHistogram.create("application", meta, HelidonHistogram.create("ignored", meta));
histoLong = HelidonHistogram.create("application", meta);
delegatingHistoLong = HelidonHistogram.create("application", meta, HelidonHistogram.create("ignored", meta));
Expand Down Expand Up @@ -212,6 +237,36 @@ void testJson() {
assertThat("p999", metricData.getJsonNumber("p999").intValue(), is(withinTolerance(99)));
}

@Test
void testJsonWithTags() {
JsonObjectBuilder builder = Json.createObjectBuilder();
histoInt.jsonData(builder, new MetricID("file_sizes", HISTO_INT_TAGS));

JsonObject result = builder.build();

JsonObject metricData = result.getJsonObject("file_sizes");
assertThat(metricData, notNullValue());

checkJsonTreeForTags("file_sizes", metricData, HISTO_INT_TAGS_AS_MAP);
}

private static void checkJsonTreeForTags(String key, JsonValue jsonValue, Map<String, String> expectedTags) {
if (jsonValue.getValueType() == JsonValue.ValueType.OBJECT) {
jsonValue.asJsonObject()
.forEach((childKey, childValue) -> checkJsonTreeForTags(key + "." + childKey, childValue, expectedTags));
} else {
assertThat("Leaf JSON node with key " + key,
tagsFromJsonKey(key),
MetricsCustomMatchers.MapContains.all(HISTO_INT_TAGS_AS_MAP));
}
}

private static Map<String, String> tagsFromJsonKey(String jsonKey) {
return jsonKey.contains(";")
? tagsFromDelimitedString(jsonKey.substring(jsonKey.indexOf(';') + 1), ";")
: Collections.emptyMap();
}

@Test
void testPrometheus() throws IOException, ParseException {
final StringBuilder sb = new StringBuilder();
Expand All @@ -222,6 +277,44 @@ void testPrometheus() throws IOException, ParseException {
is(withinTolerance(entry.getValue()))));
}

@Test
void testPrometheusWithTags() {
final StringBuilder sb = new StringBuilder();
histoInt.prometheusData(sb, histoIntIDWithTags, true);

parsePrometheusText(new LineNumberReader(new StringReader(sb.toString())).lines())
.forEach(entry -> assertThat("Missing tag labels for " + entry.getKey(),
tagsFromPrometheusKey(entry.getKey()),
MetricsCustomMatchers.MapContains.all(HISTO_INT_TAGS_AS_MAP_PROM)));
}

private static Map<String, String> tagsFromPrometheusKey(String promKey) {
// Actual tags will exclude any possible "quantile" settings.
List<Tag> result = new ArrayList<>();
Matcher m = PROMETHEUS_KEY_PATTERN.matcher(promKey);
if (!m.matches()) {
fail("Could not parse Prometheus key for tags: " + promKey);
}
if (m.groupCount() <= 1) {
return Collections.emptyMap();
}

String tagExprs = m.group(2);
assertThat("Tag expressions from Prometheus key " + promKey, tagExprs, not(isEmptyOrNullString()));

return tagsFromDelimitedString(m.group(2), ",");
}

private static Map<String, String> tagsFromDelimitedString(String delimitedString, String delimiter) {
return Arrays.stream(delimitedString.split(delimiter))
.filter(Predicate.not(tagExpr -> tagExpr.startsWith("quantile")))
.map(tagExpr -> tagExpr.split("="))
.peek(arr -> {
assertThat("Tag expression is name=value giving two segments", arr.length, is(2));
})
.collect(Collectors.toMap(arr -> arr[0], arr -> arr[1]));
}

@Test
void testStatisticalValues() {
testSnapshot(1, "integers", histoInt.getSnapshot(), 50.6, 29.4389);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright (c) 2021 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.helidon.metrics;

import org.hamcrest.Description;
import org.hamcrest.TypeSafeMatcher;

import java.util.Map;
import java.util.function.Predicate;

/**
* Custom Hamcrest matchers used in metrics tests.
*/
class MetricsCustomMatchers {

/**
* A group of matchers dealing with map contents.
*/
static abstract class MapContains extends TypeSafeMatcher<Map<?, ?>> {

static All all(Map<?, ?> targetValues) {
return new All(targetValues);
}

static None none(Map<?, ?> targetValues) {
return new None(targetValues);
}

private final Map<?, ?> targetValues;
private final String descriptionLabel;

MapContains(Map<?, ?> targetValues, String descriptionLabel) {
this.targetValues = targetValues;
this.descriptionLabel = descriptionLabel;
}

protected Map<?, ?> targetValues() {
return targetValues;
}

protected Predicate<Map.Entry<?, ?>> entryChecker(Map<?, ?> candidateMap) {
return (Map.Entry<?, ?> expectedEntry) -> candidateMap.containsKey(expectedEntry.getKey())
&& candidateMap.get(expectedEntry.getKey())
.equals(expectedEntry.getValue());
}

@Override
public void describeTo(Description description) {
description.appendText("tags containing ")
.appendText(descriptionLabel)
.appendText(" of ")
.appendText(targetValues.toString());
}

/**
* Matcher for checking that all key/value pairs in the target values appear in the candidate map.
*/
static class All extends MapContains {

All(Map<?, ?> targetValues) {
super(targetValues, "all");
}

@Override
protected boolean matchesSafely(Map<?, ?> candidateMap) {
return targetValues().entrySet()
.stream()
.allMatch(entryChecker(candidateMap));
}
}

static class None extends MapContains {

None(Map<?, ?> targetValues) {
super(targetValues, "none");
}

@Override
protected boolean matchesSafely(Map<?, ?> candidateMap) {
return targetValues().entrySet()
.stream()
.noneMatch(entryChecker(candidateMap));
}
}
}
}

0 comments on commit 6bfe88f

Please sign in to comment.