mlbiscoc commented on code in PR #2687:
URL: https://github.com/apache/solr/pull/2687#discussion_r2029138937
##########
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:
Maybe out of scope but I feel like this whole setting of
`GlobalOpenTelemetry.set(otel)` should sit in its own function called
`loadGlobalOpenTelemetry` and be called as [early as possible as per its
documentation](https://opentelemetry.io/docs/languages/java/api/#:~:text=If%20GlobalOpenTelemetry.get,by%20any%20instrumentation.)
maybe inside the constructor of CoreContainer. Then on initialization of Solr
core container, to set it's tracer just call `TraceUtils.getGlobalTracer()`.
But also this whole file is called `TracerConfigurator` when a lot of work is
configuring OTel then just returning a static global tracer. This same logic is
still used for metrics as it uses the same Java Agent and OTLP exporter from
the module not just Tracer loading.
--
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
