From 1b6c055770d41df496ae974d403cc1eb6e083d3b Mon Sep 17 00:00:00 2001 From: Alex Deparvu Date: Fri, 8 Sep 2023 05:23:34 -0700 Subject: [PATCH 1/2] SOLR-16938 Auto configure tracer without a tag in solr.xml --- .../apache/solr/core/TracerConfigurator.java | 57 +++++++++++++++++-- .../solr/util/tracing/SimplePropagator.java | 2 +- .../apache/solr/util/tracing/TraceUtils.java | 6 +- .../solr/core/TestTracerConfigurator.java | 43 ++++++++++++++ .../opentelemetry/OtelTracerConfigurator.java | 24 ++++++-- .../CustomTestOtelTracerConfigurator.java | 2 +- .../OtelTracerConfiguratorTest.java | 13 ++++- 7 files changed, 131 insertions(+), 16 deletions(-) create mode 100644 solr/core/src/test/org/apache/solr/core/TestTracerConfigurator.java diff --git a/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java b/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java index 633201db82f..a50f914955a 100644 --- a/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java +++ b/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java @@ -18,9 +18,15 @@ package org.apache.solr.core; import io.opentelemetry.api.trace.Tracer; +import java.lang.invoke.MethodHandles; +import java.util.Locale; +import org.apache.solr.common.SolrException; +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. */ public abstract class TracerConfigurator implements NamedListInitializedPlugin { @@ -28,19 +34,62 @@ public abstract class TracerConfigurator implements NamedListInitializedPlugin { public static final boolean TRACE_ID_GEN_ENABLED = Boolean.parseBoolean(System.getProperty("solr.alwaysOnTraceId", "true")); + private static final String DEFAULT_CLASS_NAME = + System.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); return configurator.getTracer(); - - } else if (TRACE_ID_GEN_ENABLED) { + } + if (shouldAutoConfigOTEL()) { + return autoConfigOTEL(loader); + } + if (TRACE_ID_GEN_ENABLED) { return SimplePropagator.load(); - } else { - return TraceUtils.noop(); } + return TraceUtils.getGlobalTracer(); } protected abstract Tracer getTracer(); + + private static Tracer autoConfigOTEL(SolrResourceLoader loader) { + try { + TracerConfigurator configurator = + loader.newInstance(DEFAULT_CLASS_NAME, TracerConfigurator.class); + configurator.init(new NamedList<>()); + return configurator.getTracer(); + } catch (SolrException e) { + log.error( + "Unable to auto-config OpenTelemetry with class {}. Make sure you have enabled the 'opentelemetry' module", + DEFAULT_CLASS_NAME, + e); + } + return TraceUtils.getGlobalTracer(); + } + + /** + * best effort way to determine if we should attempt to init OTEL from system properties. + * + * @return true if OTEL should be init + */ + static boolean shouldAutoConfigOTEL() { + boolean isSdkDisabled = Boolean.parseBoolean(getConfig("OTEL_SDK_DISABLED")); + if (isSdkDisabled) { + return false; + } + return getConfig("OTEL_SERVICE_NAME") != null; + } + + private static String getConfig(String envName) { + String envValue = System.getenv().get(envName); + String sysName = envName.toLowerCase(Locale.ROOT).replace("_", "."); + String sysValue = System.getProperty(sysName); + return sysValue != null ? sysValue : envValue; + } } diff --git a/solr/core/src/java/org/apache/solr/util/tracing/SimplePropagator.java b/solr/core/src/java/org/apache/solr/util/tracing/SimplePropagator.java index aa029899347..e81657ca2ae 100644 --- a/solr/core/src/java/org/apache/solr/util/tracing/SimplePropagator.java +++ b/solr/core/src/java/org/apache/solr/util/tracing/SimplePropagator.java @@ -64,7 +64,7 @@ public static synchronized Tracer load() { GlobalOpenTelemetry.set(otel); loaded = true; } - return GlobalOpenTelemetry.getTracer("solr"); + return TraceUtils.getGlobalTracer(); } public static TextMapPropagator getInstance() { diff --git a/solr/core/src/java/org/apache/solr/util/tracing/TraceUtils.java b/solr/core/src/java/org/apache/solr/util/tracing/TraceUtils.java index e63d8c6cbde..4d305f373fd 100644 --- a/solr/core/src/java/org/apache/solr/util/tracing/TraceUtils.java +++ b/solr/core/src/java/org/apache/solr/util/tracing/TraceUtils.java @@ -22,7 +22,6 @@ import io.opentelemetry.api.trace.SpanBuilder; import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.api.trace.Tracer; -import io.opentelemetry.api.trace.TracerProvider; import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.TextMapPropagator; import java.util.List; @@ -36,7 +35,6 @@ public class TraceUtils { private static final String REQ_ATTR_TRACING_SPAN = Span.class.getName(); private static final String REQ_ATTR_TRACING_TRACER = Tracer.class.getName(); - private static final Tracer NOOP_TRACER = TracerProvider.noop().get(null); public static final String DEFAULT_SPAN_NAME = "http.request"; public static final String WRITE_QUERY_RESPONSE_SPAN_NAME = "writeQueryResponse"; @@ -66,8 +64,8 @@ public class TraceUtils { public static final String TAG_DB_TYPE_SOLR = "solr"; - public static Tracer noop() { - return NOOP_TRACER; + public static Tracer getGlobalTracer() { + return GlobalOpenTelemetry.getTracer("solr"); } public static TextMapPropagator getTextMapPropagator() { diff --git a/solr/core/src/test/org/apache/solr/core/TestTracerConfigurator.java b/solr/core/src/test/org/apache/solr/core/TestTracerConfigurator.java new file mode 100644 index 00000000000..2d9f06b400e --- /dev/null +++ b/solr/core/src/test/org/apache/solr/core/TestTracerConfigurator.java @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.core; + +import io.opentelemetry.api.trace.TracerProvider; +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.util.tracing.TraceUtils; +import org.junit.Test; + +public class TestTracerConfigurator extends SolrTestCaseJ4 { + + @Test + public void configuratorClassDoesNotExistTest() { + System.setProperty("otel.service.name", "something"); + System.setProperty("solr.otelDefaultConfigurator", "configuratorClassDoesNotExistTest"); + try { + assertTrue(TracerConfigurator.shouldAutoConfigOTEL()); + SolrResourceLoader loader = new SolrResourceLoader(TEST_PATH().resolve("collection1")); + TracerConfigurator.loadTracer(loader, null); + assertEquals( + "Expecting noop otel after failure to auto-init", + TracerProvider.noop().get(null), + TraceUtils.getGlobalTracer()); + } finally { + System.clearProperty("solr.otelDefaultConfigurator"); + System.clearProperty("otel.service.name"); + } + } +} diff --git a/solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java b/solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java index 25d819718dc..e3820ce83c4 100644 --- a/solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java +++ b/solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java @@ -16,7 +16,6 @@ */ package org.apache.solr.opentelemetry; -import io.opentelemetry.api.GlobalOpenTelemetry; import io.opentelemetry.api.trace.Tracer; import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk; import java.lang.invoke.MethodHandles; @@ -28,6 +27,7 @@ import java.util.stream.Collectors; import org.apache.solr.common.util.NamedList; import org.apache.solr.core.TracerConfigurator; +import org.apache.solr.util.tracing.TraceUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -49,17 +49,17 @@ public OtelTracerConfigurator() { @Override public Tracer getTracer() { - // TODO remove reliance on global - return GlobalOpenTelemetry.getTracer("solr"); + return TraceUtils.getGlobalTracer(); } @Override public void init(NamedList args) { - prepareConfiguration(); + prepareConfiguration(args); AutoConfiguredOpenTelemetrySdk.initialize(); } - void prepareConfiguration() { + void prepareConfiguration(NamedList args) { + injectPluginSettingsIfNotConfigured(args); setDefaultIfNotConfigured("OTEL_SERVICE_NAME", "solr"); setDefaultIfNotConfigured("OTEL_TRACES_EXPORTER", "otlp"); setDefaultIfNotConfigured("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc"); @@ -82,6 +82,20 @@ void prepareConfiguration() { System.setProperty("otel.logs.exporter", "none"); } + /** + * Will inject plugin configuration values into system properties if not already setup (existing + * system properties take precedence) + */ + private void injectPluginSettingsIfNotConfigured(NamedList args) { + args.forEach( + (k, v) -> { + var asSysName = envNameToSyspropName(k); + if (asSysName.startsWith("otel.")) { + setDefaultIfNotConfigured(asSysName, v.toString()); + } + }); + } + /** * Add explicit tags statically to all traces, independent of request. Attributes with same name * supplied in ENV or SysProp will take precedence over attributes added in code. diff --git a/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/CustomTestOtelTracerConfigurator.java b/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/CustomTestOtelTracerConfigurator.java index bad6cce7d9f..cf7723f89ed 100644 --- a/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/CustomTestOtelTracerConfigurator.java +++ b/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/CustomTestOtelTracerConfigurator.java @@ -62,7 +62,7 @@ public static synchronized void prepareForTest() { // force early init CustomTestOtelTracerConfigurator tracer = new CustomTestOtelTracerConfigurator(); - tracer.prepareConfiguration(); + tracer.prepareConfiguration(new NamedList<>()); bootOtel(); } diff --git a/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/OtelTracerConfiguratorTest.java b/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/OtelTracerConfiguratorTest.java index b4825c4e7fd..c33db57d28f 100644 --- a/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/OtelTracerConfiguratorTest.java +++ b/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/OtelTracerConfiguratorTest.java @@ -19,6 +19,7 @@ import java.util.List; import java.util.Map; import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.util.NamedList; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -88,9 +89,19 @@ public void testSetDefaultIfNotConfigured() { @Test public void testResourceAttributes() throws Exception { System.setProperty("otel.resource.attributes", "foo=bar,ILLEGAL-LACKS-VALUE,"); - instance.prepareConfiguration(); + instance.prepareConfiguration(new NamedList<>()); assertEquals( List.of("host.name=my.solr.host", "foo=bar"), List.of(System.getProperty("otel.resource.attributes").split(","))); } + + @Test + public void testPluginConfig() throws Exception { + NamedList conf = new NamedList<>(); + conf.add("OTEL_K1", "conf-k1"); // will be replaced by sys prop + conf.add("otel.k7", "conf-k7"); // will be kept + instance.prepareConfiguration(conf); + assertEquals("prop-k1", instance.getCurrentOtelConfig().get("OTEL_K1")); + assertEquals("conf-k7", instance.getCurrentOtelConfig().get("OTEL_K7")); + } } From 1b28140c688efd51abccb74ab63d55651c41f0ab Mon Sep 17 00:00:00 2001 From: Alex Deparvu Date: Thu, 14 Sep 2023 12:46:52 -0700 Subject: [PATCH 2/2] PR feedback, some more minor changes --- solr/CHANGES.txt | 2 ++ .../apache/solr/core/TracerConfigurator.java | 32 +++++++++++++++---- .../solr/core/TestTracerConfigurator.java | 10 ++++++ .../opentelemetry/OtelTracerConfigurator.java | 17 ++-------- solr/server/resources/log4j2.xml | 6 ++-- .../pages/distributed-tracing.adoc | 2 ++ .../solr/cloud/MiniSolrCloudCluster.java | 7 ++++ 7 files changed, 52 insertions(+), 24 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 93648908345..4e988550737 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -128,6 +128,8 @@ Improvements * SOLR-16461: `/solr/coreName/replication?command=backup` now has a v2 equivalent, available at `/api/cores/coreName/replication/backups` (Sanjay Dutt, Jason Gerlowski, Alex Deparvu) +* SOLR-16938: Auto configure tracer without a tag in solr.xml (Alex Deparvu) + Optimizations --------------------- diff --git a/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java b/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java index a50f914955a..47e5d32cb7b 100644 --- a/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java +++ b/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java @@ -20,6 +20,7 @@ import io.opentelemetry.api.trace.Tracer; import java.lang.invoke.MethodHandles; import java.util.Locale; +import java.util.Map; import org.apache.solr.common.SolrException; import org.apache.solr.common.util.NamedList; import org.apache.solr.util.plugin.NamedListInitializedPlugin; @@ -74,22 +75,41 @@ private static Tracer autoConfigOTEL(SolrResourceLoader loader) { } /** - * best effort way to determine if we should attempt to init OTEL from system properties. + * Best effort way to determine if we should attempt to init OTEL from system properties. * * @return true if OTEL should be init */ static boolean shouldAutoConfigOTEL() { - boolean isSdkDisabled = Boolean.parseBoolean(getConfig("OTEL_SDK_DISABLED")); + var env = System.getenv(); + boolean isSdkDisabled = Boolean.parseBoolean(getConfig("OTEL_SDK_DISABLED", env)); if (isSdkDisabled) { return false; } - return getConfig("OTEL_SERVICE_NAME") != null; + return getConfig("OTEL_SERVICE_NAME", env) != null; } - private static String getConfig(String envName) { - String envValue = System.getenv().get(envName); - String sysName = envName.toLowerCase(Locale.ROOT).replace("_", "."); + /** + * Returns system property if found, else returns environment variable, or null if none found. + * + * @param envName the environment variable to look for + * @param env current env + * @return the resolved value + */ + protected static String getConfig(String envName, Map env) { + String sysName = envNameToSyspropName(envName); String sysValue = System.getProperty(sysName); + String envValue = env.get(envName); return sysValue != null ? sysValue : envValue; } + + /** + * In OTEL Java SDK there is a convention that the java property name for OTEL_FOO_BAR is + * otel.foo.bar + * + * @param envName the environmnet name to convert + * @return the corresponding sysprop name + */ + protected static String envNameToSyspropName(String envName) { + return envName.toLowerCase(Locale.ROOT).replace("_", "."); + } } diff --git a/solr/core/src/test/org/apache/solr/core/TestTracerConfigurator.java b/solr/core/src/test/org/apache/solr/core/TestTracerConfigurator.java index 2d9f06b400e..4882a295dfa 100644 --- a/solr/core/src/test/org/apache/solr/core/TestTracerConfigurator.java +++ b/solr/core/src/test/org/apache/solr/core/TestTracerConfigurator.java @@ -40,4 +40,14 @@ public void configuratorClassDoesNotExistTest() { System.clearProperty("otel.service.name"); } } + + @Test + public void otelDisabledByProperty() { + System.setProperty("OTEL_SDK_DISABLED", "true"); + try { + assertFalse(TracerConfigurator.shouldAutoConfigOTEL()); + } finally { + System.clearProperty("OTEL_SDK_DISABLED"); + } + } } diff --git a/solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java b/solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java index e3820ce83c4..221cb571b7b 100644 --- a/solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java +++ b/solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java @@ -152,24 +152,11 @@ Map getCurrentOtelConfig() { /** * Returns system property if found, else returns environment variable, or null if none found. * - * @param envName the environment to look for + * @param envName the environment variable to look for * @return the resolved value */ String getEnvOrSysprop(String envName) { - String envValue = currentEnv.get(envName); - String propValue = System.getProperty(envNameToSyspropName(envName)); - return propValue != null ? propValue : envValue; - } - - /** - * In OTEL Java SDK there is a convention that the java property name for OTEL_FOO_BAR is - * otel.foo.bar - * - * @param envName the environmnet name to convert - * @return the corresponding sysprop name - */ - static String envNameToSyspropName(String envName) { - return envName.toLowerCase(Locale.ROOT).replace("_", "."); + return getConfig(envName, currentEnv); } /** diff --git a/solr/server/resources/log4j2.xml b/solr/server/resources/log4j2.xml index 752c740514a..006de0c965c 100644 --- a/solr/server/resources/log4j2.xml +++ b/solr/server/resources/log4j2.xml @@ -23,7 +23,7 @@ - %maxLen{%d{yyyy-MM-dd HH:mm:ss.SSS} %-5p (%t) [%notEmpty{t:%X{trace_id}}%notEmpty{ c:%X{collection}}%notEmpty{ s:%X{shard}}%notEmpty{ r:%X{replica}}%notEmpty{ x:%X{core}}] %c{1.} %m%notEmpty{ =>%ex{short}}}{10240}%n + %maxLen{%d{yyyy-MM-dd HH:mm:ss.SSS} %-5p (%t) [%notEmpty{c:%X{collection}}%notEmpty{ s:%X{shard}}%notEmpty{ r:%X{replica}}%notEmpty{ x:%X{core}}%notEmpty{ t:%X{trace_id}}] %c{1.} %m%notEmpty{ =>%ex{short}}}{10240}%n @@ -34,7 +34,7 @@ filePattern="${sys:solr.log.dir}/solr.log.%i" > - %maxLen{%d{yyyy-MM-dd HH:mm:ss.SSS} %-5p (%t) [%notEmpty{t:%X{trace_id}}%notEmpty{ c:%X{collection}}%notEmpty{ s:%X{shard}}%notEmpty{ r:%X{replica}}%notEmpty{ x:%X{core}}] %c{1.} %m%notEmpty{ =>%ex{short}}}{10240}%n + %maxLen{%d{yyyy-MM-dd HH:mm:ss.SSS} %-5p (%t) [%notEmpty{c:%X{collection}}%notEmpty{ s:%X{shard}}%notEmpty{ r:%X{replica}}%notEmpty{ x:%X{core}}%notEmpty{ t:%X{trace_id}}] %c{1.} %m%notEmpty{ =>%ex{short}}}{10240}%n @@ -50,7 +50,7 @@ filePattern="${sys:solr.log.dir}/solr_slow_requests.log.%i" > - %maxLen{%d{yyyy-MM-dd HH:mm:ss.SSS} %-5p (%t) [%notEmpty{t:%X{trace_id}}%notEmpty{ c:%X{collection}}%notEmpty{ s:%X{shard}}%notEmpty{ r:%X{replica}}%notEmpty{ x:%X{core}}] %c{1.} %m%notEmpty{ =>%ex{short}}}{10240}%n + %maxLen{%d{yyyy-MM-dd HH:mm:ss.SSS} %-5p (%t) [%notEmpty{c:%X{collection}}%notEmpty{ s:%X{shard}}%notEmpty{ r:%X{replica}}%notEmpty{ x:%X{core}}%notEmpty{ t:%X{trace_id}}] %c{1.} %m%notEmpty{ =>%ex{short}}}{10240}%n diff --git a/solr/solr-ref-guide/modules/deployment-guide/pages/distributed-tracing.adoc b/solr/solr-ref-guide/modules/deployment-guide/pages/distributed-tracing.adoc index 18cac7c8695..92fe6c894a1 100644 --- a/solr/solr-ref-guide/modules/deployment-guide/pages/distributed-tracing.adoc +++ b/solr/solr-ref-guide/modules/deployment-guide/pages/distributed-tracing.adoc @@ -62,6 +62,8 @@ This module brings support for the industry standard https://opentelemetry.io[Op ---- +As an alternative to changing the `solr.xml` file, the `OTEL` tracer will be enabled if the system property `otel.service.name` or environment variable `OTEL_SERVICE_NAME` is present. The `opentelemetry` module still needs to be enabled for the tracer to work. + Enable the module with either system property `-Dsolr.modules=opentelemetry` or environment variable `SOLR_MODULES=opentelemetry`. === Configuration diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java b/solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java index fdc64237a80..3da6dd7a23e 100644 --- a/solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java +++ b/solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java @@ -1288,6 +1288,13 @@ public Builder withDefaultClusterProperty(String key, String value) { return this; } + /** + * Disables the default/built-in simple trace ID generation/propagation. + * + *

Tracers are registered as global singletons and if for example a test needs to use a + * MockTracer or a "real" Tracer, it needs to call this method so that the test setup doesn't + * accidentally reset the Tracer it wants to use. + */ public Builder withTraceIdGenerationDisabled() { this.disableTraceIdGeneration = true; return this;