SOLR-17855: Cleanup Dropwizard metric code and tests #3745

mlbiscoc · 2025-10-09T00:09:26Z

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

This branch changes a number of things:

Remove unused Dropwizard metric dependencies
Remove SolrReporters
Remove scope String parameter from initializeMetrics
Cleanup a number of NOCOMMITs and broken tests
Collect observables to the corresponding SolrMetricContext to close on unregister() like how Dropwizard callback gauges were closed

mlbiscoc

@dsmiley Big change here to remove a lot of dropwizard stuff. What I am mostly not sure about here is if the things I removed should have been removed or not so to have that double checked would be good. All tests were passing locally at this point. Sadly I found a few more things that were broken and need to be migrated or fixed:

SystemInfoHandler
Admin UI Node view? I think it got host names or metrics on the UI for that
Node aggregation metrics (PR is open already)

Once those things are done with this PR, the branch migration should be mostly done besides ref-guide re-writes.

mlbiscoc · 2025-10-09T00:10:06Z

solr/core/src/java/org/apache/solr/metrics/SolrMetricProducer.java

+   * @param parentContext The registry that the component will initialize metrics to
+   * @param attributes Base set of attributes that will be bound to all metrics for that component
   */
-  void initializeMetrics(SolrMetricsContext parentContext, Attributes attributes, String scope);


scope String gone

mlbiscoc · 2025-10-09T00:21:16Z

solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java

+      // NOCOMMIT: SOLR-17865
+      //      this.solrMetricsContext =
+      //          new SolrDelegateRegistryMetricsContext(
+      //              parentContext.getMetricManager(),
+      //              parentContext.getRegistryName(),
+      //              SolrMetricProducer.getUniqueMetricTag(this, parentContext.getTag()),
+      //              SolrMetricManager.getRegistryName(SolrInfoBean.Group.node));


Need to fix this as part of this PR. Might be more duplicate code which might end up looking very similar to requestHandlerBase

mlbiscoc · 2025-10-09T00:23:06Z

solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpListenerFactory.java

+    this.requestTimer =
+        new AttributedLongTimer(
+            solrMetricsContext.longHistogram(
+                "http_client_request_duration",
+                "HTTP client request duration",
+                OtelUnit.MILLISECONDS),
+            attributes);


I just found this metric wasn't migrated. Need to prefix the metric with solr_ and what should the name of this be?

mlbiscoc · 2025-10-09T00:24:08Z

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

+  // NOCOMMIT: The nodes view might be broken because of this
  @Test
+  @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-8207")
  public void proxyMetricsHandlerAllNodes() throws IOException, SolrServerException {


The admin UI might be broken based on this test. I need to come back to it

So I was looking at this and the UI and it's not broken? I am not good at Solr JS admin ui but I don't think it was using /admin/metrics for this but instead /admin/info/system handler. Turns out I also fixed that node view when I committed 7daafff going back and forth between the commits basically killing two birds with one stone. I am thinking of removing this test if no objections unless I am missing something here I don't know about the UI.

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

dsmiley

damn that was a lot of work!

dsmiley · 2025-10-09T02:50:31Z

gradle/libs.versions.toml

 cybozulabs-langdetect = { module = "com.cybozu.labs:langdetect", version.ref = "cybozulabs-langdetect" }
 decompose-decompose = { module = "com.arkivanov.decompose:decompose", version.ref = "decompose" }
 decompose-extensions-compose = { module = "com.arkivanov.decompose:extensions-compose", version.ref = "decompose" }
 dropwizard-metrics-core = { module = "io.dropwizard.metrics:metrics-core", version.ref = "dropwizard-metrics" }


I still see dropwizard dependencies

Forgot to mention this. I didn't remove it completely because a few things were using the instruments but not necessarily the registry for the metrics collection beside CrossDcConsumer (I don't know much about this module).

