dsmiley commented on code in PR #2687:
URL: https://github.com/apache/solr/pull/2687#discussion_r1740338583
##########
solr/core/src/java/org/apache/solr/core/TracerConfigurator.java:
##########
@@ -38,33 +41,53 @@
public abstract class TracerConfigurator implements NamedListInitializedPlugin
{
public static final boolean TRACE_ID_GEN_ENABLED =
- Boolean.parseBoolean(EnvUtils.getProperty("solr.alwaysOnTraceId",
"true"));
+ EnvUtils.getPropertyAsBool("solr.alwaysOnTraceId", true);
private static final String DEFAULT_CLASS_NAME =
EnvUtils.getProperty(
"solr.otelDefaultConfigurator",
"org.apache.solr.opentelemetry.OtelTracerConfigurator");
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);
+ /** Initializes {@link io.opentelemetry.api.GlobalOpenTelemetry} and returns
a Tracer. */
+ public static synchronized Tracer loadTracer(SolrResourceLoader loader,
PluginInfo info) {
+ // synchronized to avoid races in tests starting Solr concurrently setting
GlobalOpenTelemetry
+ if (TraceUtils.OTEL_AGENT_PRESENT) {
+ log.info("OpenTelemetry Java agent was already installed; skipping OTEL
initialization");
+ } else {
+ OpenTelemetry otel = null;
+ if (info != null && info.isEnabled()) {
+ var configurator = loader.newInstance(info.className,
TracerConfigurator.class);
+ configurator.init(info.initArgs);
+ otel = configurator.createOtel();
+ } else if (shouldAutoConfigOTEL()) {
+ otel = autoConfigOTEL(loader); // could return null if failed to load
+ if (otel != null) {
+ log.info("OpenTelemetry loaded via auto configuration.");
+ }
+ }
+ if (otel == null && TRACE_ID_GEN_ENABLED) {
+ otel =
OpenTelemetry.propagating(ContextPropagators.create(SimplePropagator.getInstance()));
+ log.info("OpenTelemetry loaded with simple propagation only.");
+ }
+ if (otel == null) {
+ otel = OpenTelemetry.noop();
+ }
+
+ try {
+ GlobalOpenTelemetry.set(otel); // throws IllegalStateException if
already set
Review Comment:
this is the one spot where we call this. Previously it was in multiple
places which was confusing and hard to reason about.
##########
solr/core/src/java/org/apache/solr/core/TracerConfigurator.java:
##########
@@ -38,33 +41,53 @@
public abstract class TracerConfigurator implements NamedListInitializedPlugin
{
public static final boolean TRACE_ID_GEN_ENABLED =
- Boolean.parseBoolean(EnvUtils.getProperty("solr.alwaysOnTraceId",
"true"));
+ EnvUtils.getPropertyAsBool("solr.alwaysOnTraceId", true);
private static final String DEFAULT_CLASS_NAME =
EnvUtils.getProperty(
"solr.otelDefaultConfigurator",
"org.apache.solr.opentelemetry.OtelTracerConfigurator");
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);
+ /** Initializes {@link io.opentelemetry.api.GlobalOpenTelemetry} and returns
a Tracer. */
+ public static synchronized Tracer loadTracer(SolrResourceLoader loader,
PluginInfo info) {
+ // synchronized to avoid races in tests starting Solr concurrently setting
GlobalOpenTelemetry
+ if (TraceUtils.OTEL_AGENT_PRESENT) {
+ log.info("OpenTelemetry Java agent was already installed; skipping OTEL
initialization");
+ } else {
+ OpenTelemetry otel = null;
+ if (info != null && info.isEnabled()) {
+ var configurator = loader.newInstance(info.className,
TracerConfigurator.class);
+ configurator.init(info.initArgs);
+ otel = configurator.createOtel();
+ } else if (shouldAutoConfigOTEL()) {
+ otel = autoConfigOTEL(loader); // could return null if failed to load
+ if (otel != null) {
+ log.info("OpenTelemetry loaded via auto configuration.");
+ }
+ }
+ if (otel == null && TRACE_ID_GEN_ENABLED) {
+ otel =
OpenTelemetry.propagating(ContextPropagators.create(SimplePropagator.getInstance()));
+ log.info("OpenTelemetry loaded with simple propagation only.");
+ }
+ if (otel == null) {
+ otel = OpenTelemetry.noop();
+ }
+
+ try {
+ GlobalOpenTelemetry.set(otel); // throws IllegalStateException if
already set
+ // skips needless ExecutorUtil.addThreadLocalProvider below
+ if (otel == OpenTelemetry.noop()) return otel.getTracer("solr");
+ } catch (IllegalStateException e) {
+ log.info("GlobalOpenTelemetry was already initialized (a Java agent?);
using that");
Review Comment:
Note that its common tests will log "OpenTelemetry loaded with simple
propagation only." but then log immediately after "GlobalOpenTelemetry was
already initialized (a Java agent?); using that" if the test uses more than one
CoreContainer (Solr node). I so wish GlobalOpenTelemetry had a way to know if
it's been set already.
##########
solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/BasicAuthIntegrationTracingTest.java:
##########
@@ -31,23 +33,23 @@
import org.apache.solr.security.BasicAuthPlugin;
import org.apache.solr.util.SecurityJson;
import org.apache.solr.util.tracing.TraceUtils;
-import org.junit.AfterClass;
import org.junit.BeforeClass;
+import org.junit.ClassRule;
import org.junit.Test;
public class BasicAuthIntegrationTracingTest extends SolrCloudTestCase {
private static final String COLLECTION = "collection1";
+ @ClassRule public static OpenTelemetryRule otelRule =
OpenTelemetryRule.create();
Review Comment:
I was about to write an equivalent of OpenTelemetryRule when I found that it
already exists :-)
FYI note it has docs that it doesn't work in a ClassRule yet I see it works
here.
##########
solr/core/src/java/org/apache/solr/core/TracerConfigurator.java:
##########
@@ -38,33 +41,53 @@
public abstract class TracerConfigurator implements NamedListInitializedPlugin
{
public static final boolean TRACE_ID_GEN_ENABLED =
- Boolean.parseBoolean(EnvUtils.getProperty("solr.alwaysOnTraceId",
"true"));
+ EnvUtils.getPropertyAsBool("solr.alwaysOnTraceId", true);
private static final String DEFAULT_CLASS_NAME =
EnvUtils.getProperty(
"solr.otelDefaultConfigurator",
"org.apache.solr.opentelemetry.OtelTracerConfigurator");
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);
+ /** Initializes {@link io.opentelemetry.api.GlobalOpenTelemetry} and returns
a Tracer. */
+ public static synchronized Tracer loadTracer(SolrResourceLoader loader,
PluginInfo info) {
+ // synchronized to avoid races in tests starting Solr concurrently setting
GlobalOpenTelemetry
+ if (TraceUtils.OTEL_AGENT_PRESENT) {
+ log.info("OpenTelemetry Java agent was already installed; skipping OTEL
initialization");
+ } else {
+ OpenTelemetry otel = null;
+ if (info != null && info.isEnabled()) {
+ var configurator = loader.newInstance(info.className,
TracerConfigurator.class);
+ configurator.init(info.initArgs);
+ otel = configurator.createOtel();
+ } else if (shouldAutoConfigOTEL()) {
+ otel = autoConfigOTEL(loader); // could return null if failed to load
+ if (otel != null) {
+ log.info("OpenTelemetry loaded via auto configuration.");
+ }
+ }
+ if (otel == null && TRACE_ID_GEN_ENABLED) {
+ otel =
OpenTelemetry.propagating(ContextPropagators.create(SimplePropagator.getInstance()));
+ log.info("OpenTelemetry loaded with simple propagation only.");
+ }
+ if (otel == null) {
+ otel = OpenTelemetry.noop();
+ }
+
+ try {
+ GlobalOpenTelemetry.set(otel); // throws IllegalStateException if
already set
+ // skips needless ExecutorUtil.addThreadLocalProvider below
+ if (otel == OpenTelemetry.noop()) return otel.getTracer("solr");
+ } catch (IllegalStateException e) {
+ log.info("GlobalOpenTelemetry was already initialized (a Java agent?);
using that");
+ }
}
- if (TRACE_ID_GEN_ENABLED) {
- ExecutorUtil.addThreadLocalProvider(new ContextThreadLocalProvider());
- return SimplePropagator.load();
- }
- return TraceUtils.getGlobalTracer();
+
+ ExecutorUtil.addThreadLocalProvider(new ContextThreadLocalProvider());
+ return TraceUtils.getGlobalTracer(); // GlobalOpenTelemetry.get...
}
- protected abstract Tracer getTracer();
+ protected abstract OpenTelemetry createOtel();
Review Comment:
this is the API change
##########
solr/core/src/java/org/apache/solr/core/TracerConfigurator.java:
##########
@@ -38,33 +41,53 @@
public abstract class TracerConfigurator implements NamedListInitializedPlugin
{
public static final boolean TRACE_ID_GEN_ENABLED =
- Boolean.parseBoolean(EnvUtils.getProperty("solr.alwaysOnTraceId",
"true"));
+ EnvUtils.getPropertyAsBool("solr.alwaysOnTraceId", true);
private static final String DEFAULT_CLASS_NAME =
EnvUtils.getProperty(
"solr.otelDefaultConfigurator",
"org.apache.solr.opentelemetry.OtelTracerConfigurator");
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);
+ /** Initializes {@link io.opentelemetry.api.GlobalOpenTelemetry} and returns
a Tracer. */
+ public static synchronized Tracer loadTracer(SolrResourceLoader loader,
PluginInfo info) {
+ // synchronized to avoid races in tests starting Solr concurrently setting
GlobalOpenTelemetry
+ if (TraceUtils.OTEL_AGENT_PRESENT) {
+ log.info("OpenTelemetry Java agent was already installed; skipping OTEL
initialization");
+ } else {
+ OpenTelemetry otel = null;
+ if (info != null && info.isEnabled()) {
+ var configurator = loader.newInstance(info.className,
TracerConfigurator.class);
+ configurator.init(info.initArgs);
+ otel = configurator.createOtel();
+ } else if (shouldAutoConfigOTEL()) {
+ otel = autoConfigOTEL(loader); // could return null if failed to load
+ if (otel != null) {
+ log.info("OpenTelemetry loaded via auto configuration.");
+ }
+ }
+ if (otel == null && TRACE_ID_GEN_ENABLED) {
+ otel =
OpenTelemetry.propagating(ContextPropagators.create(SimplePropagator.getInstance()));
+ log.info("OpenTelemetry loaded with simple propagation only.");
+ }
+ if (otel == null) {
+ otel = OpenTelemetry.noop();
+ }
+
+ try {
+ GlobalOpenTelemetry.set(otel); // throws IllegalStateException if
already set
+ // skips needless ExecutorUtil.addThreadLocalProvider below
+ if (otel == OpenTelemetry.noop()) return otel.getTracer("solr");
+ } catch (IllegalStateException e) {
+ log.info("GlobalOpenTelemetry was already initialized (a Java agent?);
using that");
+ }
}
- if (TRACE_ID_GEN_ENABLED) {
- ExecutorUtil.addThreadLocalProvider(new ContextThreadLocalProvider());
- return SimplePropagator.load();
- }
- return TraceUtils.getGlobalTracer();
+
+ ExecutorUtil.addThreadLocalProvider(new ContextThreadLocalProvider());
Review Comment:
Now called in exactly one spot, which I find easier to read than before
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -49,14 +48,13 @@ public OtelTracerConfigurator() {
}
@Override
- public Tracer getTracer() {
- return TraceUtils.getGlobalTracer();
Review Comment:
A needless GlobalOpenTelemetry reference
##########
solr/core/src/java/org/apache/solr/core/TracerConfigurator.java:
##########
@@ -38,33 +41,53 @@
public abstract class TracerConfigurator implements NamedListInitializedPlugin
{
public static final boolean TRACE_ID_GEN_ENABLED =
- Boolean.parseBoolean(EnvUtils.getProperty("solr.alwaysOnTraceId",
"true"));
+ EnvUtils.getPropertyAsBool("solr.alwaysOnTraceId", true);
private static final String DEFAULT_CLASS_NAME =
EnvUtils.getProperty(
"solr.otelDefaultConfigurator",
"org.apache.solr.opentelemetry.OtelTracerConfigurator");
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);
+ /** Initializes {@link io.opentelemetry.api.GlobalOpenTelemetry} and returns
a Tracer. */
+ public static synchronized Tracer loadTracer(SolrResourceLoader loader,
PluginInfo info) {
+ // synchronized to avoid races in tests starting Solr concurrently setting
GlobalOpenTelemetry
Review Comment:
I didn't personally encounter an issue here but saw existing comments (from
Alex?) that suggested to me making this synchronized would be helpful. I don't
think there's a true issue; ultimately GlobalOpenTelemetry will be set exactly
once so it doesn't much matter if others try and race to do so.
##########
solr/modules/opentelemetry/src/test-files/solr/solr.xml:
##########
@@ -34,8 +34,6 @@
<int name="connTimeout">${connTimeout:15000}</int>
</shardHandlerFactory>
- <tracerConfig name="tracerConfig"
class="org.apache.solr.opentelemetry.CustomTestOtelTracerConfigurator" />
Review Comment:
Perhaps this class could return but only for a test explicitly testing that
tracerConfig works and in a separate solr.xml for exactly that test as well
(not for the other tracing tests). Not to be used for other tests or trying to
collect spans for examination.
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -49,14 +48,13 @@ public OtelTracerConfigurator() {
}
@Override
- public Tracer getTracer() {
- return TraceUtils.getGlobalTracer();
+ public void init(NamedList<?> args) {
+ prepareConfiguration(args);
}
@Override
- public void init(NamedList<?> args) {
- prepareConfiguration(args);
- AutoConfiguredOpenTelemetrySdk.initialize();
+ public OpenTelemetry createOtel() {
+ return
AutoConfiguredOpenTelemetrySdk.builder().build().getOpenTelemetrySdk();
Review Comment:
notice the change to *not* call initialize. That method arranges for
GlobalOpenTelemetry to be set. I only want that set in TracerConfigurator.
##########
solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/CustomTestOtelTracerConfigurator.java:
##########
Review Comment:
this thing was strange; didn't need to exist, at least not as it existed
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
