Re: [PR] SOLR-17774: Refactor initialization of GlobalOpenTelemetry [solr]
mlbiscoc merged PR #3379: URL: https://github.com/apache/solr/pull/3379 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17774: Refactor initialization of GlobalOpenTelemetry [solr]
mlbiscoc commented on PR #3379: URL: https://github.com/apache/solr/pull/3379#issuecomment-2959800308 Any other comments? Will merge in the next few days otherwise and open the next PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17774: Refactor initialization of GlobalOpenTelemetry [solr]
janhoy commented on code in PR #3379:
URL: https://github.com/apache/solr/pull/3379#discussion_r2134845126
##
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##
@@ -465,6 +480,39 @@ public CoreContainer(NodeConfig config, CoresLocator
locator, boolean asyncSolrC
new SolrNamedThreadFactory("IndexFingerprintPool"));
}
+ /**
Review Comment:
I asked for docs since there was a regression in the first pass. I agree
Javadocs primarily is for public APIs, so I'm happy if the intended order of
initialization is documented in a different way, e.g. inline the if-then-else
logic as oneline `//` comments. Just short and concise to make the flow easier
to grok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17774: Refactor initialization of GlobalOpenTelemetry [solr]
mlbiscoc commented on code in PR #3379: URL: https://github.com/apache/solr/pull/3379#discussion_r2132890849 ## solr/core/src/java/org/apache/solr/core/CoreContainer.java: ## @@ -423,6 +433,11 @@ public CoreContainer(NodeConfig config, CoresLocator locator, boolean asyncSolrC this.solrHome = config.getSolrHome(); this.solrCores = SolrCores.newSolrCores(this); this.nodeKeyPair = new SolrNodeKeyPair(cfg.getCloudConfig()); +this.prometheusMetricReader = new PrometheusMetricReader(true, null); +initializeOpenTelemetrySdk(); +this.tracer = TraceUtils.getGlobalTracer(); +this.meterProvider = MetricUtils.getMeterProvider(); Review Comment: Thats understandable. Moved it out of `coreContainer` and into `MetricManager` which makes the most sense. `MetricManager` creates instruments for measuring metrics and so also putting the `MetricReader` and `MeterProvider` in there as well was perfect. `initializeOpenTelemetrySdk` was moved into the `OpenTelemetryConfigurator` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17774: Refactor initialization of GlobalOpenTelemetry [solr]
dsmiley commented on code in PR #3379:
URL: https://github.com/apache/solr/pull/3379#discussion_r2132810513
##
solr/core/src/java/org/apache/solr/core/OpenTelemetryConfigurator.java:
##
@@ -46,26 +50,53 @@ public abstract class TracerConfigurator implements
NamedListInitializedPlugin {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- public static Tracer loadTracer(SolrResourceLoader loader, PluginInfo info) {
-if (info != null && info.isEnabled()) {
- TracerConfigurator configurator =
- loader.newInstance(info.className, TracerConfigurator.class);
- configurator.init(info.initArgs);
- ExecutorUtil.addThreadLocalProvider(new ContextThreadLocalProvider());
- return configurator.getTracer();
-}
-if (shouldAutoConfigOTEL()) {
- return autoConfigOTEL(loader);
-}
+ private static volatile boolean loaded = false;
+
+ public static synchronized void configureOpenTelemetrySdk(
+ SdkMeterProvider sdkMeterProvider, SdkTracerProvider sdkTracerProvider) {
+if (loaded) return;
Review Comment:
Agreed; I said the same in my OTEL Agent PR.
##
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##
@@ -423,6 +433,11 @@ public CoreContainer(NodeConfig config, CoresLocator
locator, boolean asyncSolrC
this.solrHome = config.getSolrHome();
this.solrCores = SolrCores.newSolrCores(this);
this.nodeKeyPair = new SolrNodeKeyPair(cfg.getCloudConfig());
+this.prometheusMetricReader = new PrometheusMetricReader(true, null);
+initializeOpenTelemetrySdk();
+this.tracer = TraceUtils.getGlobalTracer();
+this.meterProvider = MetricUtils.getMeterProvider();
Review Comment:
A growing concern on my mind is that CoreContainer is a kitchen sink and has
gotten out of control. That said, if you need (or makes most logical sense)
for a field on CoreContainer, really, then fine. If you can keep OTEL stuff to
another source file in some way, that would probably be ideal.
##
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##
@@ -465,6 +480,39 @@ public CoreContainer(NodeConfig config, CoresLocator
locator, boolean asyncSolrC
new SolrNamedThreadFactory("IndexFingerprintPool"));
}
+ /**
Review Comment:
Docs are always well intentioned but here, I think this is saying way too
much. It's about as much as there is code. This is a private method; why
bother? Docs need to be maintained as well; it's not a free gift (from
whatever person/LLM wrote it).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17774: Refactor initialization of GlobalOpenTelemetry [solr]
mlbiscoc commented on PR #3379: URL: https://github.com/apache/solr/pull/3379#issuecomment-2949660931 Going to bump this if any one interesting in looking? I have another PR almost ready that implements the foundation for working OTEL metrics. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17774: Refactor initialization of GlobalOpenTelemetry [solr]
janhoy commented on code in PR #3379:
URL: https://github.com/apache/solr/pull/3379#discussion_r2118268512
##
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##
@@ -465,6 +480,19 @@ public CoreContainer(NodeConfig config, CoresLocator
locator, boolean asyncSolrC
new SolrNamedThreadFactory("IndexFingerprintPool"));
}
+ private void initializeOpenTelemetrySdk() {
+PluginInfo info = cfg.getTracerConfiguratorPluginInfo();
+
+if (OpenTelemetryConfigurator.shouldAutoConfigOTEL()) {
+ OpenTelemetryConfigurator.autoConfigureOpenTelemetrySdk(loader);
+} else if (info != null && info.isEnabled()) {
+ OpenTelemetryConfigurator.loadOpenTelemetrySdk(loader,
cfg.getTracerConfiguratorPluginInfo());
Review Comment:
Thanks. I like the similar naming better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17774: Refactor initialization of GlobalOpenTelemetry [solr]
mlbiscoc commented on code in PR #3379:
URL: https://github.com/apache/solr/pull/3379#discussion_r2116805755
##
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##
@@ -465,6 +480,39 @@ public CoreContainer(NodeConfig config, CoresLocator
locator, boolean asyncSolrC
new SolrNamedThreadFactory("IndexFingerprintPool"));
}
+ /**
+ * Initializes the {@link io.opentelemetry.api.GlobalOpenTelemetry} instance
by configuring the
+ * {@link io.opentelemetry.sdk.OpenTelemetrySdk}. The initialization process
prioritizes in the
+ * following order:
+ *
+ * {@link PluginInfo} for an OpenTelemetry configurator is provided and
enabled, it will be
+ * used to load and initialize a custom OpenTelemetry SDK.
+ *
+ * If auto-configuration is enabled, the SDK will be auto-configured
using default behavior of
+ * AutoConfiguredOpenTelemetrySdk
+ *
+ * If neither is enabled, the default creates a basic SDK is configured
with a {@link
+ * SdkMeterProvider} and {@link
org.apache.solr.util.tracing.SimplePropagator} for context
+ * propagation
+ *
+ * @see OpenTelemetryConfigurator
+ */
+ private void initializeOpenTelemetrySdk() {
+PluginInfo info = cfg.getTracerConfiguratorPluginInfo();
+
+if (info != null && info.isEnabled()) {
+ OpenTelemetryConfigurator.configureCustomOpenTelemetrySdk(
+ loader, cfg.getTracerConfiguratorPluginInfo());
+} else if (OpenTelemetryConfigurator.shouldAutoConfigOTEL()) {
+ OpenTelemetryConfigurator.autoConfigureOpenTelemetrySdk(loader);
+} else {
+ // Initializing sampler as always off to replicate no-op Tracer provider
+ OpenTelemetryConfigurator.configureOpenTelemetrySdk(
+
SdkMeterProvider.builder().registerMetricReader(prometheusMetricReader).build(),
+ SdkTracerProvider.builder().setSampler(Sampler.alwaysOff()).build());
Review Comment:
@stillalex going to CC you since I saw you were the last author of the
`SimplePropagator`. When initializing the SDK, there is no `NOOP` Tracer
provider that I can set, and so the best I was able to do is just set it to
always off and I changed a few places around that were checking for `NOOP`.
You can see it in this [commit
here](https://github.com/apache/solr/pull/3379/commits/f34e9577733d22e636dc329233a0c29b11e6495e)
. This seems to be somewhat equivalent but let me know if you disagree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17774: Refactor initialization of GlobalOpenTelemetry [solr]
mlbiscoc commented on code in PR #3379:
URL: https://github.com/apache/solr/pull/3379#discussion_r2116269332
##
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##
@@ -465,6 +480,19 @@ public CoreContainer(NodeConfig config, CoresLocator
locator, boolean asyncSolrC
new SolrNamedThreadFactory("IndexFingerprintPool"));
}
+ private void initializeOpenTelemetrySdk() {
+PluginInfo info = cfg.getTracerConfiguratorPluginInfo();
+
+if (OpenTelemetryConfigurator.shouldAutoConfigOTEL()) {
+ OpenTelemetryConfigurator.autoConfigureOpenTelemetrySdk(loader);
+} else if (info != null && info.isEnabled()) {
+ OpenTelemetryConfigurator.loadOpenTelemetrySdk(loader,
cfg.getTracerConfiguratorPluginInfo());
Review Comment:
Unintended change by me when I was moving it. Good catch. I switched it back
around and added a javadoc that explains how it prioritizes (custom ->
autoconfigure -> default configuration).
And the naming was probably bad. I changed `load` to
`configureCustomOpenTelemetrySdk`. Maybe this is better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17774: Refactor initialization of GlobalOpenTelemetry [solr]
janhoy commented on code in PR #3379:
URL: https://github.com/apache/solr/pull/3379#discussion_r2115601188
##
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##
@@ -465,6 +480,19 @@ public CoreContainer(NodeConfig config, CoresLocator
locator, boolean asyncSolrC
new SolrNamedThreadFactory("IndexFingerprintPool"));
}
+ private void initializeOpenTelemetrySdk() {
+PluginInfo info = cfg.getTracerConfiguratorPluginInfo();
+
+if (OpenTelemetryConfigurator.shouldAutoConfigOTEL()) {
+ OpenTelemetryConfigurator.autoConfigureOpenTelemetrySdk(loader);
+} else if (info != null && info.isEnabled()) {
+ OpenTelemetryConfigurator.loadOpenTelemetrySdk(loader,
cfg.getTracerConfiguratorPluginInfo());
Review Comment:
In the existing tracer loading code
https://github.com/apache/solr/blob/main/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java#L49-L65
audoConfig of tracer would only happen if an explicit `` is not
present in solr.xml. It seems as if this is switched around here let
auto-config override explicit config in solr.xml?
Perhaps a short method javadoc to explain the intended logic?
The difference between `autoConfigure...` and `load...` is also not clear,
both will actually configure **and** load the sdk?
##
solr/core/src/java/org/apache/solr/core/OpenTelemetryConfigurator.java:
##
@@ -17,25 +17,29 @@
package org.apache.solr.core;
+import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
+import io.opentelemetry.context.propagation.ContextPropagators;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
+import io.opentelemetry.sdk.OpenTelemetrySdkBuilder;
+import io.opentelemetry.sdk.metrics.SdkMeterProvider;
+import io.opentelemetry.sdk.trace.SdkTracerProvider;
import java.lang.invoke.MethodHandles;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;
-import org.apache.solr.common.SolrException;
import org.apache.solr.common.util.EnvUtils;
import org.apache.solr.common.util.ExecutorUtil;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.util.plugin.NamedListInitializedPlugin;
import org.apache.solr.util.tracing.SimplePropagator;
-import org.apache.solr.util.tracing.TraceUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/** Produces a {@link Tracer} from configuration. */
Review Comment:
Update javadoc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17774: Refactor initialization of GlobalOpenTelemetry [solr]
mlbiscoc commented on code in PR #3379:
URL: https://github.com/apache/solr/pull/3379#discussion_r2114832472
##
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##
@@ -423,6 +433,11 @@ public CoreContainer(NodeConfig config, CoresLocator
locator, boolean asyncSolrC
this.solrHome = config.getSolrHome();
this.solrCores = SolrCores.newSolrCores(this);
this.nodeKeyPair = new SolrNodeKeyPair(cfg.getCloudConfig());
+this.prometheusMetricReader = new PrometheusMetricReader(true, null);
+initializeOpenTelemetrySdk();
+this.tracer = TraceUtils.getGlobalTracer();
+this.meterProvider = MetricUtils.getMeterProvider();
Review Comment:
This was initially done with `TracerConfigurator.loadTracer` inside of
`loadInternal` Moved it up to be called earlier.
##
solr/core/src/java/org/apache/solr/core/OpenTelemetryConfigurator.java:
##
@@ -46,26 +50,53 @@ public abstract class TracerConfigurator implements
NamedListInitializedPlugin {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- public static Tracer loadTracer(SolrResourceLoader loader, PluginInfo info) {
-if (info != null && info.isEnabled()) {
- TracerConfigurator configurator =
- loader.newInstance(info.className, TracerConfigurator.class);
- configurator.init(info.initArgs);
- ExecutorUtil.addThreadLocalProvider(new ContextThreadLocalProvider());
- return configurator.getTracer();
-}
-if (shouldAutoConfigOTEL()) {
- return autoConfigOTEL(loader);
-}
+ private static volatile boolean loaded = false;
+
+ public static synchronized void configureOpenTelemetrySdk(
+ SdkMeterProvider sdkMeterProvider, SdkTracerProvider sdkTracerProvider) {
+if (loaded) return;
Review Comment:
Its a bit annoying Open Telemetry doesn't have a method to call if the
`GlobalOpenTelemetry` was already set. So I understand why this boolean loaded
existed on `SimplePropagator` now.
##
solr/core/src/java/org/apache/solr/util/tracing/SimplePropagator.java:
##
@@ -54,19 +50,6 @@ public class SimplePropagator implements TextMapPropagator {
private static final AtomicLong traceCounter = new AtomicLong(0);
- private static volatile boolean loaded = false;
-
- public static synchronized Tracer load() {
-if (!loaded) {
- log.info("OpenTelemetry tracer enabled with simple propagation only.");
- OpenTelemetry otel =
-
OpenTelemetry.propagating(ContextPropagators.create(SimplePropagator.getInstance()));
- GlobalOpenTelemetry.set(otel);
- loaded = true;
-}
-return TraceUtils.getGlobalTracer();
- }
Review Comment:
This basically moved into the new `OpenTelemetryConfigurator`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
