dsmiley commented on code in PR #2405: URL: https://github.com/apache/solr/pull/2405#discussion_r1641064461
########## solr/core/src/java/org/apache/solr/metrics/prometheus/jvm/SolrJvmBuffersMetric.java: ########## @@ -0,0 +1,52 @@ +/* + * 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.prometheus.jvm; + +import com.codahale.metrics.Gauge; +import com.codahale.metrics.Metric; +import org.apache.solr.metrics.prometheus.SolrPrometheusExporter; + +/* Dropwizard metrics of name buffers.* */ +public class SolrJvmBuffersMetric extends SolrJvmMetric { + public static final String JVM_BUFFERS = "solr_metrics_jvm_buffers"; + public static final String JVM_BUFFERS_BYTES = "solr_metrics_jvm_buffers_bytes"; + + public SolrJvmBuffersMetric(Metric dropwizardMetric, String metricName) { + super(dropwizardMetric, metricName); + } + + @Override + public SolrJvmMetric parseLabels() { + String[] parsedMetric = metricName.split("\\."); Review Comment: When parsing stuff, it really helps to have a one-line comment of a sample value ########## solr/core/src/java/org/apache/solr/metrics/prometheus/node/SolrPrometheusNodeExporter.java: ########## @@ -0,0 +1,99 @@ +/* + * 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.prometheus.node; + +import static org.apache.solr.metrics.prometheus.node.SolrNodeMetric.NODE_THREAD_POOL; + +import com.codahale.metrics.Counter; +import com.codahale.metrics.Meter; +import com.codahale.metrics.Metric; +import com.google.common.base.Enums; +import io.prometheus.metrics.model.snapshots.Labels; +import org.apache.solr.metrics.prometheus.SolrMetric; +import org.apache.solr.metrics.prometheus.SolrNoOpMetric; +import org.apache.solr.metrics.prometheus.SolrPrometheusExporter; + +/** + * This class maintains a {@link io.prometheus.metrics.model.snapshots.MetricSnapshot}s exported + * from solr.node {@link com.codahale.metrics.MetricRegistry} + */ +public class SolrPrometheusNodeExporter extends SolrPrometheusExporter + implements PrometheusNodeExporterInfo { + public SolrPrometheusNodeExporter() { + super(); + } + + @Override + public void exportDropwizardMetric(Metric dropwizardMetric, String metricName) { + if (metricName.contains(".threadPool.")) { + exportThreadPoolMetric(dropwizardMetric, metricName); + return; + } + + SolrMetric solrNodeMetric = categorizeMetric(dropwizardMetric, metricName); + solrNodeMetric.parseLabels().toPrometheus(this); Review Comment: if we forget to call parseLabels, will we recognize a mistake? not a big deal but just a thought ########## solr/core/src/java/org/apache/solr/metrics/prometheus/node/SolrPrometheusNodeExporter.java: ########## @@ -0,0 +1,99 @@ +/* + * 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.prometheus.node; + +import static org.apache.solr.metrics.prometheus.node.SolrNodeMetric.NODE_THREAD_POOL; + +import com.codahale.metrics.Counter; +import com.codahale.metrics.Meter; +import com.codahale.metrics.Metric; +import com.google.common.base.Enums; +import io.prometheus.metrics.model.snapshots.Labels; +import org.apache.solr.metrics.prometheus.SolrMetric; +import org.apache.solr.metrics.prometheus.SolrNoOpMetric; +import org.apache.solr.metrics.prometheus.SolrPrometheusExporter; + +/** + * This class maintains a {@link io.prometheus.metrics.model.snapshots.MetricSnapshot}s exported + * from solr.node {@link com.codahale.metrics.MetricRegistry} + */ +public class SolrPrometheusNodeExporter extends SolrPrometheusExporter + implements PrometheusNodeExporterInfo { + public SolrPrometheusNodeExporter() { + super(); + } + + @Override + public void exportDropwizardMetric(Metric dropwizardMetric, String metricName) { + if (metricName.contains(".threadPool.")) { + exportThreadPoolMetric(dropwizardMetric, metricName); + return; + } + + SolrMetric solrNodeMetric = categorizeMetric(dropwizardMetric, metricName); + solrNodeMetric.parseLabels().toPrometheus(this); + } + + @Override + public SolrMetric categorizeMetric(Metric dropwizardMetric, String metricName) { + String metricCategory = metricName.split("\\.", 2)[0]; + if (!Enums.getIfPresent(PrometheusNodeExporterInfo.NodeCategory.class, metricCategory) + .isPresent()) { + return new SolrNoOpMetric(); + } + switch (NodeCategory.valueOf(metricCategory)) { + case ADMIN: + case UPDATE: + return new SolrNodeHandlerMetric(dropwizardMetric, metricName); + case CONTAINER: + return new SolrNodeContainerMetric(dropwizardMetric, metricName); + default: + return new SolrNoOpMetric(); + } + } + + private void exportThreadPoolMetric(Metric dropwizardMetric, String metricName) { + Labels labels; + String[] parsedMetric = metricName.split("\\."); Review Comment: again and in many places, a sample value would be awesome ########## solr/core/src/java/org/apache/solr/metrics/prometheus/node/SolrNodeContainerMetric.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.prometheus.node; + +import com.codahale.metrics.Gauge; +import com.codahale.metrics.Metric; +import org.apache.solr.metrics.prometheus.SolrMetric; +import org.apache.solr.metrics.prometheus.SolrPrometheusExporter; + +/* Dropwizard metrics of name CONTAINER.* */ Review Comment: Great; please add similar javadocs for other classes where you forgot ########## solr/core/src/java/org/apache/solr/response/PrometheusResponseWriter.java: ########## @@ -118,13 +108,41 @@ public static void toPrometheus( (metricName, metric) -> { try { Metric dropwizardMetric = dropwizardMetrics.get(metricName); - solrPrometheusCoreExporter.exportDropwizardMetric(dropwizardMetric, metricName); + exporter.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); } }); - consumer.accept(solrPrometheusCoreExporter); + consumer.accept(exporter); + } + + public static SolrPrometheusExporter getExporterType(String registryName) { + String coreName; + boolean cloudMode = false; + String[] rawParsedRegistry = registryName.split("\\."); + List<String> parsedRegistry = new ArrayList<>(Arrays.asList(rawParsedRegistry)); Review Comment: why clone? ########## solr/solr-ref-guide/modules/deployment-guide/pages/monitoring-with-prometheus-and-grafana.adoc: ########## @@ -587,3 +594,27 @@ This screenshot shows what it might look like: .Grafana Dashboard image::monitoring-with-prometheus-and-grafana/grafana-solr-dashboard.png[image,width=800] + +== Prometheus Metrics API Review Comment: Not sure we want to name it this way... perhaps "Metrics API with Prometheus format" ########## solr/core/src/java/org/apache/solr/metrics/prometheus/node/SolrNodeHandlerMetric.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.prometheus.node; + +import com.codahale.metrics.Counter; +import com.codahale.metrics.Gauge; +import com.codahale.metrics.Meter; +import com.codahale.metrics.Metric; +import org.apache.solr.metrics.prometheus.SolrMetric; +import org.apache.solr.metrics.prometheus.SolrPrometheusExporter; + +/* Dropwizard metrics of name ADMIN.* and UPDATE.* */ +public class SolrNodeHandlerMetric extends SolrNodeMetric { + public static final String NODE_REQUESTS = "solr_metrics_node_requests"; + public static final String NODE_SECONDS_TOTAL = "solr_metrics_node_requests_time"; + public static final String NODE_CONNECTIONS = "solr_metrics_node_connections"; Review Comment: Okay but FWIW *IMO* in situations like this class (parsing/exporting) constants like this only diffuse readability (add a needless indirection). Take it or leave it; your choice. ########## solr/core/src/java/org/apache/solr/response/PrometheusResponseWriter.java: ########## @@ -118,13 +108,41 @@ public static void toPrometheus( (metricName, metric) -> { try { Metric dropwizardMetric = dropwizardMetrics.get(metricName); - solrPrometheusCoreExporter.exportDropwizardMetric(dropwizardMetric, metricName); + exporter.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); } }); - consumer.accept(solrPrometheusCoreExporter); + consumer.accept(exporter); + } + + public static SolrPrometheusExporter getExporterType(String registryName) { + String coreName; + boolean cloudMode = false; + String[] rawParsedRegistry = registryName.split("\\."); + List<String> parsedRegistry = new ArrayList<>(Arrays.asList(rawParsedRegistry)); + + switch (parsedRegistry.get(1)) { + case ("core"): Review Comment: These parenthesis in your case statements are pointless; not sure I've ever seen that -- 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]
