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.

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

SOLR-17587: Prometheus Writer duplicate TYPE information in exposition format #2902

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,20 @@ private NamedList<Object> handlePrometheusExport(SolrParams params) {
List<MetricType> metricTypes = parseMetricTypes(params);
List<MetricFilter> metricFilters =
metricTypes.stream().map(MetricType::asMetricFilter).collect(Collectors.toList());

Set<String> requestedRegistries = parseRegistries(params);
MetricRegistry mergedCoreRegistries = new MetricRegistry();

for (String registryName : requestedRegistries) {
MetricRegistry dropwizardRegistry = metricManager.registry(registryName);

// Merge all core registries into a single registry and
// append the core name to the metric to avoid duplicate metrics name
if (registryName.startsWith("solr.core")) {
mergedCoreRegistries.registerAll(getCoreNameFromRegistry(registryName), dropwizardRegistry);
continue;
}

PrometheusResponseWriter.toPrometheus(
dropwizardRegistry,
registryName,
Expand All @@ -203,6 +213,22 @@ private NamedList<Object> handlePrometheusExport(SolrParams params) {
response.add(registryName, registry);
});
}

if (!mergedCoreRegistries.getMetrics().isEmpty()) {
PrometheusResponseWriter.toPrometheus(
mergedCoreRegistries,
"solr.core",
metricFilters,
mustMatchFilter,
propertyFilter,
false,
false,
true,
(registry) -> {
response.add("solr.core", registry);
});
}

return response;
}

Expand Down Expand Up @@ -523,6 +549,11 @@ private List<MetricType> parseMetricTypes(SolrParams params) {
return metricTypes;
}

private String getCoreNameFromRegistry(String registryName) {
String coreName = registryName.substring(registryName.indexOf('.') + 1);
return coreName.replace(".", "_");
}

