stillalex commented on code in PR #2687:
URL: https://github.com/apache/solr/pull/2687#discussion_r2066362901
##########
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:
hmm. if I remember correctly this is why I created
`CustomTestOtelTracerConfigurator` so tests that race to set this would not
fail. it felt awkward to have to catch `IllegalStateException` for problems
that are triggered only via testing code.
I also remember there was an issue with checking for noop? there were some
docs that were stating you should not check if the impl is noop, but I can't
find a reference now.
--
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