Stats was being used for overseer and Zk distributed queue and this was also being used by embedded zk metrics. It worked with the OverseerStatusCmd API to just get some averages and rates from the instruments.

MiniClusterState was using the meter instrument for logging somethings

CrossDcConsumer was creating its own registry it looks like for collecting metrics

Okay; looks like another JIRA I guess. At least doesn't seem like a release blocker.

dsmiley · 2025-10-09T02:56:20Z

solr/server/etc/jetty.xml

  <!-- Consult the javadoc of o.e.j.util.thread.QueuedThreadPool   -->
  <!-- for all configuration that may be set here.                 -->
  <!-- =========================================================== -->
-  <Arg name="threadPool">


hm; I wonder if Solr starts -- i.e. do any BATS integration tests work

dsmiley · 2025-10-09T03:01:03Z

solr/core/src/java/org/apache/solr/core/HttpSolrClientProvider.java


  private void initializeMetrics(SolrMetricsContext parentContext) {
    var solrMetricsContext = parentContext.getChildContext(this);
    String expandedScope =


now not needed

solr/core/src/java/org/apache/solr/handler/admin/SystemInfoHandler.java

dsmiley · 2025-10-09T03:12:22Z

solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java

      Boolean.getBoolean("solr.disableRequestId");

  private HandlerMetrics metricsShard = HandlerMetrics.NO_OP;
-  private final Map<String, Counter> shardPurposes = new ConcurrentHashMap<>();


huh.... I know of this thing and it's surprising that we don't actually consume from it. Well good riddance then.

solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpListenerFactory.java

dsmiley · 2025-10-09T04:25:12Z

solr/core/src/test/org/apache/solr/cloud/CloudExitableDirectoryReaderTest.java

            TimeUnit.SECONDS,
            (n, c) -> DocCollection.isFullyActive(n, c, 2, 1));

    fiveHundredsByNode = new LinkedHashMap<>();


assertNo500s uses that

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

mlbiscoc · 2025-10-09T20:57:20Z

solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java

-    response = handleDropwizardRegistry(params);
-
-    consumer.accept("metrics", response);
+    handlePrometheusRequest(params, consumer);


One thing I couldn't figure out here is how do we default the response to the PrometheusResponseWriter without wt? Other option I was thinking is we require the wt parameter with either prometheus or openmetrics and throw an exception otherwise. This would keep it verbose so you know if which format you are reading.

It's okay to default to one or the other.
Note eventually we should consider the Accepts header but I wouldn't worry about that now.

Will probably default to prometheus in that case then. My question though was actually how do you implement this to go to the PrometheusResponseWriter without wt parameter since it seems to always default to the JacksonJsonWriter otherwise?

Ugh. Looks like you need to set the params on the request to set WT. Try SolrParams.wrapDefaults(orig,SolrParams.of("wt", "prometheus"))

dsmiley · 2025-10-10T20:03:15Z

solr/core/src/java/org/apache/solr/util/stats/MetricUtils.java

