SOLR-17793: Create dedicated SdkMeterProviders for Solr core metrics #3402

mlbiscoc · 2025-06-23T17:11:27Z

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

OTEL metrics are immutable once created for a meter provider. This is problematic for creation and deletion of Solr Cores keeping stale metrics. To do this we do a few things:

Reuse Dropwizards concept of registries and tie it to a dedicated SdkMeterProvider for each Solr Core with a registered Prometheus Metric Reader.
With the use of dedicated SdkmeterProviders, we move away from using GlobalOpenTelemetry singleton instance for metrics
When deleting a core, close the corresponding SdkMeterProvider and its metric readers with it

mlbiscoc · 2025-06-23T17:14:32Z

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

-  private static void configureOpenTelemetrySdk(
-      SdkMeterProvider sdkMeterProvider, SdkTracerProvider sdkTracerProvider) {
+  private static void configureOpenTelemetrySdk() {
    if (loaded) return;

-    OpenTelemetrySdkBuilder builder = OpenTelemetrySdk.builder();
    if (TRACE_ID_GEN_ENABLED) {
      log.info("OpenTelemetry tracer enabled with simple propagation only.");
      ExecutorUtil.addThreadLocalProvider(new ContextThreadLocalProvider());
-      builder.setPropagators(ContextPropagators.create(SimplePropagator.getInstance()));
    }

-    if (sdkMeterProvider != null) builder.setMeterProvider(sdkMeterProvider);
-    if (sdkTracerProvider != null) builder.setTracerProvider(sdkTracerProvider);
-
-    GlobalOpenTelemetry.set(builder.build());


We stop using GlobalOpenTelemetry instance as you are fixed to 1 MeterProvider for the whole process and can't be modified. So reverted OTELs initialization of its SimplePropogator.

You say that but the code here continues to set GlobalOpenTelemetry 🤷

FWIW I think we should continue to use GOT but only for the container instance, if that makes sense.

I should have been more clear here. We are not using GlobalOpenTelemetry for metrics here anymore but traces still use it for the SimplePropogator now which is why I am still setting the GlobalOpenTelemetry.

Not sure what you mean using GlobalOpenTelemetry only for the container instance? If we continue with our manage dediated SDKMeterProviders and the GlobalOpenTelemetry for metrics things might get messy. Different metric readers and exporters managed by different things. Autoconfiguration would work for only the container instance but not our instrumented sdk providers.

Oh I see; nevermind my suggestion then..

mlbiscoc · 2025-06-23T17:15:04Z

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

@@ -115,6 +115,8 @@ public class SolrMetricManager {

  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());

+  public static final String OTEL_SCOPE_NAME = "org.apache.solr";


All metrics in Solr will have this as OTEL scope name

mlbiscoc · 2025-06-23T17:15:56Z

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

+  private final ConcurrentMap<String, MeterProviderAndReaders> meterProviderAndReaders =
+      new ConcurrentHashMap<>();


We wrap a Meter Provider and its corresponding Metric Readers together. Currently the only metric reader is Prometheus but additional metric reader could be a periodic reader to push with OTLP. This is essentially our concept of dropwizard's registry

mlbiscoc · 2025-06-23T17:19:47Z

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

+   * resulting in duplicate metric names across cores which is an illegal format if not under the
+   * same prometheus grouping.
+   */
+  private MetricSnapshots mergeSnapshots(List<MetricSnapshot> snapshots) {


This is a bit annoying that it had to be done. All Solr Cores have the same metric names with just different labels, but when using dedicated MeterProviders they are considered duplicated names running into this bug I fixed a while ago because they aren't under the same grouping. So we need to merge the duplicate metrics into a single prometheus grouping so we are compliant with Prometheus exposition format.

mlbiscoc · 2025-06-23T17:21:42Z

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

 import org.junit.BeforeClass;
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

-public class TestPrometheusResponseWriter extends SolrTestCaseJ4 {
+public class TestPrometheusResponseWriter extends SolrCloudTestCase {


I moved to SolrCloudTestCase since all of this was being tested in standalone mode. All of these should test in cloud mode

What does it matter? In general, tests should be lean, only testing at the highest level that's pertinent to the feature. Metrics don't have to do with SolrCloud.

Solr defaults to Cloud mode, so felt it make sense to move to cloud mode. Lets you also get specific labels from cloud. For example, all cloud and standalone core metrics have the label coreName but collection or shard is cloud specific. Lets us test for that with CloudTestCase but I get where you're coming from.

I can revert it back if you prefer. I was going to make a separate test suite like you recommended below anyways and move testCollectionDeletePrometheusOutput that I made.

Solr defaults to Cloud mode

That's not relevant. Would you then suggest every test in Solr run SolrCloud? It's been difficult to keep our test times down (we used to be at ~40min); I shudder to think if every test had to run SolrCloud.

all cloud and standalone core metrics have the label coreName but collection or shard is cloud specific

Okay then I don't have a strong preference. If you do wind up separating the higher level integration test, maybe it could have check at least one core metric has the collection & shard.

Thats understandable. I went with meeting in the middle and wrote a test to just check a single metric that the cloud labels exist and moved testCollectionDeletePrometheusOutput into that test suite. Reverted everything else back.

mlbiscoc · 2025-06-23T17:22:43Z

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

+  }
+
+  @Test
+  public void testCollectionDeletePrometheusOutput() throws Exception {


I don't know if this should be here as its a full high level test. Maybe a lower level test for this in MetricsHandlerTest or just a unit test in TestSolrMetricManager is suffice?

I suggest a separate test suite so that TestPrometheusResponseWriter doesn't need SolrCloud.

mlbiscoc · 2025-06-23T17:52:33Z

With dedicated SDK Meter Providers, this will not automatically work with the Java Agent as that relies on a linking with a GlobalOpenTelemetry which has a single SdkMeterProvider and so we lose auto-configuration that tracing gets and OTLP. So for OTLP push we need to manually set the SDK up. I have another change that I did not add to this PR to keep it in scope, but essentially to MetricManager we add a OtlpExporter and register a PeriodicMetricReader with the MeterProvider to push tjese metrics with OTLP.

dsmiley · 2025-06-25T18:14:40Z

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

@@ -849,6 +847,7 @@ private void loadInternal() {
        new SolrMetricsContext(
            metricManager, SolrMetricManager.getRegistryName(SolrInfoBean.Group.node), metricTag);

+    // NOCOMMIT: Do we need this for OTEL or is this specific for reporters?


It's not specific to otel; it's generic. At the moment, I see it's only used for async core loading, which feels needless but anyway another topic. I may put up a trivial PR.

dsmiley · 2025-06-25T18:31:46Z

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

-  private static void configureOpenTelemetrySdk(
-      SdkMeterProvider sdkMeterProvider, SdkTracerProvider sdkTracerProvider) {
+  private static void configureOpenTelemetrySdk() {
    if (loaded) return;

-    OpenTelemetrySdkBuilder builder = OpenTelemetrySdk.builder();
    if (TRACE_ID_GEN_ENABLED) {
      log.info("OpenTelemetry tracer enabled with simple propagation only.");
      ExecutorUtil.addThreadLocalProvider(new ContextThreadLocalProvider());
-      builder.setPropagators(ContextPropagators.create(SimplePropagator.getInstance()));
    }

-    if (sdkMeterProvider != null) builder.setMeterProvider(sdkMeterProvider);
-    if (sdkTracerProvider != null) builder.setTracerProvider(sdkTracerProvider);
-
-    GlobalOpenTelemetry.set(builder.build());


You say that but the code here continues to set GlobalOpenTelemetry 🤷

FWIW I think we should continue to use GOT but only for the container instance, if that makes sense.

dsmiley · 2025-06-25T18:45:14Z

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

+    if (!meterProviderAndReaders.containsKey(name)) return null;
+    return meterProviderAndReaders.get(name).prometheusMetricReader();


calling contains and then conditionally get is a minor anti-pattern since it's a double-lookup.

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

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

dsmiley · 2025-06-25T18:55:28Z

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

+                    ? base.unit(counterSnapshot.getMetadata().getUnit())
+                    : base;
+              });
+          counterSnapshot.getDataPoints().forEach(counterSnapshotMap.get(metricName)::dataPoint);


it's not clear what these lines actually accomplish. You loop stuff but appear to do do nothing with any results. disclaimer: I'm merely looking via GitHub UI.

Honestly maybe my java docs weren't clear enough. Essentially we need to return a single Snapshots object (Not to confuse with Snapshot object) that has all of our metrics. All these lines combine equivalent metrics names together. With multiple metric readers because of multiple meter providers, Prometheus structures the data like so between 2 cores from 2 Snapshot

Counter snapshot: core_1

metric name “requests_total”
Datapoints

{core: “core_1”, path: “/select”}, value = 10

Counter snapshot: core_2

metric name “requests_total”
Datapoints

{core: “core_2”, path: “/select”}, value = 20

We then need to merge these into a single Counter snapshot otherwise we output 2 Snapshots with duplicate metric name. So we take the builder for that specific metric name and merge them into a single snapshot.

Counter snapshot: merged

metric name “requests_total”
Datapoints

{core: “core_1”, path: “/select”}, value = 10

{core: “core_2”, path: “/select”}, value = 20

At the very end of this method, we build all the snapshots and create the final Snapshots object that the prometheus write can output without the duplicate TYPE and DESC information.

I tested this as well with an an internal OTLP exporting in solr to a OTEL collector and the collector seems to do some kind merging like this to avoid the duplicate information.

Thanks! What I specifically was confused about was the line of code that had an appearance of being side-effect free, thus pointless. But the method reference dataPoint adds a data point. It looks ambiguous to my fresh eyes on this API.

Having to write all this here is a bit of a PITA

dsmiley · 2025-06-25T18:58:33Z

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

 import org.junit.BeforeClass;
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

-public class TestPrometheusResponseWriter extends SolrTestCaseJ4 {
+public class TestPrometheusResponseWriter extends SolrCloudTestCase {


What does it matter? In general, tests should be lean, only testing at the highest level that's pertinent to the feature. Metrics don't have to do with SolrCloud.

dsmiley · 2025-06-25T19:00:19Z

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

+  }
+
+  @Test
+  public void testCollectionDeletePrometheusOutput() throws Exception {


I suggest a separate test suite so that TestPrometheusResponseWriter doesn't need SolrCloud.

dsmiley · 2025-06-27T03:22:57Z

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

    metricManager.removeRegistry(core.getCoreMetricManager().getRegistryName());
+    metricManager.closeMeterProvider(core.getCoreMetricManager().getRegistryName());


you could restore the previous name

Do you mean keep the name removeRegistry instead of a separate? If so, sure then I'll just move the closing of the meterProviders into removeRegistry until we completely move off. Made that change in latest commit

dsmiley · 2025-06-27T03:26:12Z

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

  public Set<String> registryNames() {
    Set<String> set = new HashSet<>();
    set.addAll(registries.keySet());
    set.addAll(SharedMetricRegistries.names());
    return set;
  }

+  public Set<String> providerNames() {


is this needed? We're calling these registries...

dsmiley · 2025-06-27T03:38:46Z

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

+import org.junit.Test;
+
+public class TestPrometheusResponseWriterCloud extends SolrCloudTestCase {
+  @ClassRule public static SolrJettyTestRule solrClientTestRule = new SolrJettyTestRule();


what is this doing here? It's a standalone Solr instance inside a SolrCloud test!

For your awareness, I have a plan to hopefully make a TestRule for SolrCloud, so that we need not extend SolrCloudTestCase just to test SolrCloud.

Mistake leaving that in. Was the result of me copy and pasting some things and modifying...

dsmiley · 2025-06-27T03:41:48Z

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

+            line -> {
+              if (line.startsWith("solr_metrics_core_requests_total")
+                  && line.contains("handler=\"/select\""))
+                assertTrue(


we should check that at least one line reaches this point :-)
Maybe use the anyMatch technique that you use later.

dsmiley · 2025-06-27T03:43:22Z

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

+    promReq.setResponseParser(new InputStreamResponseParser("prometheus"));
+
+    NamedList<Object> prometheusResponse = solrClient.request(promReq);
+    assertNotNull("null response from server", prometheusResponse);


IMO this is never needed

dsmiley · 2025-06-27T03:47:11Z

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

+    NamedList<Object> prometheusResponse = solrClient.request(promReq);
+    assertNotNull("null response from server", prometheusResponse);
+
+    InputStream in = (InputStream) prometheusResponse.get("stream");


these streams need to be closed

dsmiley · 2025-06-27T03:54:19Z

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

I think a tricky thing here right now is that SolrMetricManager is holding both DropWizard & OTEL metrics. Thus only one can have the method name we want (i.e. one with registry in the name). Seems you should make the leap to OTEL completely soon. Could go most of the way by retaining the DropWizard pervasive methods like counter, etc. and return NoopCounter and similar until it can completely go.

Its mostly the tests that blow the number of changes which is why I was trying to add otel in parallel then incrementally migrating a subset of dropwizard metrics and the corresponding tests using them. Since everything goes through metric manager, if I switch it to NoopCounter then every test relying on counter will fail. I'm fine just moving over and making one large PR with the migration by returning a NoopCounter if you prefer and then moving to OTEL completely.

But I also found a while ago there is something in solr that was parsing the dropwizard output and using those metrics. It may have been the UI or something but it was a failing a test and I found something about it making the fix not simple. Don't fully remember but I'll look for it again.

I have another somewhat small PR I wanted to put after this that add in a OTLP exporter that can be enabled. Although its debatable where that code should live which is what I wanted to bring up next.

mlbiscoc added 6 commits June 23, 2025 10:41

Create dynamic SDK Meter Providers and tests

e93f977

Merge branch 'feature/SOLR-17458' into temp-branch-otel

50ad1b4

Bad merge from feature branch

c416429

Wrap meter providers and metric readers

6b5a01f

Test cleanup

2b2095a

Cleanup

a3365b0

github-actions bot added test-framework tests cat:search cat:cloud cat:metrics labels Jun 23, 2025

mlbiscoc commented Jun 23, 2025

View reviewed changes

dsmiley self-requested a review June 25, 2025 13:33

dsmiley reviewed Jun 25, 2025

View reviewed changes

mlbiscoc added 3 commits June 25, 2025 16:32

Fix double lookups

ef73e32

Create a separate test suite for prometheus

e303f23

Move back to registry name

b92699c

dsmiley reviewed Jun 27, 2025

View reviewed changes

Changes from comments

f8c2681

dsmiley approved these changes Jun 27, 2025

View reviewed changes

mlbiscoc merged commit c7a5950 into apache:feature/SOLR-17458 Jun 30, 2025
1 check passed

		@@ -115,6 +115,8 @@ public class SolrMetricManager {

		private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());

		public static final String OTEL_SCOPE_NAME = "org.apache.solr";

		private final ConcurrentMap<String, MeterProviderAndReaders> meterProviderAndReaders =
		new ConcurrentHashMap<>();

		if (!meterProviderAndReaders.containsKey(name)) return null;
		return meterProviderAndReaders.get(name).prometheusMetricReader();

		metricManager.removeRegistry(core.getCoreMetricManager().getRegistryName());
		metricManager.closeMeterProvider(core.getCoreMetricManager().getRegistryName());

SOLR-17793: Create dedicated SdkMeterProviders for Solr core metrics #3402

SOLR-17793: Create dedicated SdkMeterProviders for Solr core metrics #3402

Uh oh!

Conversation

mlbiscoc commented Jun 23, 2025

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!

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!

mlbiscoc commented Jun 23, 2025

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!

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!

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!

Uh oh!