mlbiscoc commented on code in PR #4057: URL: https://github.com/apache/solr/pull/4057#discussion_r2710084381
########## solr/core/src/test/org/apache/solr/handler/admin/api/GetMetricsTest.java: ########## @@ -0,0 +1,230 @@ +/* + * 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 jakarta.ws.rs.core.StreamingOutput; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.Map; +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.params.CommonParams; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.metrics.MetricsUtil; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.request.SolrQueryRequestBase; +import org.apache.solr.response.SolrQueryResponse; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +public class GetMetricsTest extends SolrTestCaseJ4 { + + private static CoreContainer cc; + private static Path outputPath; + + @BeforeClass + public static void beforeClass() throws Exception { + initCore("solrconfig-minimal.xml", "schema.xml"); + h.getCoreContainer().waitForLoadingCoresToFinish(30000); + cc = h.getCoreContainer(); + outputPath = createTempDir(); + } + + @AfterClass + public static void afterClass() throws Exception { + SolrTestCaseJ4.teardownTestCases(); + } + + @Test + public void testGetMetricsDefault() throws IOException { + String expectedHelp = "# HELP solr_core_disk_space_megabytes"; + String expectedType = "# TYPE solr_core_disk_space_megabytes"; + String expectedDiskSpaceMetric = + """ + solr_core_disk_space_megabytes{category="CORE",core="collection1",otel_scope_name="org.apache.solr",type="total_space"} + """; + SolrQueryRequest req = new SolrQueryRequestBase(h.getCore(), new ModifiableSolrParams()) {}; + SolrQueryResponse resp = new SolrQueryResponse(); + + GetMetrics get = new GetMetrics(cc, req, resp); + StreamingOutput output = get.getMetrics(); + Assert.assertNotNull(output); + + Path tmpFile = Files.createTempFile(outputPath, "test-", "-GetMetricsDefault"); + try (OutputStream tmpOut = + Files.newOutputStream( + tmpFile, + StandardOpenOption.CREATE, + StandardOpenOption.TRUNCATE_EXISTING, + StandardOpenOption.WRITE)) { + output.write(tmpOut); + } + try (InputStream tmpIn = Files.newInputStream(tmpFile, StandardOpenOption.READ)) { + byte[] bytes = tmpIn.readNBytes(1024); + String str = new String(bytes, StandardCharsets.UTF_8); + // System.out.println(str); + Assert.assertTrue(str.contains(expectedHelp)); + Assert.assertTrue(str.contains(expectedType)); + Assert.assertTrue(str.contains(expectedDiskSpaceMetric.trim())); + } + Files.delete(tmpFile); + } + + @Test + public void testGetMetricsPrometheus() throws IOException { + String expectedHelp = "# HELP solr_core_disk_space_megabytes"; + String expectedType = "# TYPE solr_core_disk_space_megabytes"; + String expectedDiskSpaceMetric = + """ + solr_core_disk_space_megabytes{category="CORE",core="collection1",otel_scope_name="org.apache.solr",type="total_space"} + """; + SolrQueryRequest req = req(CommonParams.WT, MetricsUtil.PROMETHEUS_METRICS_WT); + SolrQueryResponse resp = new SolrQueryResponse(); + + GetMetrics get = new GetMetrics(cc, req, resp); + StreamingOutput output = get.getMetrics(); + Assert.assertNotNull(output); + + Path tmpFile = Files.createTempFile(outputPath, "test-", "-GetMetricsPrometheus"); + try (OutputStream tmpOut = + Files.newOutputStream( + tmpFile, + StandardOpenOption.CREATE, + StandardOpenOption.TRUNCATE_EXISTING, + StandardOpenOption.WRITE)) { + output.write(tmpOut); + } + try (InputStream tmpIn = Files.newInputStream(tmpFile, StandardOpenOption.READ)) { + byte[] bytes = tmpIn.readNBytes(1024); + String str = new String(bytes, StandardCharsets.UTF_8); + // System.out.println(str); + Assert.assertTrue(str.contains(expectedHelp)); + Assert.assertTrue(str.contains(expectedType)); + Assert.assertTrue(str.contains(expectedDiskSpaceMetric.trim())); + } + Files.delete(tmpFile); + } + + @Test + public void testGetMetricsOpenMetrics() throws IOException { + String expectedHelp = "# HELP solr_core_disk_space_megabytes"; + String expectedType = "# TYPE solr_core_disk_space_megabytes"; + String expectedDiskSpaceMetric = + """ + solr_core_disk_space_megabytes{category="CORE",core="collection1",otel_scope_name="org.apache.solr",type="total_space"} + """; + SolrQueryRequest req = req(CommonParams.WT, MetricsUtil.OPEN_METRICS_WT); + SolrQueryResponse resp = new SolrQueryResponse(); + + GetMetrics get = new GetMetrics(cc, req, resp); + StreamingOutput output = get.getMetrics(); + Assert.assertNotNull(output); + + Path tmpFile = Files.createTempFile(outputPath, "test-", "-GetMetricsOpenMetrics"); + try (OutputStream tmpOut = + Files.newOutputStream( + tmpFile, + StandardOpenOption.CREATE, + StandardOpenOption.TRUNCATE_EXISTING, + StandardOpenOption.WRITE)) { + output.write(tmpOut); + } + try (InputStream tmpIn = Files.newInputStream(tmpFile, StandardOpenOption.READ)) { + byte[] bytes = tmpIn.readNBytes(1024); + String str = new String(bytes, StandardCharsets.UTF_8); + // System.out.println(str); Review Comment: Left in comment here and a few places. Lets clean it up ########## solr/solr-ref-guide/modules/deployment-guide/pages/metrics-reporting.adoc: ########## @@ -148,14 +153,14 @@ The `prometheus-config.yml` file needs to be configured for a Prometheus server ---- scrape_configs: - job_name: 'solr' - metrics_path: "/solr/admin/metrics" + metrics_path: "/api/metrics" static_configs: - targets: ['localhost:8983', 'localhost:7574'] ---- === OpenMetrics -OpenMetrics format is available from the `/admin/metrics` endpoint by providing the `wt=openmetrics` parameter or by passing the Accept header `application/openmetrics-text;version=1.0.0`. OpenMetrics is an extension of the Prometheus format that adds additional metadata and exemplars. +OpenMetrics format is available from the `/metrics` endpoint by providing the `wt=openmetrics` parameter or by passing the Accept header `application/openmetrics-text;version=1.0.0`. OpenMetrics is an extension of the Prometheus format that adds additional metadata and exemplars. Review Comment: I had written something custom to check content-type but just for OpenMetrics but not prometheus. See my comment above. ########## 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: In that case see https://prometheus.io/docs/instrumenting/content_negotiation/#accept-header-construction for the accept header for prometheus vs openmetrics. I had done a content type and accept header for OpenMetrics [here](https://github.com/apache/solr/blob/c95fa2bf863bebc5654e1bc198958d8ab0ebba4a/solr/core/src/test/org/apache/solr/response/TestPrometheusResponseWriter.java#L117). I guess this is a good time to actually do it for prometheus ########## 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: That was me who put it there originally probably. Please improve anything you think can use it. Thank you! ########## solr/core/src/test/org/apache/solr/handler/admin/api/GetMetricsTest.java: ########## @@ -0,0 +1,230 @@ +/* + * 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 jakarta.ws.rs.core.StreamingOutput; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.Map; +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.params.CommonParams; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.metrics.MetricsUtil; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.request.SolrQueryRequestBase; +import org.apache.solr.response.SolrQueryResponse; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +public class GetMetricsTest extends SolrTestCaseJ4 { + + private static CoreContainer cc; + private static Path outputPath; + + @BeforeClass + public static void beforeClass() throws Exception { + initCore("solrconfig-minimal.xml", "schema.xml"); + h.getCoreContainer().waitForLoadingCoresToFinish(30000); + cc = h.getCoreContainer(); + outputPath = createTempDir(); + } + + @AfterClass + public static void afterClass() throws Exception { + SolrTestCaseJ4.teardownTestCases(); + } + + @Test + public void testGetMetricsDefault() throws IOException { + String expectedHelp = "# HELP solr_core_disk_space_megabytes"; + String expectedType = "# TYPE solr_core_disk_space_megabytes"; + String expectedDiskSpaceMetric = + """ + solr_core_disk_space_megabytes{category="CORE",core="collection1",otel_scope_name="org.apache.solr",type="total_space"} + """; + SolrQueryRequest req = new SolrQueryRequestBase(h.getCore(), new ModifiableSolrParams()) {}; + SolrQueryResponse resp = new SolrQueryResponse(); + + GetMetrics get = new GetMetrics(cc, req, resp); + StreamingOutput output = get.getMetrics(); + Assert.assertNotNull(output); + + Path tmpFile = Files.createTempFile(outputPath, "test-", "-GetMetricsDefault"); + try (OutputStream tmpOut = + Files.newOutputStream( + tmpFile, + StandardOpenOption.CREATE, + StandardOpenOption.TRUNCATE_EXISTING, + StandardOpenOption.WRITE)) { + output.write(tmpOut); + } + try (InputStream tmpIn = Files.newInputStream(tmpFile, StandardOpenOption.READ)) { + byte[] bytes = tmpIn.readNBytes(1024); + String str = new String(bytes, StandardCharsets.UTF_8); + // System.out.println(str); + Assert.assertTrue(str.contains(expectedHelp)); + Assert.assertTrue(str.contains(expectedType)); + Assert.assertTrue(str.contains(expectedDiskSpaceMetric.trim())); + } + Files.delete(tmpFile); + } + + @Test + public void testGetMetricsPrometheus() throws IOException { + String expectedHelp = "# HELP solr_core_disk_space_megabytes"; + String expectedType = "# TYPE solr_core_disk_space_megabytes"; + String expectedDiskSpaceMetric = + """ + solr_core_disk_space_megabytes{category="CORE",core="collection1",otel_scope_name="org.apache.solr",type="total_space"} + """; + SolrQueryRequest req = req(CommonParams.WT, MetricsUtil.PROMETHEUS_METRICS_WT); + SolrQueryResponse resp = new SolrQueryResponse(); + + GetMetrics get = new GetMetrics(cc, req, resp); + StreamingOutput output = get.getMetrics(); + Assert.assertNotNull(output); + + Path tmpFile = Files.createTempFile(outputPath, "test-", "-GetMetricsPrometheus"); + try (OutputStream tmpOut = + Files.newOutputStream( + tmpFile, + StandardOpenOption.CREATE, + StandardOpenOption.TRUNCATE_EXISTING, + StandardOpenOption.WRITE)) { + output.write(tmpOut); + } + try (InputStream tmpIn = Files.newInputStream(tmpFile, StandardOpenOption.READ)) { + byte[] bytes = tmpIn.readNBytes(1024); + String str = new String(bytes, StandardCharsets.UTF_8); + // System.out.println(str); + Assert.assertTrue(str.contains(expectedHelp)); + Assert.assertTrue(str.contains(expectedType)); + Assert.assertTrue(str.contains(expectedDiskSpaceMetric.trim())); + } + Files.delete(tmpFile); + } + + @Test + public void testGetMetricsOpenMetrics() throws IOException { Review Comment: See my tests in `TestPrometheusResponseWriter`. For this OpenMetrics format, the easiest way I know if openmetrics format exists is if `# EOF` actually exists. Otherwise this test from what I can see would still pass for regular prometheus format. (They are very very similar and only way you'll see a major difference is if you turn on distributed trace as well. You can see that in `TestMetricExemplars` for that test but not saying you need to make something like that here.) ########## solr/core/src/java/org/apache/solr/metrics/MetricsUtil.java: ########## Review Comment: We have a `MetricUtils` class under a util package and now `MetricsUtil`. Kinda awkward having both of these exist. I would merge or keep one or the other. I would get rid of one of them and merge these into one file. But also many of these utility methods are so specific to this metrics thing like merging the snapshots, I am not sure if someone would use these utility classes anywhere else and maybe just keeping it static in `GetMetrics` class works as well so `MetricsHandler` has access but not sure if this is a bad pattern to do. Up to you, but I definitely think at the minimum drop one of the Util classes. ########## 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: I don't see a path where we will bring JSON metrics back unless some popular format for reading metrics in such a way happens with OTEL or we for some reason need to bridge JSON metrics again. I'd rather avoid that latter though. -- 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]
