Conversation
…trics SolrMetricsContext only registered observable gauges/counters for closing in the overloads without an OtelUnit. SolrIndexSearcher's "index.commit_size" gauge passes OtelUnit.MEGABYTES, so its callback (capturing the DirectoryReader) survived SolrMetricsContext.close() and pinned every searcher ever opened in the OTel meter. Same gap affected UpdateLog, SuggestComponent and ZkContainer metrics registered with a unit. Registration of the closeable now happens in the unit-bearing overloads, which the no-unit overloads delegate to.
| import org.junit.Before; | ||
| import org.junit.Test; | ||
|
|
||
| public class SolrMetricsContextTest extends SolrTestCaseJ4 { |
There was a problem hiding this comment.
can't we use SolrTestCase ? Fable didn't read our AGENTS.md :-(
| var observableLongGauge = | ||
| metricManager.observableLongGauge(registryName, metricName, description, callback, unit); | ||
| closeables.add(observableLongGauge); | ||
| return observableLongGauge; |
There was a problem hiding this comment.
Ah oh wow this overload made me tripped me up when writing this and I added the closables in the wrong place! :(
Thank you @janhoy and @mkhludnev for fixing and finding this.
There was a problem hiding this comment.
I think we should better protect devs from this in the future or if new observable instruments get created and the author forgets to do closeables.add or places it in the wrong place (like I did).
What if these observable instruments here in SolrMetricsContext just return void instead so the users of these instruments don't need to remember to close them or figure out what to do with the object. Each of the observable instrument goes through a registerObservable which takes care of adding to closeable for the caller. Then make the observable instruments from metricManager package-private so only SolrMetricsContext can use it. Wdyt?
This is a bigger scoped changed but I think it is worth it.
There was a problem hiding this comment.
@mlbiscoc +1, Solr 9 used a similar mechanism to solve a similar problem - Dropwizard gauges were constructed with lambdas that would keep references to closed searchers, thus causing similar leaks. The solution we came up with was to hide this mechanism in SolrMetricManager - always register gauges with a helper method in SolrMetricManager that would wrap the gauge instance + a unique tag and add these lambdas to a list of closables to be called on SolrMetricsContext.unregister().
There was a problem hiding this comment.
As for the timing wrt 10.1 release... This minimal fix here addresses the immediate problem, perhaps refactoring could be scheduled for later?
There was a problem hiding this comment.
Understandable. Approved then because it does address the actual problem.
There was a problem hiding this comment.
|
Guys, I don't have time to run with this, so I'm happy to let you commit to this PR branch or just fork it into your own, add yourself to changelog and see it to completion. Also I have not checked whether Solr 9 has a similar issue. |
| invocations.set(0); | ||
| reader.collect(); | ||
| assertEquals("no callback should fire after the context is closed", 0, invocations.get()); | ||
| } |
There was a problem hiding this comment.
I wonder if we could somehow add a test to actually detect leaks... I imagine something like using the DiagnosticCommand MXBean with GC.class_histogram command to do a full GC and list the class/instance histogram, then do several known-to-be-tricky operations like add/commit/commitWithin, core reload, schema change etc, repeat this 100 times or so, and then do the histogram again (which again performs a full GC) to see if differences are "reasonable".
4 participants
https://issues.apache.org/jira/browse/SOLR-18442
This research and PR is 100% the work of Claude Code Fable 5.1
Root cause
The
SolrMetricsContext.observable{Long,Double}{Gauge,Counter}overloads that take anOtelUnitnever added the instrument to the context'scloseables, soSolrMetricsContext.close()could not unregister them.SolrIndexSearcherregisters itsindex.commit_sizegauge withOtelUnit.MEGABYTES, so exactly oneCallbackRegistrationper searcher survivedclose(), pinning the searcher, itsDirectoryReaderand every segment's live-docs bitset in the OTel meter. This matches the one-registration-per-searcher growth in the JIRA heap histograms.UpdateLog,SuggestComponentandZkContainerregister metrics with a unit through the same path.Fix
The unit-bearing overloads now register the closeable; the no-unit overloads delegate to them. No changes to callers.
Test
SolrMetricsContextTestregisters all eight overload variants, closes the context and asserts no callback fires on the next collect. Fails without the fix (the four unit-bearing callbacks keep firing), passes with it, no GC dependence.Compared with #4902: this fixes the missing unregistration instead of weakening the reader reference, and also covers the other leaking callers. The
<metrics enabled="false"/>question from the JIRA is left out of this PR.