@Override
public String getDescription() {
return "A handler to return all the metrics gathered by Solr";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
*/
package org.apache.solr.metrics.prometheus.core;

import java.util.regex.Pattern;

public interface PrometheusCoreFormatterInfo {
/** Category of prefix Solr Core dropwizard handler metric names */
enum CoreCategory {
Expand All @@ -32,6 +30,4 @@ enum CoreCategory {
INDEX,
CORE
}

Pattern CLOUD_CORE_PATTERN = Pattern.compile("^core_(.*)_(shard[0-9]+)_(replica_.[0-9]+)$");
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@
public class SolrCoreCacheMetric extends SolrCoreMetric {
public static final String CORE_CACHE_SEARCHER_METRICS = "solr_metrics_core_cache";

public SolrCoreCacheMetric(
Metric dropwizardMetric, String coreName, String metricName, boolean cloudMode) {
super(dropwizardMetric, coreName, metricName, cloudMode);
public SolrCoreCacheMetric(Metric dropwizardMetric, String metricName) {
super(dropwizardMetric, metricName);
}

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,8 @@ public class SolrCoreHandlerMetric extends SolrCoreMetric {
public static final String CORE_REQUESTS_TOTAL_TIME = "solr_metrics_core_requests_time";
public static final String CORE_REQUEST_TIMES = "solr_metrics_core_average_request_time";

public SolrCoreHandlerMetric(
Metric dropwizardMetric, String coreName, String metricName, boolean cloudMode) {
super(dropwizardMetric, coreName, metricName, cloudMode);
public SolrCoreHandlerMetric(Metric dropwizardMetric, String metricName) {
super(dropwizardMetric, metricName);
}

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@
public class SolrCoreHighlighterMetric extends SolrCoreMetric {
public static final String CORE_HIGHLIGHER_METRICS = "solr_metrics_core_highlighter_requests";

public SolrCoreHighlighterMetric(
Metric dropwizardMetric, String coreName, String metricName, boolean cloudMode) {
super(dropwizardMetric, coreName, metricName, cloudMode);
public SolrCoreHighlighterMetric(Metric dropwizardMetric, String metricName) {
super(dropwizardMetric, metricName);
}

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@
public class SolrCoreIndexMetric extends SolrCoreMetric {
public static final String CORE_INDEX_METRICS = "solr_metrics_core_index_size_bytes";

public SolrCoreIndexMetric(
Metric dropwizardMetric, String coreName, String metricName, boolean cloudMode) {
super(dropwizardMetric, coreName, metricName, cloudMode);
public SolrCoreIndexMetric(Metric dropwizardMetric, String metricName) {
super(dropwizardMetric, metricName);
}

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,37 +16,41 @@
*/
package org.apache.solr.metrics.prometheus.core;

import static org.apache.solr.metrics.prometheus.core.PrometheusCoreFormatterInfo.CLOUD_CORE_PATTERN;

import com.codahale.metrics.Metric;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.apache.solr.common.SolrException;
import org.apache.solr.metrics.prometheus.SolrMetric;

/** Base class is a wrapper to export a solr.core {@link com.codahale.metrics.Metric} */
public abstract class SolrCoreMetric extends SolrMetric {
public String coreName;

public SolrCoreMetric(
Metric dropwizardMetric, String coreName, String metricName, boolean cloudMode) {
super(dropwizardMetric, metricName);
this.coreName = coreName;
labels.put("core", coreName);
if (cloudMode) {
appendCloudModeLabels();
}
}
public static Pattern CLOUD_CORE_PATTERN =
Pattern.compile(
"(?<core>^core_(?<collection>.*)_(?<shard>shard[0-9]+)_(?<replica>replica_.[0-9]+)).(.*)$");
public static Pattern STANDALONE_CORE_PATTERN = Pattern.compile("^core_(?<core>.*?)\\.(.*)$");

public String coreName;

private void appendCloudModeLabels() {
Matcher m = CLOUD_CORE_PATTERN.matcher(coreName);
if (m.find()) {
labels.put("collection", m.group(1));
labels.put("shard", m.group(2));
labels.put("replica", m.group(3));
public SolrCoreMetric(Metric dropwizardMetric, String metricName) {
// Remove Core Name prefix from metric as it is no longer needed for tag parsing from this point
super(dropwizardMetric, metricName.substring(metricName.indexOf(".") + 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

when you do that, a little comment like // chop off ___ would be really helpful to the reader

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Matcher cloudPattern = CLOUD_CORE_PATTERN.matcher(metricName);
Matcher standalonePattern = STANDALONE_CORE_PATTERN.matcher(metricName);
if (cloudPattern.find()) {
coreName = cloudPattern.group("core");
cloudPattern
.namedGroups()
.forEach((key, value) -> labels.put(key, cloudPattern.group(value)));
} else if (standalonePattern.find()) {
coreName = standalonePattern.group("core");
standalonePattern
.namedGroups()
.forEach((key, value) -> labels.put(key, standalonePattern.group(value)));
} else {
throw new SolrException(
SolrException.ErrorCode.SERVER_ERROR,
"Core name does not match pattern for parsing " + coreName);
"Core name does not match pattern for parsing in metric " + metricName);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,8 @@ public class SolrCoreSearcherMetric extends SolrCoreMetric {
public static final String CORE_SEARCHER_METRICS = "solr_metrics_core_searcher_documents";
public static final String CORE_SEARCHER_TIMES = "solr_metrics_core_average_searcher_warmup_time";

public SolrCoreSearcherMetric(
Metric dropwizardMetric, String coreName, String metricName, boolean cloudMode) {
super(dropwizardMetric, coreName, metricName, cloudMode);
public SolrCoreSearcherMetric(Metric dropwizardMetric, String metricName) {
super(dropwizardMetric, metricName);
}

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@
public class SolrCoreTlogMetric extends SolrCoreMetric {
public static final String CORE_TLOG_METRICS = "solr_metrics_core_tlog";

public SolrCoreTlogMetric(
Metric dropwizardMetric, String coreName, String metricName, boolean cloudMode) {
super(dropwizardMetric, coreName, metricName, cloudMode);
public SolrCoreTlogMetric(Metric dropwizardMetric, String metricName) {
super(dropwizardMetric, metricName);
}

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,9 @@
*/
public class SolrPrometheusCoreFormatter extends SolrPrometheusFormatter
implements PrometheusCoreFormatterInfo {
public final String coreName;
public final boolean cloudMode;

public SolrPrometheusCoreFormatter(String coreName, boolean cloudMode) {
public SolrPrometheusCoreFormatter() {
super();
this.coreName = coreName;
this.cloudMode = cloudMode;
}

@Override
Expand All @@ -45,7 +41,7 @@ public void exportDropwizardMetric(Metric dropwizardMetric, String metricName) {

@Override
public SolrMetric categorizeMetric(Metric dropwizardMetric, String metricName) {
String metricCategory = metricName.split("\\.", 2)[0];
String metricCategory = metricName.split("\\.", 3)[1];
if (!Enums.getIfPresent(PrometheusCoreFormatterInfo.CoreCategory.class, metricCategory)
.isPresent()) {
return new SolrNoOpMetric();
Expand All @@ -55,17 +51,17 @@ public SolrMetric categorizeMetric(Metric dropwizardMetric, String metricName) {
case QUERY:
case UPDATE:
case REPLICATION:
return new SolrCoreHandlerMetric(dropwizardMetric, coreName, metricName, cloudMode);
return new SolrCoreHandlerMetric(dropwizardMetric, metricName);
case TLOG:
return new SolrCoreTlogMetric(dropwizardMetric, coreName, metricName, cloudMode);
return new SolrCoreTlogMetric(dropwizardMetric, metricName);
case CACHE:
return new SolrCoreCacheMetric(dropwizardMetric, coreName, metricName, cloudMode);
return new SolrCoreCacheMetric(dropwizardMetric, metricName);
case SEARCHER:
return new SolrCoreSearcherMetric(dropwizardMetric, coreName, metricName, cloudMode);
return new SolrCoreSearcherMetric(dropwizardMetric, metricName);
case HIGHLIGHTER:
return new SolrCoreHighlighterMetric(dropwizardMetric, coreName, metricName, cloudMode);
return new SolrCoreHighlighterMetric(dropwizardMetric, metricName);
case INDEX:
return new SolrCoreIndexMetric(dropwizardMetric, coreName, metricName, cloudMode);
return new SolrCoreIndexMetric(dropwizardMetric, metricName);
case CORE:
default:
return new SolrNoOpMetric();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,11 @@
import java.io.IOException;
import java.io.OutputStream;
import java.lang.invoke.MethodHandles;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.function.Consumer;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.metrics.AggregateMetric;
import org.apache.solr.metrics.prometheus.SolrPrometheusFormatter;
Expand Down Expand Up @@ -115,30 +114,22 @@ public static void toPrometheus(
Metric dropwizardMetric = dropwizardMetrics.get(metricName);
formatter.exportDropwizardMetric(dropwizardMetric, metricName);
} catch (Exception e) {
// Do not fail entirely for metrics exporting. Log and try to export next metric
log.warn("Error occurred exporting Dropwizard Metric to Prometheus", e);
throw new SolrException(
SolrException.ErrorCode.SERVER_ERROR,
"Error occurred exporting Dropwizard Metric to Prometheus",
e);
Comment on lines +117 to +120
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also wanted to mention this. I was thinking to just remove the log.warn and actually throw a SolrException. I think at the time I thought that not failing metrics entirely and even just partially getting metrics was ok. But after some thought, adding this would actually fail any metrics from posting but helps exporting tests actually get caught if there is something wrong or even a user finding a bug. WDYT?

}
});

consumer.accept(formatter);
}

public static SolrPrometheusFormatter getFormatterType(String registryName) {
String coreName;
boolean cloudMode = false;
String[] parsedRegistry = registryName.split("\\.");

switch (parsedRegistry[1]) {
case "core":
if (parsedRegistry.length == 3) {
coreName = parsedRegistry[2];
} else if (parsedRegistry.length == 5) {
coreName = Arrays.stream(parsedRegistry).skip(1).collect(Collectors.joining("_"));
cloudMode = true;
} else {
coreName = registryName;
}
return new SolrPrometheusCoreFormatter(coreName, cloudMode);
return new SolrPrometheusCoreFormatter();
case "jvm":
return new SolrPrometheusJvmFormatter();
case "jetty":
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -725,8 +725,7 @@ public void testPrometheusMetricsCore() throws Exception {
NamedList<?> values = resp.getValues();
assertNotNull(values.get("metrics"));
values = (NamedList<?>) values.get("metrics");
SolrPrometheusFormatter formatter =
(SolrPrometheusFormatter) values.get("solr.core.collection1");
SolrPrometheusFormatter formatter = (SolrPrometheusFormatter) values.get("solr.core");
assertNotNull(formatter);
MetricSnapshots actualSnapshots = formatter.collect();
assertNotNull(actualSnapshots);
Expand Down Expand Up @@ -1009,8 +1008,7 @@ public void testPrometheusMetricsFilter() throws Exception {
NamedList<?> values = resp.getValues();
assertNotNull(values.get("metrics"));
values = (NamedList<?>) values.get("metrics");
SolrPrometheusFormatter formatter =
(SolrPrometheusFormatter) values.get("solr.core.collection1");
SolrPrometheusFormatter formatter = (SolrPrometheusFormatter) values.get("solr.core");
assertNotNull(formatter);
MetricSnapshots actualSnapshots = formatter.collect();
assertNotNull(actualSnapshots);
Expand Down
25 changes: 13 additions & 12 deletions solr/core/src/test/org/apache/solr/metrics/SolrCoreMetricTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,26 @@
public class SolrCoreMetricTest extends SolrTestCaseJ4 {

@Test
public void testStandaloneDefaultLabels() throws InterruptedException {
String expectedCoreName = "test_core";
public void testStandaloneDefaultLabels() {
String dropwizardMetricName = "core_Core_Name.test_core_metric";

String expectedCoreName = "Core_Name";
String expectedMetricName = "test_core_metric";
Labels expectedLabels = Labels.of("core", expectedCoreName);
TestSolrCoreMetric testSolrCoreMetric =
new TestSolrCoreMetric(null, expectedCoreName, expectedMetricName, false);
TestSolrCoreMetric testSolrCoreMetric = new TestSolrCoreMetric(null, dropwizardMetricName);

assertEquals(expectedCoreName, testSolrCoreMetric.coreName);
assertEquals(expectedMetricName, testSolrCoreMetric.metricName);
assertEquals(expectedLabels, testSolrCoreMetric.getLabels());
}

@Test
public void testCloudDefaultLabels() throws InterruptedException {
String expectedCoreName = "core_test_core_shard1_replica_n1";
public void testCloudDefaultLabels() {
String dropwizardMetricName = "core_Collection_Name_shard1_replica_n1.test_core_metric";

String expectedCoreName = "core_Collection_Name_shard1_replica_n1";
String expectedMetricName = "test_core_metric";
String expectedCollectionName = "test_core";
String expectedCollectionName = "Collection_Name";
String expectedShardName = "shard1";
String expectedReplicaName = "replica_n1";

Expand All @@ -56,18 +59,16 @@ public void testCloudDefaultLabels() throws InterruptedException {
expectedShardName,
"replica",
expectedReplicaName);
TestSolrCoreMetric testSolrCoreMetric =
new TestSolrCoreMetric(null, expectedCoreName, expectedMetricName, true);
TestSolrCoreMetric testSolrCoreMetric = new TestSolrCoreMetric(null, dropwizardMetricName);

assertEquals(expectedCoreName, testSolrCoreMetric.coreName);
assertEquals(expectedMetricName, testSolrCoreMetric.metricName);
assertEquals(expectedLabels, testSolrCoreMetric.getLabels());
}

static class TestSolrCoreMetric extends SolrCoreMetric {
public TestSolrCoreMetric(
Metric dropwizardMetric, String coreName, String metricName, boolean cloudMode) {
super(dropwizardMetric, coreName, metricName, cloudMode);
public TestSolrCoreMetric(Metric dropwizardMetric, String metricName) {
super(dropwizardMetric, metricName);
}

@Override
Expand Down
Loading
Loading