-        consumer.accept(name, (MapWriter) ew -> ew.putNoEx("count", counter.getCount()));
-      }
-    }
+  public static void addMetrics(NamedList<Object> lst, Timer timer) {


You moved this from its former position. Can you put it back unless there's a reason for the move?

dsmiley · 2025-10-10T20:06:52Z

solr/core/src/java/org/apache/solr/util/stats/MetricUtils.java

   * @param consumer consumer for created names and metrics
   * @param <T> formal type
   */
  public static <T extends PlatformManagedObject> void addMXBeanMetrics(


The javadocs of this method are no longer true. Maybe even the name needs adjustment... perhaps foreachGetterValue?
It's not even metrics related at all; maybe it should move to its only caller in SystemInfoHandler

dsmiley · 2025-10-10T20:07:54Z

solr/core/src/java/org/apache/solr/util/stats/MetricUtils.java

   * @param consumer consumer for created names and metrics
   * @param <T> formal type
   */
  public static <T extends PlatformManagedObject> void addMXBeanMetrics(


And likewise the name and javadocs are no longer accurate

dsmiley · 2025-10-10T20:14:27Z

gradle/libs.versions.toml

 cybozulabs-langdetect = { module = "com.cybozu.labs:langdetect", version.ref = "cybozulabs-langdetect" }
 decompose-decompose = { module = "com.arkivanov.decompose:decompose", version.ref = "decompose" }
 decompose-extensions-compose = { module = "com.arkivanov.decompose:extensions-compose", version.ref = "decompose" }
 dropwizard-metrics-core = { module = "io.dropwizard.metrics:metrics-core", version.ref = "dropwizard-metrics" }


Okay; looks like another JIRA I guess. At least doesn't seem like a release blocker.

mlbiscoc · 2025-10-13T19:45:19Z

solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java

+    String format = params.get(CommonParams.WT);
+
+    if (format == null) {
+      req.setParams(SolrParams.wrapDefaults(params, SolrParams.of("wt", "prometheus")));


Now defaulting to prometheus

mlbiscoc · 2025-10-13T19:48:00Z

solr/core/src/java/org/apache/solr/metrics/reporters/solr/SolrReporter.java

- * sends them periodically as update requests to a selected Solr collection and to a configured
- * handler.
- */
-public class SolrReporter extends ScheduledReporter {


@dsmiley Just to respond to the JIRA, here is where I removed the reporter.

dsmiley

Nice. Will merge tonight.

mlbiscoc · 2025-10-13T20:49:18Z

Actually just realized I broke a few tests because some are trying to hit /admin/metrics endpoint but throw my new exception because it requested in javabin. I didn't look too deep but if you want to merge it I can fix it in a separate PR when I get back.

mlbiscoc · 2025-10-13T21:12:27Z

Gah I see. All the splitshard API tests is what started failing because of this. Was techinically always failing, but if metrics weren't reached, it just proceeded without fail and logged a warning. Now we throw an exception which does fail it. Other test is this which isn't too bad. Just add wt=prometheus. Once those are fixed we should be good 🤞.

… fork/mlbiscoc/SOLR-17855-dropwizard-to-otel # NODE_REGISTRY not getRegistryName # Conflicts: # solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java # solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java # solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java

Remove NOCOMMITs and dropwizard code

95a8f4b

mlbiscoc commented Oct 9, 2025

View reviewed changes

mlbiscoc requested a review from dsmiley October 9, 2025 00:31

dsmiley reviewed Oct 9, 2025

View reviewed changes

mlbiscoc commented Oct 9, 2025

View reviewed changes

Show prometheus metrics for jettyRunner

3c41eb3

mlbiscoc added 2 commits October 10, 2025 12:58

Move off dropwizard for SystemInfoHandler

7daafff

Bring back setupCluster for CloudExitableDirectoryReaderTest

4d6af72

dsmiley reviewed Oct 10, 2025

View reviewed changes

Changes from comments

9ae98af

mlbiscoc commented Oct 13, 2025

View reviewed changes

dsmiley approved these changes Oct 13, 2025

View reviewed changes

dsmiley added 3 commits October 13, 2025 23:45

Disable SplitShardCmd checkDiskSpace with nocommit

f2bbb3a

fix TestDistributedTracing wt=prometheus

c477a4c

dsmiley merged commit cee53df into apache:feature/SOLR-17458-rebased Oct 14, 2025
1 check passed

SOLR-17855: Cleanup Dropwizard metric code and tests #3745

SOLR-17855: Cleanup Dropwizard metric code and tests #3745

Uh oh!

Conversation

mlbiscoc commented Oct 9, 2025

Uh oh!

mlbiscoc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

mlbiscoc commented Oct 13, 2025

Uh oh!

mlbiscoc commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mlbiscoc commented Oct 13, 2025 •

edited

Loading