SOLR-10654: Introduce output of Prometheus metrics directly from Solr #2405

mlbiscoc · 2024-04-17T15:39:26Z

https://issues.apache.org/jira/browse/SOLR-10654

Description

Introduce a new Prometheus response writer /solr/admin/metrics?wt=prometheus to output Dropwizards Solr Core registry metrics directly from Solr. Output looks similar to the prometheus exporter. This exports solr.core.* solr.node solr.jvm solr.jetty metric registries

Solution

Currently, Solr metrics are collected by Dropwizard and differentiated by metric names. This is an anti-pattern to Prometheus. Manually export Dropwizard Solr core registry metrics to a new SolrPrometheusExporter base class to parse out the labels from the metric name and output in a Prometheus readable format for the Prometheus Response writer. The exporter is just temporarily created to export a snapshot of what the current Dropwizard registry is at. These are not held in memory.

The exporter will categorize these metrics and export to prometheus format with labels depending on the category and metric names.

In standalone, all labels for the solr.core registry includescore. In cloud mode, all metrics include core, collection, shard, replica

NOTE: Not all metrics are possible to convert to Prometheus such as Dropwizard string metrics. This is because Prometheus does not support non-numeric types

Tests

Added MetricsHandlerTest, SolrPrometheusExporterTest, TestPrometheusReponseWriter

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

richardmlu · 2024-04-17T17:39:42Z

solr/core/src/java/org/apache/solr/metrics/prometheus/core/SolrCoreSearcherMetric.java

