gerlowskija commented on code in PR #4057: URL: https://github.com/apache/solr/pull/4057#discussion_r2705571466
########## solr/core/src/java/org/apache/solr/metrics/MetricsUtil.java: ########## @@ -0,0 +1,186 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.solr.metrics; + +import io.prometheus.metrics.model.snapshots.CounterSnapshot; +import io.prometheus.metrics.model.snapshots.GaugeSnapshot; +import io.prometheus.metrics.model.snapshots.HistogramSnapshot; +import io.prometheus.metrics.model.snapshots.InfoSnapshot; +import io.prometheus.metrics.model.snapshots.MetricSnapshot; +import io.prometheus.metrics.model.snapshots.MetricSnapshots; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.SortedMap; +import java.util.TreeMap; +import org.apache.solr.common.params.SolrParams; +import org.apache.solr.common.util.StrUtils; + +/** Utility methods for Metrics */ +public class MetricsUtil { + + public static final String PROMETHEUS_METRICS_WT = "prometheus"; + public static final String OPEN_METRICS_WT = "openmetrics"; + + 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_TYPE_PARAM = "replica_type"; + public static final String METRIC_NAME_PARAM = "name"; + private static final Set<String> labelFilterKeys = + Set.of(CATEGORY_PARAM, CORE_PARAM, COLLECTION_PARAM, SHARD_PARAM, REPLICA_TYPE_PARAM); + + /** + * 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 under the same prometheus grouping. + */ + public static MetricSnapshots mergeSnapshots(List<MetricSnapshot> snapshots) { + Map<String, CounterSnapshot.Builder> counterSnapshotMap = new HashMap<>(); + Map<String, GaugeSnapshot.Builder> gaugeSnapshotMap = new HashMap<>(); + Map<String, HistogramSnapshot.Builder> histogramSnapshotMap = new HashMap<>(); + InfoSnapshot otelInfoSnapshots = null; + + for (MetricSnapshot snapshot : snapshots) { + String metricName = snapshot.getMetadata().getPrometheusName(); + + switch (snapshot) { + case CounterSnapshot counterSnapshot -> { + CounterSnapshot.Builder builder = + counterSnapshotMap.computeIfAbsent( + metricName, + k -> { + var base = + CounterSnapshot.builder() + .name(counterSnapshot.getMetadata().getName()) + .help(counterSnapshot.getMetadata().getHelp()); + return counterSnapshot.getMetadata().hasUnit() + ? base.unit(counterSnapshot.getMetadata().getUnit()) + : base; + }); + counterSnapshot.getDataPoints().forEach(builder::dataPoint); + } + case GaugeSnapshot gaugeSnapshot -> { + GaugeSnapshot.Builder builder = + gaugeSnapshotMap.computeIfAbsent( + metricName, + k -> { + var base = + GaugeSnapshot.builder() + .name(gaugeSnapshot.getMetadata().getName()) + .help(gaugeSnapshot.getMetadata().getHelp()); + return gaugeSnapshot.getMetadata().hasUnit() + ? base.unit(gaugeSnapshot.getMetadata().getUnit()) + : base; + }); + gaugeSnapshot.getDataPoints().forEach(builder::dataPoint); + } + case HistogramSnapshot histogramSnapshot -> { + HistogramSnapshot.Builder builder = + histogramSnapshotMap.computeIfAbsent( + metricName, + k -> { + var base = + HistogramSnapshot.builder() + .name(histogramSnapshot.getMetadata().getName()) + .help(histogramSnapshot.getMetadata().getHelp()); + return histogramSnapshot.getMetadata().hasUnit() + ? base.unit(histogramSnapshot.getMetadata().getUnit()) + : base; + }); + histogramSnapshot.getDataPoints().forEach(builder::dataPoint); + } + case InfoSnapshot infoSnapshot -> { + // InfoSnapshot is a special case in that each SdkMeterProvider will create a duplicate + // metric called target_info containing OTEL SDK metadata. Only one of these need to be + // kept + if (otelInfoSnapshots == null) + otelInfoSnapshots = + new InfoSnapshot(infoSnapshot.getMetadata(), infoSnapshot.getDataPoints()); + } + default -> { + // Handle unexpected snapshot types gracefully + } + } + } + + MetricSnapshots.Builder snapshotsBuilder = MetricSnapshots.builder(); + counterSnapshotMap.values().forEach(b -> snapshotsBuilder.metricSnapshot(b.build())); + gaugeSnapshotMap.values().forEach(b -> snapshotsBuilder.metricSnapshot(b.build())); + histogramSnapshotMap.values().forEach(b -> snapshotsBuilder.metricSnapshot(b.build())); + if (otelInfoSnapshots != null) snapshotsBuilder.metricSnapshot(otelInfoSnapshots); + return snapshotsBuilder.build(); + } + + /** Gather label filters */ + public static SortedMap<String, Set<String>> labelFilters(SolrParams params) { + SortedMap<String, Set<String>> labelFilters = new TreeMap<>(); + labelFilterKeys.forEach( + (paramName) -> { + Set<String> filterValues = readParamsAsSet(params, paramName); + if (!filterValues.isEmpty()) { + labelFilters.put(paramName, filterValues); + } + }); + + return labelFilters; + } + + /** Add label filters to the filters map */ + public static void addLabelFilters(String value, Map<String, Set<String>> filters) { + labelFilterKeys.forEach( + (paramName) -> { + Set<String> filterValues = paramValueAsSet(value); + if (!filterValues.isEmpty()) { + filters.put(paramName, filterValues); + } + }); + } + + /** Split the coma-separated param values into a set */ + public static Set<String> paramValueAsSet(String paramValue) { + String[] values = paramValue.split(","); + List<String> valuesSet = new ArrayList<>(); + for (String value : values) { + valuesSet.add(value); + } + return Set.copyOf(valuesSet); + } + + /** + * Read Solr parameters as a Set. + * + * <p>Could probably be moved to a more generic utility class, but only used in MetricsHandler and + * GetMetrics resource. + */ + public static Set<String> readParamsAsSet(SolrParams params, String paramName) { + String[] paramValues = params.getParams(paramName); + if (paramValues == null || paramValues.length == 0) { + return Set.of(); + } + + List<String> paramSet = new ArrayList<>(); Review Comment: [Q] Could this be a Set to save on the "copyOf" later? ########## solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java: ########## @@ -863,11 +863,16 @@ public static void checkDiskSpace( new ModifiableSolrParams() .add("key", indexSizeMetricName) .add("key", freeDiskSpaceMetricName); + // TODO: SOLR-17955 SolrResponse rsp = new MetricsRequest(params).process(cloudManager.getSolrClient()); + if (rsp == null) { + log.warn("No Solr response available from parent shard leader."); + return; + } Number size = (Number) rsp.getResponse()._get(List.of("metrics", indexSizeMetricName), null); if (size == null) { - log.warn("cannot verify information for parent shard leader"); + log.warn("missing index size information for parent shard leader"); Review Comment: [+1] Great little logging clarification; thanks! ########## solr/core/src/java/org/apache/solr/handler/admin/api/GetMetrics.java: ########## @@ -0,0 +1,159 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.solr.handler.admin.api; + +import io.prometheus.metrics.model.snapshots.MetricSnapshot; +import io.prometheus.metrics.model.snapshots.MetricSnapshots; +import jakarta.inject.Inject; +import jakarta.ws.rs.WebApplicationException; +import jakarta.ws.rs.core.StreamingOutput; +import java.io.IOException; +import java.io.OutputStream; +import java.lang.invoke.MethodHandles; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.SortedMap; +import org.apache.solr.client.api.endpoint.MetricsApi; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.CommonParams; +import org.apache.solr.common.params.SolrParams; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.handler.admin.AdminHandlersProxy; +import org.apache.solr.jersey.PermissionName; +import org.apache.solr.metrics.MetricsUtil; +import org.apache.solr.metrics.SolrMetricManager; +import org.apache.solr.metrics.otel.FilterablePrometheusMetricReader; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.PrometheusResponseWriter; +import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.security.PermissionNameProvider; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * V2 API implementation to fetch metrics gathered by Solr. + * + * <p>This API is analogous to the v1 /admin/metrics endpoint. + */ +public class GetMetrics extends AdminAPIBase implements MetricsApi { + + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + + private final SolrMetricManager metricManager; + private final boolean enabled; + + @Inject + public GetMetrics( + CoreContainer coreContainer, + SolrQueryRequest solrQueryRequest, + SolrQueryResponse solrQueryResponse) { + super(coreContainer, solrQueryRequest, solrQueryResponse); + this.metricManager = coreContainer.getMetricManager(); + this.enabled = coreContainer.getConfig().getMetricsConfig().isEnabled(); + } + + @Override + @PermissionName(PermissionNameProvider.Name.METRICS_READ_PERM) + public StreamingOutput getMetrics() { + + validateRequest(); + + if (proxyToNodes()) { + return null; + } + + SolrParams params = solrQueryRequest.getParams(); + + Set<String> metricNames = MetricsUtil.readParamsAsSet(params, MetricsUtil.METRIC_NAME_PARAM); + SortedMap<String, Set<String>> labelFilters = MetricsUtil.labelFilters(params); + + return doGetMetrics(metricNames, labelFilters); + } + + private void validateRequest() { + + if (!enabled) { + throw new SolrException( + SolrException.ErrorCode.INVALID_STATE, "Metrics collection is disabled"); + } + + if (metricManager == null) { + throw new SolrException( + SolrException.ErrorCode.INVALID_STATE, "SolrMetricManager instance not initialized"); + } + + SolrParams params = solrQueryRequest.getParams(); Review Comment: [-1] Solr's v1 APIs rely on "wt" to indicate what response should be sent to users, but in our v2 APIs we're trying to use the more HTTP standard ["Accept" header](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Accept) for this sort of response-format negotiation thing. (In fact, we have a "[pre-request filter](https://github.com/apache/solr/blob/main/solr/core/src/java/org/apache/solr/jersey/MediaTypeOverridingFilter.java)" that runs before any v2 API that attempts to convert any user-provided "wt" params into an "Accept" header equivalent so that our v2 code only has to check the header.) So, rather than fetching "wt" here out of SolrQueryRequest, IMO we should add a parameter to this method representing the accept header, and then key off of that instead. Something like the code below would work I think: ``` @HeaderParam("Accept") String acceptHeader ``` ########## solr/core/src/java/org/apache/solr/handler/admin/api/GetMetrics.java: ########## @@ -0,0 +1,159 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.solr.handler.admin.api; + +import io.prometheus.metrics.model.snapshots.MetricSnapshot; +import io.prometheus.metrics.model.snapshots.MetricSnapshots; +import jakarta.inject.Inject; +import jakarta.ws.rs.WebApplicationException; +import jakarta.ws.rs.core.StreamingOutput; +import java.io.IOException; +import java.io.OutputStream; +import java.lang.invoke.MethodHandles; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.SortedMap; +import org.apache.solr.client.api.endpoint.MetricsApi; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.CommonParams; +import org.apache.solr.common.params.SolrParams; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.handler.admin.AdminHandlersProxy; +import org.apache.solr.jersey.PermissionName; +import org.apache.solr.metrics.MetricsUtil; +import org.apache.solr.metrics.SolrMetricManager; +import org.apache.solr.metrics.otel.FilterablePrometheusMetricReader; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.PrometheusResponseWriter; +import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.security.PermissionNameProvider; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * V2 API implementation to fetch metrics gathered by Solr. + * + * <p>This API is analogous to the v1 /admin/metrics endpoint. + */ +public class GetMetrics extends AdminAPIBase implements MetricsApi { + + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + + private final SolrMetricManager metricManager; + private final boolean enabled; + + @Inject + public GetMetrics( + CoreContainer coreContainer, + SolrQueryRequest solrQueryRequest, + SolrQueryResponse solrQueryResponse) { + super(coreContainer, solrQueryRequest, solrQueryResponse); + this.metricManager = coreContainer.getMetricManager(); + this.enabled = coreContainer.getConfig().getMetricsConfig().isEnabled(); + } + + @Override + @PermissionName(PermissionNameProvider.Name.METRICS_READ_PERM) + public StreamingOutput getMetrics() { + + validateRequest(); + + if (proxyToNodes()) { + return null; + } + + SolrParams params = solrQueryRequest.getParams(); + + Set<String> metricNames = MetricsUtil.readParamsAsSet(params, MetricsUtil.METRIC_NAME_PARAM); Review Comment: [-1] One of the goals of the JAX-RS framework is to make the inputs and outputs of each API as clear as possible. It'll undermine the code re-use you were trying to get out of the "MetricsUtil" stuff (which I very much appreciate!), but IMO we need to declare this METRIC_NAME_PARAM input in the method signature. This makes the code here easier to read as folks know at a glance what the inputs are, and it ensures that the OpenAPI spec (and the Java, Python, Javascript, etc. clients generated from that) know about the parameters this API expects. Something like: ``` @QueryParam("metricNames") String[] metricNames ``` should work I think? ########## solr/api/src/java/org/apache/solr/client/api/endpoint/MetricsApi.java: ########## @@ -0,0 +1,40 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.solr.client.api.endpoint; + +import static org.apache.solr.client.api.util.Constants.RAW_OUTPUT_PROPERTY; + +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.extensions.Extension; +import io.swagger.v3.oas.annotations.extensions.ExtensionProperty; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.core.StreamingOutput; + +/** V2 API definitions to fetch metrics. */ +@Path("/metrics") +public interface MetricsApi { + + @GET + @Operation( + summary = "Retrieve metrics gathered by Solr.", + tags = {"metrics"}, + extensions = { + @Extension(properties = {@ExtensionProperty(name = RAW_OUTPUT_PROPERTY, value = "true")}) Review Comment: [+1] Just came here from SOLR-17436 where you left a comment or two about struggling to get the template-generated code to compile. Glad you were able to find "StreamingOutput" and this property, that's exactly the right approach! ########## solr/api/src/java/org/apache/solr/client/api/model/MetricsResponse.java: ########## @@ -0,0 +1,47 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.solr.client.api.model; + +import com.fasterxml.jackson.annotation.JsonAnyGetter; +import com.fasterxml.jackson.annotation.JsonAnySetter; +import java.util.HashMap; +import java.util.Map; + +/** + * Response from /api/metrics is actually a "prometheus" or "openmetrics" format + * (jakarta.ws.rs.core.StreamingOutput). + * + * <p>This class could be used if a json output is ever needed again? + */ +public class MetricsResponse extends SolrJerseyResponse { Review Comment: [-0] I'll miss our metrics' JSON support, but if there's no active plan to support that then IMO we should take this file out of the PR 😦 . We can always resurrect it later from this PR if adding JSON support comes back up... -- 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]
