dsmiley commented on code in PR #3499:
URL: https://github.com/apache/solr/pull/3499#discussion_r2296349223
##########
solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java:
##########
@@ -151,6 +169,73 @@ private void handleRequest(SolrParams params,
BiConsumer<String, Object> consume
consumer.accept("metrics", response);
}
+ private void handlePrometheusRequest(SolrParams params, BiConsumer<String,
Object> consumer) {
+ Set<String> metricNames = metricNamesFilter(params);
+ Map<String, Set<String>> labelFilters = labelFilters(params);
+
+ if (!metricNames.isEmpty() || !labelFilters.isEmpty()) {
Review Comment:
logic seems wrong. A comment would help clarify the intention. I think
such a comment here would be "no filtering". But then the boolean expression
should be: `metricNames.isEmpty() && labelFilters.isEmpty()`
##########
solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java:
##########
@@ -151,6 +169,73 @@ private void handleRequest(SolrParams params,
BiConsumer<String, Object> consume
consumer.accept("metrics", response);
}
+ private void handlePrometheusRequest(SolrParams params, BiConsumer<String,
Object> consumer) {
+ Set<String> metricNames = metricNamesFilter(params);
+ Map<String, Set<String>> labelFilters = labelFilters(params);
+
+ if (!metricNames.isEmpty() || !labelFilters.isEmpty()) {
+ consumer.accept(
+ "metrics",
+ mergeSnapshots(
+ metricManager.getPrometheusMetricReaders().values().stream()
+ .flatMap(r -> r.collect().stream())
+ .toList()));
+ return;
+ }
+
+ List<MetricSnapshot> allSnapshots = new ArrayList<>();
+ for (FilterablePrometheusMetricReader reader :
+ metricManager.getPrometheusMetricReaders().values()) {
+ MetricSnapshots filteredSnapshots = reader.collect(metricNames,
labelFilters);
+ filteredSnapshots.forEach(allSnapshots::add);
+ }
+
+ // Merge all filtered snapshots and return the merged result
+ MetricSnapshots mergedSnapshots = mergeSnapshots(allSnapshots);
+ consumer.accept("metrics", mergedSnapshots);
+ }
+
+ private Set<String> metricNamesFilter(SolrParams params) {
+ String[] metricNameParams = params.getParams(METRIC_NAME_PARAM);
+ if (metricNameParams == null || metricNameParams.length == 0) {
+ return Collections.emptySet();
Review Comment:
nit: nowadays use `Set.of()`
##########
solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java:
##########
@@ -499,9 +584,87 @@ private List<MetricType> parseMetricTypes(SolrParams
params) {
return metricTypes;
}
- private String getCoreNameFromRegistry(String registryName) {
- String coreName = registryName.substring(registryName.indexOf('.') + 1);
- return coreName.replace(".", "_");
+ /**
+ * Merge a collection of individual {@link MetricSnapshot} instances into
one {@link
+ * MetricSnapshots}. This is necessary because we create a {@link
+ * io.opentelemetry.sdk.metrics.SdkMeterProvider} per Solr core resulting in
duplicate metric
+ * names across cores which is an illegal format if not under the same
prometheus grouping.
Review Comment:
> if not under
Don't you mean the inverse, "if under" ?
##########
solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java:
##########
@@ -151,6 +169,73 @@ private void handleRequest(SolrParams params,
BiConsumer<String, Object> consume
consumer.accept("metrics", response);
}
+ private void handlePrometheusRequest(SolrParams params, BiConsumer<String,
Object> consumer) {
+ Set<String> metricNames = metricNamesFilter(params);
+ Map<String, Set<String>> labelFilters = labelFilters(params);
+
+ if (!metricNames.isEmpty() || !labelFilters.isEmpty()) {
+ consumer.accept(
+ "metrics",
+ mergeSnapshots(
+ metricManager.getPrometheusMetricReaders().values().stream()
+ .flatMap(r -> r.collect().stream())
+ .toList()));
+ return;
+ }
+
+ List<MetricSnapshot> allSnapshots = new ArrayList<>();
+ for (FilterablePrometheusMetricReader reader :
+ metricManager.getPrometheusMetricReaders().values()) {
+ MetricSnapshots filteredSnapshots = reader.collect(metricNames,
labelFilters);
+ filteredSnapshots.forEach(allSnapshots::add);
+ }
+
+ // Merge all filtered snapshots and return the merged result
+ MetricSnapshots mergedSnapshots = mergeSnapshots(allSnapshots);
+ consumer.accept("metrics", mergedSnapshots);
+ }
+
+ private Set<String> metricNamesFilter(SolrParams params) {
+ String[] metricNameParams = params.getParams(METRIC_NAME_PARAM);
+ if (metricNameParams == null || metricNameParams.length == 0) {
+ return Collections.emptySet();
+ }
+
+ Set<String> metricNames = new HashSet<>();
+ for (String param : metricNameParams) {
+ if (param != null && !param.trim().isEmpty()) {
Review Comment:
A null value is impossible. And no empty check is necessary; splitSmart
handles that. Thus you can keep this simple and drop the condition.
##########
solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java:
##########
@@ -151,6 +169,73 @@ private void handleRequest(SolrParams params,
BiConsumer<String, Object> consume
consumer.accept("metrics", response);
}
+ private void handlePrometheusRequest(SolrParams params, BiConsumer<String,
Object> consumer) {
+ Set<String> metricNames = metricNamesFilter(params);
+ Map<String, Set<String>> labelFilters = labelFilters(params);
+
+ if (!metricNames.isEmpty() || !labelFilters.isEmpty()) {
+ consumer.accept(
+ "metrics",
+ mergeSnapshots(
+ metricManager.getPrometheusMetricReaders().values().stream()
+ .flatMap(r -> r.collect().stream())
+ .toList()));
+ return;
+ }
+
+ List<MetricSnapshot> allSnapshots = new ArrayList<>();
+ for (FilterablePrometheusMetricReader reader :
+ metricManager.getPrometheusMetricReaders().values()) {
+ MetricSnapshots filteredSnapshots = reader.collect(metricNames,
labelFilters);
+ filteredSnapshots.forEach(allSnapshots::add);
+ }
+
+ // Merge all filtered snapshots and return the merged result
+ MetricSnapshots mergedSnapshots = mergeSnapshots(allSnapshots);
+ consumer.accept("metrics", mergedSnapshots);
+ }
+
+ private Set<String> metricNamesFilter(SolrParams params) {
+ String[] metricNameParams = params.getParams(METRIC_NAME_PARAM);
+ if (metricNameParams == null || metricNameParams.length == 0) {
+ return Collections.emptySet();
+ }
+
+ Set<String> metricNames = new HashSet<>();
+ for (String param : metricNameParams) {
+ if (param != null && !param.trim().isEmpty()) {
+ metricNames.addAll(StrUtils.splitSmart(param, ','));
+ }
+ }
+ return metricNames;
+ }
+
+ private Map<String, Set<String>> labelFilters(SolrParams params) {
+ Map<String, Set<String>> labelFilters = new HashMap<>();
+ labelFilterKeys.forEach(
+ (paramName) -> {
+ Set<String> filterValues = labelFilterValues(params, paramName);
+ if (!filterValues.isEmpty()) {
+ labelFilters.put(paramName, filterValues);
+ }
+ });
+
+ return labelFilters;
+ }
+
+ private Set<String> labelFilterValues(SolrParams params, String paramName) {
+ String[] filterLabels = params.getParams(paramName);
+ Set<String> labelValues = new HashSet<>();
+ if (filterLabels != null) {
+ for (String labels : filterLabels) {
+ if (labels != null && !labels.trim().isEmpty()) {
Review Comment:
same feedback as earlier.
Maybe should have a general method readParamAsSet(params, name, char delim)
##########
solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java:
##########
@@ -151,6 +169,73 @@ private void handleRequest(SolrParams params,
BiConsumer<String, Object> consume
consumer.accept("metrics", response);
}
+ private void handlePrometheusRequest(SolrParams params, BiConsumer<String,
Object> consumer) {
+ Set<String> metricNames = metricNamesFilter(params);
+ Map<String, Set<String>> labelFilters = labelFilters(params);
+
+ if (!metricNames.isEmpty() || !labelFilters.isEmpty()) {
+ consumer.accept(
+ "metrics",
+ mergeSnapshots(
+ metricManager.getPrometheusMetricReaders().values().stream()
+ .flatMap(r -> r.collect().stream())
+ .toList()));
+ return;
+ }
+
+ List<MetricSnapshot> allSnapshots = new ArrayList<>();
+ for (FilterablePrometheusMetricReader reader :
+ metricManager.getPrometheusMetricReaders().values()) {
+ MetricSnapshots filteredSnapshots = reader.collect(metricNames,
labelFilters);
+ filteredSnapshots.forEach(allSnapshots::add);
+ }
+
+ // Merge all filtered snapshots and return the merged result
+ MetricSnapshots mergedSnapshots = mergeSnapshots(allSnapshots);
+ consumer.accept("metrics", mergedSnapshots);
+ }
+
+ private Set<String> metricNamesFilter(SolrParams params) {
+ String[] metricNameParams = params.getParams(METRIC_NAME_PARAM);
+ if (metricNameParams == null || metricNameParams.length == 0) {
+ return Collections.emptySet();
+ }
+
+ Set<String> metricNames = new HashSet<>();
Review Comment:
I recommend instead collecting to a List then finally calling
`Set.copyOf(list)`. These immutable implementations are very efficient; I try
to use them whenever I need an immutable List, Set, Map, especially if it's
going to be evaluated/used multiple times.
##########
solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java:
##########
@@ -69,6 +77,16 @@ public class MetricsHandler extends RequestHandlerBase
implements PermissionName
public static final String KEY_PARAM = "key";
public static final String EXPR_PARAM = "expr";
public static final String TYPE_PARAM = "type";
+ // Prometheus filtering parameters
+ public static final String CATEGORY_PARAM = "category";
+ public static final String CORE_PARAM = "core";
+ public static final String COLLECTION_PARAM = "collection";
+ public static final String SHARD_PARAM = "shard";
+ public static final String REPLICA_PARAM = "replica";
+ public static final String METRIC_NAME_PARAM = "name";
+ private final Set<String> labelFilterKeys =
Review Comment:
not static?
##########
solr/core/src/java/org/apache/solr/metrics/SolrMetricManager.java:
##########
@@ -1661,17 +1661,17 @@ public MetricsConfig getMetricsConfig() {
return metricsConfig;
}
- /** Get a shallow copied map of {@link PrometheusMetricReader}. */
- public Map<String, PrometheusMetricReader> getPrometheusMetricReaders() {
+ /** Get a shallow copied map of {@link FilterablePrometheusMetricReader}. */
Review Comment:
nit: just because the precise implementation is filterable, doesn't mean you
need to be so precise in the javadoc. Whatever though.
##########
solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java:
##########
@@ -499,9 +584,87 @@ private List<MetricType> parseMetricTypes(SolrParams
params) {
return metricTypes;
}
- private String getCoreNameFromRegistry(String registryName) {
- String coreName = registryName.substring(registryName.indexOf('.') + 1);
- return coreName.replace(".", "_");
+ /**
+ * Merge a collection of individual {@link MetricSnapshot} instances into
one {@link
+ * MetricSnapshots}. This is necessary because we create a {@link
+ * io.opentelemetry.sdk.metrics.SdkMeterProvider} per Solr core resulting in
duplicate metric
+ * names across cores which is an illegal format if not under the same
prometheus grouping.
+ */
+ private MetricSnapshots mergeSnapshots(List<MetricSnapshot> snapshots) {
Review Comment:
deja vu... did you take this from elsewhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