+  public SolrCoreMetric parseLabels() {
+    String[] parsedMetric = metricName.split("\\.");
+    if (dropwizardMetric instanceof Gauge) {
+      String type = parsedMetric[2];


Is it possible for metricName to be size 1 at this point? If so, we should surround this in a try/throw or a check.

I have not seen a metric with delimited with a size of 1 yet but in cases that does, I did surround with try/catch at a higher level here

dsmiley

There are enough additional dependencies that I feel strongly this should be another module.

It's a shame to see so much code to do the mapping but... shrug, ah well.

solr/core/src/java/org/apache/solr/metrics/prometheus/core/SolrCoreMetric.java

solr/core/src/java/org/apache/solr/metrics/prometheus/core/package-info.java

solr/core/src/java/org/apache/solr/metrics/prometheus/SolrPrometheusRegistry.java

mlbiscoc · 2024-05-02T20:15:06Z

Moving this out of draft for review

dsmiley · 2024-05-07T13:45:49Z

There are enough additional dependencies that I feel strongly this should be another module.

WDYT?

mlbiscoc · 2024-05-07T14:15:52Z

There are enough additional dependencies that I feel strongly this should be another module.

WDYT?

Sorry, I somehow completely missed this comment. When you say be another module, do you mean move it to solr/modules that needs to be enabled? If so, I think that prometheus metrics is valuable enough to be part of core and enabled by default but I also only added 2 additional dependencies. Is that enough to be its own module?

dsmiley · 2024-05-07T14:19:42Z

io.prometheus:prometheus-metrics-config:1.1.0 (1 constraints: 3e164ed8)
io.prometheus:prometheus-metrics-exposition-formats:1.1.0 (1 constraints: 0405f335)
io.prometheus:prometheus-metrics-model:1.1.0 (2 constraints: 411b133b)
io.prometheus:prometheus-metrics-shaded-protobuf:1.1.0 (1 constraints:

No, there are 4 dependencies. Only the last one concerns me. Why is protobuf needed at all? Can it be excluded? Can any of the others be excluded as well, while you're at it?

dsmiley · 2024-05-31T15:43:17Z

It doesn't feel right to merge this PR right now. Needs docs. Also the choice of what metrics are done so far is inverted from the value that would make most sense for this whole approach -- OS/JVM/node issues are highest value.

mlbiscoc · 2024-06-10T18:29:30Z

It doesn't feel right to merge this PR right now. Needs docs. Also the choice of what metrics are done so far is inverted from the value that would make most sense for this whole approach -- OS/JVM/node issues are highest value.

@dsmiley Just finished up exporting solr.node solr.jvm and solr.jetty metrics to prometheus and is ready for another round of review whenever you are. I updated the description above and attached files if you want to see what the prometheus output looks like. Last thing I did not finish writing up is docs. I'm assuming the 2 places to update would be the metrics-api section and possibly Monitoring with Prometheus and Grafana?

mlbiscoc · 2024-06-11T18:54:25Z

Added ref-guide documentation on Prometheus endpoint

dsmiley

I'd like to suggest an integration test showing all the output (say to a file) stripped of numbers (to remove brittle/changing stuff) to show that we export what we mean to export. I could imagine someone renaming some metric and then forgetting to update the code here.

solr/core/src/java/org/apache/solr/metrics/prometheus/jvm/SolrJvmBuffersMetric.java

solr/core/src/java/org/apache/solr/metrics/prometheus/node/SolrNodeContainerMetric.java

dsmiley · 2024-06-15T11:42:19Z

solr/core/src/java/org/apache/solr/metrics/prometheus/node/SolrNodeHandlerMetric.java

+  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";


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.

I personally prefer constants so I'd leave it unless someone feels strongly about it.

dsmiley · 2024-06-15T11:44:03Z

solr/core/src/java/org/apache/solr/metrics/prometheus/node/SolrPrometheusNodeExporter.java

+    }
+
+    SolrMetric solrNodeMetric = categorizeMetric(dropwizardMetric, metricName);
+    solrNodeMetric.parseLabels().toPrometheus(this);


if we forget to call parseLabels, will we recognize a mistake? not a big deal but just a thought

I wouldn't say its a mistake. I split these into 2 abstract functions to split out the logic for getting the labels, then converting the correct dropwizard type to the correct prometheus type instead of 1 big function.

I think it's fine if not called. If down the line additional metrics want to be exported from dropwizard, it's possible to just have it exported without any labels and just it's value to which parselabels isn't needed.

solr/core/src/java/org/apache/solr/metrics/prometheus/node/SolrPrometheusNodeExporter.java

solr/core/src/java/org/apache/solr/response/PrometheusResponseWriter.java

solr/solr-ref-guide/modules/deployment-guide/pages/monitoring-with-prometheus-and-grafana.adoc

mlbiscoc · 2024-06-18T15:18:22Z

I'd like to suggest an integration test showing all the output (say to a file) stripped of numbers (to remove brittle/changing stuff) to show that we export what we mean to export. I could imagine someone renaming some metric and then forgetting to update the code here.

Thanks for reviewing again. Added in an integration test with a file stripped of values. Also the unit tests I wrote originally should catch if any changes that happen to the naming of the prometheus metric or labels.

Also thanks for catching missing javadocs. Made sure to add them in for each class and added sample values for what is being exported to prometheus.

solr/solr-ref-guide/modules/deployment-guide/pages/monitoring-with-prometheus-and-grafana.adoc

…with-prometheus-and-grafana.adoc Co-authored-by: Christine Poerschke <[email protected]>

…us-solrcore

dsmiley

I tried this PR locally using the "techproducts" example. I thing I noticed is that the metrics filtering appears to not work, which is a big problem. A test should be added for this, and it should work.

http://localhost:8983/solr/admin/metrics?category=solr.jetty&wt=prometheus

dsmiley · 2024-06-28T11:22:18Z

solr/core/src/java/org/apache/solr/metrics/prometheus/core/SolrCoreHandlerMetric.java

+    return this;
+  }
+
+  /*


(this feedback applies to most classes where you did this).
You've put this where javadocs go yet it's it's technically not javadocs since you didn't start with a leading double-asterisk. If you were to do that, you might need to add markup :-/ I suppose leaving this as-is is fine. Even moving it above e parseLabels and add a blank line to further show this is not (trying to be) javadoc. FWIW when I suggested adding input examples for parsing, I thought you would add some trivial "//" in-line at the point of parsing. But this is fine, as you wish.

Sorry for this. Went with your other suggestion of putting it above parseLabels and add an empty line below it.

If you still prefer just normal inline comments, I can make the appropriate changes.

dsmiley · 2024-06-28T11:37:39Z

solr/core/src/java/org/apache/solr/metrics/prometheus/node/SolrNodeHandlerMetric.java

+   */
+  @Override
+  public void toPrometheus(SolrPrometheusExporter exporter) {
+    if (dropwizardMetric instanceof Meter) {


As I re-read all this code in the PR, esp. for these toPrometheus methods, the instanceof checks seem more awkward with questionable value. If instead we see that the name matches a pattern, let's just cast. If we get a ClassCastException, a test will catch it.

I went ahead and removed most of the instanceof checks but kept in some places. In the places I kept, it seemed cleaner to export that way as not all my checks are just straight names checks and we don't export every metric. So we can just export a large amount by just checking its type. I can try refactoring further of removing instanceof checks if you still disagree.

dsmiley · 2024-06-28T11:48:03Z

solr/core/src/test-files/solr/solr-prometheus-output.txt

This doesn't go in the "solr" folder; that's a "solr home dir" and thus only solr config files go there.

Created it's own prometheus directory to place the file in a place we can place future test files relating to prometheus

solr/core/src/test/org/apache/solr/response/TestPrometheusResponseWriter.java

dsmiley · 2024-06-28T13:41:25Z

solr/core/src/test/org/apache/solr/response/TestPrometheusResponseWriter.java

+          Files.readString(
+              new File(TEST_PATH().toString(), "solr-prometheus-output.txt").toPath(),
+              StandardCharsets.UTF_8);
+      assertEquals(expectedOutput, actual.replaceAll("(?<=}).*", ""));


We can expect there will be more cleansing/normalization to do. For example the JVM used will result in different "item" labels on solr_metrics_jvm_gc. I got "G1-Old-Generation" vs "PS-MarkSweep". People will use different JVMs for tests.

Parsed out the JVM item tag from the test with additional regex

dsmiley · 2024-06-28T13:42:30Z

solr/core/src/test/org/apache/solr/response/TestPrometheusResponseWriter.java

+          Files.readString(
+              new File(TEST_PATH().toString(), "solr-prometheus-output.txt").toPath(),
+              StandardCharsets.UTF_8);
+      assertEquals(expectedOutput, actual.replaceAll("(?<=}).*", ""));


could you add a comment explaining that regexp please? It's all symbols 😅

Added in a comment with explanation.

mlbiscoc · 2024-06-28T20:05:09Z

I tried this PR locally using the "techproducts" example. I thing I noticed is that the metrics filtering appears to not work, which is a big problem. A test should be added for this, and it should work.

http://localhost:8983/solr/admin/metrics?category=solr.jetty&wt=prometheus

Filtering does work I think the parameter you are looking for is group. Try localhost:8983/solr/admin/metrics?group=jvm&wt=prometheus and you can change it with jetty node core for the corresponding registries

But still you are correct, I should have a test for this. Will add one in.

The only other thing I remember now about this is that regex, key, expr parameters for filtering will not work with this prometheus wt. I did that on purpose because those 3 parameters are for filtering based on the name of the metric while the others are kind of more generic such as registry or type filtering. I didn't think it made sense because prometheus naming is different from the dropwizard naming after exporting. For example: solr\.core\..*:QUERY\..*\.requestTimes:max_ms is not an output of prometheus but to allow expr filtering around that felt odd.

Even then if you need extensive filtering or trying to get some specific metric, maybe the Prometheus Exporter process with a custom config might be the better solution.

dsmiley · 2024-06-28T22:23:58Z

Oops; maybe I was hurried. I'm glad the category works at least! The other filtering options could be deferred to a future JIRA by matching the prometheus name. It's a very useful capability; some metrics implemented as gauges are expensive to compute, and others are just excessive output that aren't needed for O(N) for N cores if N is for some users quite high.

mlbiscoc · 2024-07-01T20:36:11Z

Oops; maybe I was hurried. I'm glad the category works at least! The other filtering options could be deferred to a future JIRA by matching the prometheus name. It's a very useful capability; some metrics implemented as gauges are expensive to compute, and others are just excessive output that aren't needed for O(N) for N cores if N is for some users quite high.

Added in a test for the filtering. I'd be happy expand on this and add further filtering feature specific to prometheus after this is ready to be merged.

dsmiley

This is super close now.
Can you propose a brief CHANGES.txt under 9.7 "New Features" (I think it's closer to this than an improvement).

dsmiley · 2024-07-01T21:18:34Z

solr/core/src/test/org/apache/solr/handler/admin/MetricsHandlerTest.java

+
+    actualSnapshot = getMetricSnapshot(actualSnapshots, "solr_metrics_jvm_gc_seconds");
+    actualGaugeDataPoint =
+        getGaugeDatapointSnapshot(actualSnapshot, Labels.of("item", "G1-Old-Generation"));


This assertion will fail for varying JVMs; skip this one. Some others too above... it's okay to make the test looser / less specific.

Removed a few more tests from the JVM registry

dsmiley · 2024-07-01T21:26:04Z

solr/solr-ref-guide/modules/deployment-guide/pages/metrics-reporting.adoc

@@ -879,7 +879,7 @@ The response will look like this:
  ]}
 ----

-Like other request handlers, the Metrics API can also take the `wt` parameter to define the output format.
+Like other request handlers, the Metrics API can also take the `wt` parameter to define the output format. The Metrics API also provides an additional special `wt` parameter `prometheus` that will output metrics in Prometheus format. This can be used for xref:monitoring-with-prometheus-and-grafana.adoc#metrics-api-with-prometheus-format[Monitoring with Prometheus and Grafana]


After each sentence, put a newline (doesn't change the final render but it to keep editing & diffs more manageable).

Should document what options do not work with prometheus. The regex one doesn't, you said.

Added in a small note about the filter query parameters. Let me know if you prefer a different way to write it on the ref-guide

mlbiscoc · 2024-07-02T01:23:31Z

This is super close now. Can you propose a brief CHANGES.txt under 9.7 "New Features" (I think it's closer to this than an improvement).

Added entry into CHANGES.txt

An alternative to the "Prometheus Exporter" in which each Solr node can return the Prometheus format natively from the MetricsHandler using `wt=prometheus` param. It's much faster and architecturally simpler, albeit less flexible. Co-authored-by: mbiscocho <[email protected]> Co-authored-by: Christine Poerschke <[email protected]> Co-authored-by: David Smiley <[email protected]> (cherry picked from commit fd7d447)

mbiscocho added 4 commits April 2, 2024 13:31

Init commit for dropwizard to prometheus

3c3095a

Expose Prometheus Metrics directly from Solr from Core registry

b38b55b

Incorrect check on counter instead of gauge

8eb18f7

Keep continuity in parameter orderings

f531e49

github-actions bot added dependencies Dependency upgrades tool:build cat:search cat:metrics labels Apr 17, 2024

mlbiscoc mentioned this pull request Apr 17, 2024

[Experimental] Expose Prometheus metrics directly from Solr with prometheus client #2375

Closed

gradle tidy

d594f50

richardmlu reviewed Apr 17, 2024

View reviewed changes

richardmlu approved these changes Apr 17, 2024

View reviewed changes

dsmiley reviewed Apr 18, 2024

View reviewed changes

mbiscocho added 4 commits April 29, 2024 15:46

Write javadocs for new classes

59cc9b4

Refactor out prometheus registry to Prometheus metric snapshots

bc6a60a

Update dependencies

a57ffb4

Add MetricsHandler prometheus test

58fb18d

github-actions bot added the tests label May 1, 2024

mbiscocho added 4 commits May 2, 2024 11:42

Add new methods of creating and collecting datapoints

680a0a3

Add tests for SolrPrometheusExporter

e4c5053

Add test for Prometheus Response Writer

a802716

Remove fobidden api usages

a57e107

mlbiscoc marked this pull request as ready for review May 2, 2024 20:11

mbiscocho added 2 commits May 3, 2024 11:35

Check against different metric for test

c40dda3

Broken precommit

0aa1b55

Remove transitive dependencies

88e6347

Export jetty/jvm/node registry metrics

acde988

mlbiscoc changed the title ~~SOLR-10654: Introduce output of Prometheus metrics for Solr Core registry~~ SOLR-10654: Introduce output of Prometheus metrics directly from Solr Jun 10, 2024

mbiscocho added 2 commits June 11, 2024 13:32

Update broken tests

060bb59

Update ref-guide with Prometheus metrics endpoint

7359d7c

github-actions bot added the documentation Improvements or additions to documentation label Jun 11, 2024

Change test assertion for metrics affected by testing environment

835c946

dsmiley reviewed Jun 15, 2024

View reviewed changes

Add additional java docs and integration test

6cce972

cpoerschke reviewed Jun 21, 2024

View reviewed changes

solr/solr-ref-guide/modules/deployment-guide/pages/monitoring-with-prometheus-and-grafana.adoc Outdated Show resolved Hide resolved

mbiscocho and others added 4 commits June 21, 2024 14:44

Change integration test to use SolrJettyTestRule

bceb4fe

Update solr/solr-ref-guide/modules/deployment-guide/pages/monitoring-…

f1b410a

…with-prometheus-and-grafana.adoc Co-authored-by: Christine Poerschke <[email protected]>

Manually register metrics not initializing from JettyTestRule

a9ee37f

Merge branch 'refs/heads/main' into fork/mlbiscoc/SOLR-10654-promethe…

1949b5b

…us-solrcore

dsmiley reviewed Jun 28, 2024

View reviewed changes

Changes from review for refactor and tests

1d7b65a

dsmiley reviewed Jul 1, 2024

View reviewed changes

mlbiscoc added 3 commits July 1, 2024 21:19

Add additional documentation to ref-guide

d6786ec

Remove some JVM tests

d6bd407

Add entry into CHANGES.txt

9f1abbe

Add names to CHANGES.txt

5da678c

dsmiley merged commit fd7d447 into apache:main Jul 9, 2024
3 checks passed

SOLR-10654: Introduce output of Prometheus metrics directly from Solr #2405

SOLR-10654: Introduce output of Prometheus metrics directly from Solr #2405

Conversation

mlbiscoc commented Apr 17, 2024 • edited Loading

Description

Solution

Tests

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley left a comment

Choose a reason for hiding this comment

mlbiscoc commented May 2, 2024

dsmiley commented May 7, 2024

mlbiscoc commented May 7, 2024

dsmiley commented May 7, 2024

dsmiley commented May 31, 2024

mlbiscoc commented Jun 10, 2024

mlbiscoc commented Jun 11, 2024

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbiscoc commented Jun 18, 2024

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbiscoc commented Jun 28, 2024

dsmiley commented Jun 28, 2024

mlbiscoc commented Jul 1, 2024

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbiscoc commented Jul 2, 2024

mlbiscoc commented Apr 17, 2024 •

edited

Loading