SOLR-18455: actually disable metrics on <metrics enabled="false"/> by mkhludnev · Pull Request #4902 · apache/solr

mkhludnev · 2026-09-10T22:35:28Z

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

Description

Please provide a short description of the changes you're making with this pull request.

Solution

Please provide a short description of the approach taken to implement your solution.
We appreciate transparent disclosure of significant AI Coding assistants usage.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

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, not available for branches on forks living under an organisation)
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
I have added a changelog entry for my change

Co-authored-by: mkhludnev <807522+mkhludnev@users.noreply.github.com>

mkhludnev · 2026-09-10T22:36:57Z

+    assertU(adoc("id", "1000000", "field1_s", "foo"));
+    openNewSearcherAndWait();
+
+    assertEventuallyReleased(replacedSearcher);


too heavy test. Put it into Nightly or drop?

If a test take a while depending on the person environment, probably nightly makes sense if that is the case in this test.

here's an idea from @sigram https://issues.apache.org/jira/browse/SOLR-18442?focusedCommentId=18115182&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-18115182

mkhludnev · 2026-09-11T06:38:29Z

at least this patch let to disable metrics and it let me pass the benchmark for #4749

sigram · 2026-09-14T11:32:39Z

+  public void testReplacedSearcherCanBeGarbageCollected() throws Exception {
+    WeakReference<SolrIndexSearcher> replacedSearcher = registerCurrentSearcherForGcCheck();
+
+    assertU(adoc("id", "1000000", "field1_s", "foo"));


This doc needs to be deleted at the end of the test to avoid index pollution for other tests.

sigram · 2026-09-14T12:05:09Z

Running with metricsEnabled=false produces this exception:

2026-09-14 12:01:09.161 ERROR (qtp1465346452-42-localhost-12) [ t:localhost-12] o.a.s.s.HttpSolrCall 500 Exception
java.io.IOException: No metrics found in response
        at org.apache.solr.response.PrometheusResponseWriter.write(PrometheusResponseWriter.java:66)
        at org.apache.solr.servlet.HttpSolrCall.writeResponse(HttpSolrCall.java:814)
        at org.apache.solr.servlet.HttpSolrCall.logAndFlushAdminRequest(HttpSolrCall.java:730)
        at org.apache.solr.servlet.HttpSolrCall.handleAdminRequest(HttpSolrCall.java:712)
        at org.apache.solr.servlet.HttpSolrCall.call(HttpSolrCall.java:478)
        at org.apache.solr.servlet.SolrServlet.dispatch(SolrServlet.java:114)

Edit: this looks like a pre-existing bug, it happens also on main.

mlbiscoc · 2026-09-16T21:41:27Z

          }
        });
    // size of the currently opened commit
    solrMetricsContext.observableDoubleGauge(


Based on this PR is what caused the leak. Do we need the rest of this stuff then?

it's up you. #4902 is just a draft by claude, which unblocked my longevity test.

mlbiscoc · 2026-09-16T21:43:14Z

+    assertU(adoc("id", "1000000", "field1_s", "foo"));
+    openNewSearcherAndWait();
+
+    assertEventuallyReleased(replacedSearcher);


If a test take a while depending on the person environment, probably nightly makes sense if that is the case in this test.

mlbiscoc · 2026-09-16T21:49:12Z

+  private Meter meter(String registry) {
+    if (!enabled) {
+      return OpenTelemetry.noop().getMeterProvider().get(OTEL_SCOPE_NAME);
+    }
+    return meterProvider(registry).get(OTEL_SCOPE_NAME);
+  }


I think this PR should be rescoped and titled to fixing this disabling of metrics.

extracted to https://issues.apache.org/jira/browse/SOLR-18455

I like the changes for this on disabling metrics. Can you change the title to SOLR-18455 and rip out the searcher changes? The other PR should be the actual fix for the leak.

mkhludnev · 2026-09-17T20:54:14Z

it seems to cause the fail

10426 INFO  (TEST-TestPullReplicaWithAuth.testPKIAuthWorksForPullReplication-seed#[E8AE82F6E427848D]) [n:127.0.0.1:42647_solr] o.a.s.SolrTestCaseJ4 ###Ending testPKIAuthWorksForPullReplication
   >     java.lang.NullPointerException: Cannot invoke "io.opentelemetry.exporter.prometheus.PrometheusMetricReader.collect()" because "reader" is null
   >         at __randomizedtesting.SeedInfo.seed([E8AE82F6E427848D:BD9471DA03F7D9BB]:0)
   >         at org.apache.solr.util.SolrMetricTestUtils.getDataPointSnapshot(SolrMetricTestUtils.java:85)
   >         at org.apache.solr.util.SolrMetricTestUtils.getDataPoint(SolrMetricTestUtils.java:141)
   >         at org.apache.solr.util.SolrMetricTestUtils.getDatapoint(SolrMetricTestUtils.java:136)
   >         at org.apache.solr.util.SolrMetricTestUtils.getCounterDatapoint(SolrMetricTestUtils.java:151)
   >         at org.apache.solr.cloud.TestPullReplicaWithAuth.testPKIAuthWorksForPullReplication(TestPullReplicaWithAuth.java:111)

changelog

dsmiley · 2026-09-19T19:20:01Z

+    previousMetricsEnabled = System.getProperty("metricsEnabled");
+    System.setProperty("metricsEnabled", "false");


why do this? Solr's test infrastructure ensures that each test suite (class) starts with the same set of system properties, thus making it impossible for one test suite to mess with system properties in a way affecting another test.

thanks @dsmiley

dsmiley · 2026-09-20T18:58:02Z

    return builder;
  }

+  private Meter meter(String registry) {


why introduce this... couldn't you modify the meterProvider method here to do this enabled check and respond similarly?

Right! Thanks @dsmiley

…4902) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mkhludnev <807522+mkhludnev@users.noreply.github.com> (cherry picked from commit 4ceaa32)

Copilot AI and others added 2 commits September 10, 2026 22:08

Add metrics leak regression coverage

b844abb

Co-authored-by: mkhludnev <807522+mkhludnev@users.noreply.github.com>

Disable metrics registries when metrics are off

89dcfd5

Co-authored-by: mkhludnev <807522+mkhludnev@users.noreply.github.com>

github-actions Bot added tests cat:search cat:metrics labels Sep 10, 2026

mkhludnev commented Sep 10, 2026

View reviewed changes

dsmiley requested a review from mlbiscoc September 11, 2026 01:14

dsmiley changed the title ~~SolrIndexSearcher is retained for the life of the node by OpenTelemetry observable gauges~~ SOLR-18422: SolrIndexSearcher OpenTelemetry observable gauges memory leak Sep 11, 2026

sigram reviewed Sep 14, 2026

View reviewed changes

janhoy mentioned this pull request Sep 16, 2026

SOLR-18442: Fix SolrIndexSearcher leak via unit-bearing observable metrics #4917

Open

mlbiscoc reviewed Sep 16, 2026

View reviewed changes

mkhludnev changed the title ~~SOLR-18422: SolrIndexSearcher OpenTelemetry observable gauges memory leak~~ SOLR-18455: actually disable metrics on <metrics enabled="false"/> Sep 17, 2026

mkhludnev added 2 commits September 17, 2026 23:01

rollback SolrIndexSearcher.java

a12af46

rollback SolrIndexSearcherTest.java

8d86a74

github-actions Bot removed the cat:search label Sep 17, 2026

mkhludnev added this to the 10.x milestone Sep 18, 2026

SOLR-18455: actually enable metrics for tests which read them

cd6d8ed

changelog

github-actions Bot added the cat:cloud label Sep 18, 2026

mkhludnev marked this pull request as ready for review September 18, 2026 17:49

mkhludnev requested review from mlbiscoc and sigram September 18, 2026 18:09

dsmiley reviewed Sep 19, 2026

View reviewed changes

SOLR-18455: rely on SystemPropertiesRestoreRule

96b8dad

mkhludnev requested a review from dsmiley September 20, 2026 11:01

dsmiley reviewed Sep 20, 2026

View reviewed changes

simplify code change

e56b363

dsmiley approved these changes Sep 20, 2026

View reviewed changes

mkhludnev merged commit 4ceaa32 into apache:main Sep 21, 2026
5 of 7 checks passed

		previousMetricsEnabled = System.getProperty("metricsEnabled");
		System.setProperty("metricsEnabled", "false");

Conversation

mkhludnev commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Tests

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhludnev Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhludnev commented Sep 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigram commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mkhludnev Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mkhludnev commented Sep 17, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mkhludnev commented Sep 10, 2026 •

edited

Loading

mkhludnev Sep 17, 2026 •

edited

Loading

sigram commented Sep 14, 2026 •

edited

Loading

mkhludnev Sep 17, 2026 •

edited

Loading