From 3b348838232d4b1c7e076e80247e736d0a41b01a Mon Sep 17 00:00:00 2001 From: Alex Deparvu Date: Mon, 28 Aug 2023 12:23:11 -0700 Subject: [PATCH 1/7] SOLR-15367 [9.x] Convert "rid" functionality into a default Tracer --- .../src/java/org/apache/solr/api/Api.java | 2 +- .../apache/solr/core/TracerConfigurator.java | 6 + .../admin/api/RestoreCollectionAPI.java | 2 +- .../solr/util/tracing/SimplePropagator.java | 304 ++++++++++++++++++ .../apache/solr/util/tracing/TraceUtils.java | 3 +- ...estSimplePropagatorDistributedTracing.java | 118 +++++++ .../solr/jaeger/TestJaegerConfigurator.java | 1 + .../OtelTracerConfiguratorTest.java | 1 + solr/server/resources/log4j2.xml | 6 +- .../solr/cloud/MiniSolrCloudCluster.java | 13 + 10 files changed, 450 insertions(+), 6 deletions(-) create mode 100644 solr/core/src/java/org/apache/solr/util/tracing/SimplePropagator.java create mode 100644 solr/core/src/test/org/apache/solr/util/tracing/TestSimplePropagatorDistributedTracing.java diff --git a/solr/core/src/java/org/apache/solr/api/Api.java b/solr/core/src/java/org/apache/solr/api/Api.java index 54010912ae2..8e881b7f74e 100644 --- a/solr/core/src/java/org/apache/solr/api/Api.java +++ b/solr/core/src/java/org/apache/solr/api/Api.java @@ -41,7 +41,7 @@ public Map getCommandSchema() { if (commandSchema == null) { synchronized (this) { if (commandSchema == null) { - ValidatingJsonMap commands = getSpec().getMap("commands", null); + ValidatingJsonMap commands = spec != null ? getSpec().getMap("commands", null) : null; commandSchema = commands != null ? Map.copyOf(ApiBag.getParsedSchema(commands)) : Map.of(); } 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 ba36ed127ec..c7259e0b7be 100644 --- a/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java +++ b/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java @@ -25,6 +25,7 @@ import java.util.concurrent.atomic.AtomicReference; import org.apache.solr.common.util.ExecutorUtil; import org.apache.solr.util.plugin.NamedListInitializedPlugin; +import org.apache.solr.util.tracing.SimplePropagator; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -32,6 +33,9 @@ public abstract class TracerConfigurator implements NamedListInitializedPlugin { private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + public static final boolean TRACE_ID_GEN_ENABLED = + Boolean.parseBoolean(System.getProperty("solr.alwaysOnTraceId", "true")); + public abstract Tracer getTracer(); public static Tracer loadTracer(SolrResourceLoader loader, PluginInfo info) { @@ -48,6 +52,8 @@ public static Tracer loadTracer(SolrResourceLoader loader, PluginInfo info) { configurator.init(info.initArgs); return configurator.getTracer(); }); + } else if (TRACE_ID_GEN_ENABLED) { + SimplePropagator.load(); } if (GlobalTracer.isRegistered()) { // ideally we would furthermore check that it's not a no-op impl either but diff --git a/solr/core/src/java/org/apache/solr/handler/admin/api/RestoreCollectionAPI.java b/solr/core/src/java/org/apache/solr/handler/admin/api/RestoreCollectionAPI.java index 18caed1933a..1809d83f6b9 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/api/RestoreCollectionAPI.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/api/RestoreCollectionAPI.java @@ -108,8 +108,8 @@ public SubResponseAccumulatingJerseyResponse restoreCollection( } final String collectionName = requestBody.collection; - recordCollectionForLogAndTracing(collectionName, solrQueryRequest); SolrIdentifierValidator.validateCollectionName(collectionName); + recordCollectionForLogAndTracing(collectionName, solrQueryRequest); if (coreContainer.getAliases().hasAlias(collectionName)) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, 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 new file mode 100644 index 00000000000..bbbf5526a42 --- /dev/null +++ b/solr/core/src/java/org/apache/solr/util/tracing/SimplePropagator.java @@ -0,0 +1,304 @@ +/* + * 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.util.tracing; + +import io.opentracing.Scope; +import io.opentracing.ScopeManager; +import io.opentracing.Span; +import io.opentracing.SpanContext; +import io.opentracing.Tracer; +import io.opentracing.Tracer.SpanBuilder; +import io.opentracing.propagation.Format; +import io.opentracing.propagation.TextMap; +import io.opentracing.tag.Tag; +import io.opentracing.util.GlobalTracer; +import io.opentracing.util.ThreadLocalScopeManager; +import java.lang.invoke.MethodHandles; +import java.util.Collections; +import java.util.Iterator; +import java.util.Map; +import java.util.Map.Entry; +import java.util.concurrent.atomic.AtomicLong; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class SimplePropagator { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + + private static final String TRACE_HOST_NAME = + System.getProperty("trace_host_name", System.getProperty("host")); + + static final String TRACE_ID = System.getProperty("TRACE_ID", "X-Trace-Id"); + + private static final AtomicLong traceCounter = new AtomicLong(0); + + public static void load() { + GlobalTracer.registerIfAbsent( + () -> { + log.info("Always-on trace id generation enabled."); + return new SimplePropagatorTracer(); + }); + } + + /** + * Tracer that only aims to do simple header propagation, tailored to how Solr works. + * + *

Heavily inspired from JaegerTracer, NoopTracer + */ + static class SimplePropagatorTracer implements Tracer { + + private final ScopeManager scopeManager = new ThreadLocalScopeManager(); + + @Override + public ScopeManager scopeManager() { + return scopeManager; + } + + @Override + public Span activeSpan() { + return scopeManager.activeSpan(); + } + + @Override + public Scope activateSpan(Span span) { + return scopeManager.activate(span); + } + + @Override + public SpanBuilder buildSpan(String operationName) { + return new SimplePropagatorSpanBuilder(scopeManager); + } + + @Override + public void close() {} + + @Override + public void inject(SpanContext spanContext, Format format, C carrier) { + if (!format.equals(Format.Builtin.HTTP_HEADERS)) { + throw new IllegalArgumentException("unsupported format " + format); + } + String traceId = spanContext.toTraceId(); + if (traceId != null && !traceId.isEmpty()) { + TextMap tm = (TextMap) carrier; + tm.put(TRACE_ID, traceId); + } + } + + @Override + public SpanContext extract(Format format, C carrier) { + if (!format.equals(Format.Builtin.HTTP_HEADERS)) { + throw new IllegalArgumentException("unsupported format " + format); + } + String traceId = null; + TextMap tm = (TextMap) carrier; + Iterator> it = tm.iterator(); + while (it.hasNext()) { + var e = it.next(); + // 'equalsIgnoreCase' because header name might be lowercase + if (e.getKey().equalsIgnoreCase(TRACE_ID)) { + traceId = e.getValue(); + break; + } + } + if (traceId == null) { + traceId = newTraceId(); + } + return new SimplePropagatorSpanContext(traceId); + } + + @Override + public String toString() { + return "SimplePropagator"; + } + } + + private static final class SimplePropagatorSpanBuilder implements SpanBuilder { + + private final ScopeManager scopeManager; + // storing parent to support the parent being injected via the asChildOf method + private SpanContext parent; + + public SimplePropagatorSpanBuilder(ScopeManager scopeManager) { + this.scopeManager = scopeManager; + } + + @Override + public Span start() { + if (parent != null) { + return new SimplePropagatorSpan(parent); + } else if (scopeManager.activeSpan() != null) { + return new SimplePropagatorSpan(scopeManager.activeSpan().context()); + } else { + return new SimplePropagatorSpan(new SimplePropagatorSpanContext("")); + } + } + + @Override + public SpanBuilder addReference(String referenceType, SpanContext reference) { + return this; + } + + @Override + public SpanBuilder asChildOf(SpanContext parent) { + this.parent = parent; + return this; + } + + @Override + public SpanBuilder asChildOf(Span parent) { + return this; + } + + @Override + public SpanBuilder ignoreActiveSpan() { + return this; + } + + @Override + public SpanBuilder withStartTimestamp(long arg0) { + return this; + } + + @Override + public SpanBuilder withTag(String arg0, String arg1) { + return this; + } + + @Override + public SpanBuilder withTag(String arg0, boolean arg1) { + return this; + } + + @Override + public SpanBuilder withTag(String arg0, Number arg1) { + return this; + } + + @Override + public SpanBuilder withTag(Tag arg0, T arg1) { + return this; + } + } + + private static final class SimplePropagatorSpanContext implements SpanContext { + + private final String traceId; + + public SimplePropagatorSpanContext(String traceId) { + this.traceId = traceId; + } + + @Override + public String toTraceId() { + return traceId; + } + + @Override + public String toSpanId() { + return ""; + } + + @Override + public Iterable> baggageItems() { + return Collections.emptyList(); + } + + @Override + public String toString() { + return "SimplePropagatorSpanContext [traceId=" + traceId + "]"; + } + } + + static final class SimplePropagatorSpan implements Span { + + private final SpanContext ctx; + + private SimplePropagatorSpan(SpanContext ctx) { + this.ctx = ctx; + } + + @Override + public SpanContext context() { + return ctx; + } + + @Override + public void finish() {} + + @Override + public void finish(long arg0) {} + + @Override + public String getBaggageItem(String arg0) { + return null; + } + + @Override + public Span log(Map arg0) { + return this; + } + + @Override + public Span log(String arg0) { + return this; + } + + @Override + public Span log(long arg0, Map arg1) { + return this; + } + + @Override + public Span log(long arg0, String arg1) { + return this; + } + + @Override + public Span setBaggageItem(String arg0, String arg1) { + return this; + } + + @Override + public Span setOperationName(String arg0) { + return this; + } + + @Override + public Span setTag(String arg0, String arg1) { + return this; + } + + @Override + public Span setTag(String arg0, boolean arg1) { + return this; + } + + @Override + public Span setTag(String arg0, Number arg1) { + return this; + } + + @Override + public Span setTag(Tag arg0, T arg1) { + return this; + } + } + + private static String newTraceId() { + return TRACE_HOST_NAME + "-" + traceCounter.incrementAndGet(); + } +} 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 069da747a34..8168e8607d0 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,6 +22,7 @@ import java.util.List; import java.util.function.Consumer; import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.util.tracing.SimplePropagator.SimplePropagatorSpan; /** Utilities for distributed tracing. */ public class TraceUtils { @@ -33,7 +34,7 @@ public static void setDbInstance(SolrQueryRequest req, String coreOrColl) { } public static void ifNotNoop(Span span, Consumer consumer) { - if (span != null && !(span instanceof NoopSpan)) { + if (span != null && !(span instanceof NoopSpan) && !(span instanceof SimplePropagatorSpan)) { consumer.accept(span); } } diff --git a/solr/core/src/test/org/apache/solr/util/tracing/TestSimplePropagatorDistributedTracing.java b/solr/core/src/test/org/apache/solr/util/tracing/TestSimplePropagatorDistributedTracing.java new file mode 100644 index 00000000000..a08d3ade5b1 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/util/tracing/TestSimplePropagatorDistributedTracing.java @@ -0,0 +1,118 @@ +/* + * 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.util.tracing; + +import io.opentracing.util.GlobalTracer; +import java.io.IOException; +import java.util.concurrent.TimeUnit; +import org.apache.solr.client.solrj.SolrQuery; +import org.apache.solr.client.solrj.SolrServerException; +import org.apache.solr.client.solrj.impl.CloudSolrClient; +import org.apache.solr.client.solrj.request.CollectionAdminRequest; +import org.apache.solr.client.solrj.request.QueryRequest; +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.util.SuppressForbidden; +import org.apache.solr.common.util.TimeSource; +import org.apache.solr.core.SolrCore; +import org.apache.solr.logging.MDCLoggingContext; +import org.apache.solr.update.processor.LogUpdateProcessorFactory; +import org.apache.solr.util.LogListener; +import org.apache.solr.util.TimeOut; +import org.junit.BeforeClass; +import org.junit.Test; + +@SuppressForbidden(reason = "We need to use log4J2 classes directly to test MDC impacts") +public class TestSimplePropagatorDistributedTracing extends SolrCloudTestCase { + + private static final String COLLECTION = "collection1"; + + @BeforeClass + public static void setupCluster() throws Exception { + + configureCluster(4).addConfig("conf", configset("cloud-minimal")).configure(); + + TimeOut timeOut = new TimeOut(30, TimeUnit.SECONDS, TimeSource.NANO_TIME); + timeOut.waitFor( + "Waiting for GlobalTracer is registered", + () -> GlobalTracer.get().toString().contains("SimplePropagator")); + + CollectionAdminRequest.createCollection(COLLECTION, "conf", 2, 2) + .process(cluster.getSolrClient()); + cluster.waitForActiveCollection(COLLECTION, 2, 4); + } + + @Test + public void test() throws IOException, SolrServerException { + CloudSolrClient cloudClient = cluster.getSolrClient(); + + // Indexing has trace ids + try (LogListener reqLog = LogListener.info(LogUpdateProcessorFactory.class.getName())) { + // verify all indexing events have trace id present + cloudClient.add(COLLECTION, sdoc("id", "1")); + cloudClient.add(COLLECTION, sdoc("id", "2")); + cloudClient.add(COLLECTION, sdoc("id", "3")); + var queue = reqLog.getQueue(); + assertFalse(queue.isEmpty()); + while (!queue.isEmpty()) { + var reqEvent = queue.poll(); + String evTraceId = reqEvent.getContextData().getValue(MDCLoggingContext.TRACE_ID); + assertNotNull(evTraceId); + } + + // TODO this doesn't work due to solr client creating the UpdateRequest without headers + // // verify all events have the same 'custom' traceid + // String traceId = "tidTestSimplePropagatorDistributedTracing0"; + // var doc = sdoc("id", "4"); + // UpdateRequest u = new UpdateRequest(); + // u.add(doc); + // u.addHeader(SimplePropagator.TRACE_ID, traceId); + // var r1 = u.process(cloudClient, COLLECTION); + // assertEquals(0, r1.getStatus()); + // assertSameTraceId(reqLog, traceId); + } + + // Searching has trace ids + try (LogListener reqLog = LogListener.info(SolrCore.class.getName() + ".Request")) { + // verify all query events have the same auto-generated traceid + var r1 = cloudClient.query(COLLECTION, new SolrQuery("*:*")); + assertEquals(0, r1.getStatus()); + assertSameTraceId(reqLog, null); + + // verify all query events have the same 'custom' traceid + String traceId = "tidTestSimplePropagatorDistributedTracing1"; + var q = new QueryRequest(new SolrQuery("*:*")); + q.addHeader(SimplePropagator.TRACE_ID, traceId); + var r2 = q.process(cloudClient, COLLECTION); + assertEquals(0, r2.getStatus()); + assertSameTraceId(reqLog, traceId); + } + } + + private void assertSameTraceId(LogListener reqLog, String traceId) { + var queue = reqLog.getQueue(); + assertFalse(queue.isEmpty()); + if (traceId == null) { + traceId = queue.poll().getContextData().getValue(MDCLoggingContext.TRACE_ID); + assertNotNull(traceId); + } + while (!queue.isEmpty()) { + var reqEvent = queue.poll(); + String evTraceId = reqEvent.getContextData().getValue(MDCLoggingContext.TRACE_ID); + assertEquals(traceId, evTraceId); + } + } +} diff --git a/solr/modules/jaegertracer-configurator/src/test/org/apache/solr/jaeger/TestJaegerConfigurator.java b/solr/modules/jaegertracer-configurator/src/test/org/apache/solr/jaeger/TestJaegerConfigurator.java index 4759c5ced19..0291b46ff0b 100644 --- a/solr/modules/jaegertracer-configurator/src/test/org/apache/solr/jaeger/TestJaegerConfigurator.java +++ b/solr/modules/jaegertracer-configurator/src/test/org/apache/solr/jaeger/TestJaegerConfigurator.java @@ -49,6 +49,7 @@ public void testInjected() throws Exception { new MiniSolrCloudCluster.Builder(2, createTempDir()) .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf")) .withSolrXml(getFile("solr/solr.xml").toPath()) + .withTraceIdGenerationDisabled() .build(); try { TimeOut timeOut = new TimeOut(2, TimeUnit.MINUTES, TimeSource.NANO_TIME); 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 5aef12103da..c7cda9fbb7a 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 @@ -103,6 +103,7 @@ public void testInjected() throws Exception { new MiniSolrCloudCluster.Builder(2, createTempDir()) .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf")) .withSolrXml(getFile("solr/solr.xml").toPath()) + .withTraceIdGenerationDisabled() .build(); try { assertTrue( diff --git a/solr/server/resources/log4j2.xml b/solr/server/resources/log4j2.xml index 186e86bd8cc..752c740514a 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{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{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 @@ -34,7 +34,7 @@ filePattern="${sys:solr.log.dir}/solr.log.%i" > - %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}}] %c{1.} %m%notEmpty{ =>%ex{short}}}{10240}%n + %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 @@ -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{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{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 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 704cbfa8589..a7e55345093 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 @@ -78,9 +78,11 @@ import org.apache.solr.common.util.SolrNamedThreadFactory; import org.apache.solr.common.util.TimeSource; import org.apache.solr.core.CoreContainer; +import org.apache.solr.core.TracerConfigurator; import org.apache.solr.embedded.JettyConfig; import org.apache.solr.embedded.JettySolrRunner; import org.apache.solr.util.TimeOut; +import org.apache.solr.util.tracing.SimplePropagator; import org.apache.zookeeper.KeeperException; import org.eclipse.jetty.server.handler.HandlerWrapper; import org.eclipse.jetty.servlet.ServletHolder; @@ -1106,6 +1108,7 @@ public static class Builder { private boolean useDistributedCollectionConfigSetExecution; private boolean useDistributedClusterStateUpdate; private boolean formatZkServer = true; + private boolean disableTraceIdGeneration = false; /** * Create a builder @@ -1299,6 +1302,11 @@ public MiniSolrCloudCluster build() throws Exception { "solr.distributedClusterStateUpdates", Boolean.toString(useDistributedClusterStateUpdate)); + // eager init to prevent OTEL init races caused by test setup + if (!disableTraceIdGeneration && TracerConfigurator.TRACE_ID_GEN_ENABLED) { + SimplePropagator.load(); + } + JettyConfig jettyConfig = jettyConfigBuilder.build(); MiniSolrCloudCluster cluster = new MiniSolrCloudCluster( @@ -1341,5 +1349,10 @@ public Builder withDefaultClusterProperty(String key, String value) { cluster.put(key, value); return this; } + + public Builder withTraceIdGenerationDisabled() { + this.disableTraceIdGeneration = true; + return this; + } } } From b6cf6098721ddb18090680c557dc72b6fa80a911 Mon Sep 17 00:00:00 2001 From: Alex Deparvu Date: Thu, 31 Aug 2023 11:01:00 -0700 Subject: [PATCH 2/7] PR feedback --- .../solr/util/tracing/SimplePropagator.java | 48 ++++++++----------- .../apache/solr/util/tracing/TraceUtils.java | 3 +- 2 files changed, 22 insertions(+), 29 deletions(-) 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 bbbf5526a42..b3607b9b67b 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 @@ -22,6 +22,7 @@ import io.opentracing.SpanContext; import io.opentracing.Tracer; import io.opentracing.Tracer.SpanBuilder; +import io.opentracing.noop.NoopSpan; import io.opentracing.propagation.Format; import io.opentracing.propagation.TextMap; import io.opentracing.tag.Tag; @@ -54,6 +55,10 @@ public static void load() { }); } + private static String newTraceId() { + return TRACE_HOST_NAME + "-" + traceCounter.incrementAndGet(); + } + /** * Tracer that only aims to do simple header propagation, tailored to how Solr works. * @@ -89,7 +94,8 @@ public void close() {} @Override public void inject(SpanContext spanContext, Format format, C carrier) { if (!format.equals(Format.Builtin.HTTP_HEADERS)) { - throw new IllegalArgumentException("unsupported format " + format); + // unsupported via the Solr injectors + return; } String traceId = spanContext.toTraceId(); if (traceId != null && !traceId.isEmpty()) { @@ -101,8 +107,10 @@ public void inject(SpanContext spanContext, Format format, C carrier) { @Override public SpanContext extract(Format format, C carrier) { if (!format.equals(Format.Builtin.HTTP_HEADERS)) { - throw new IllegalArgumentException("unsupported format " + format); + // unsupported via the Solr injectors + return NoopSpan.INSTANCE.context(); } + String traceId = null; TextMap tm = (TextMap) carrier; Iterator> it = tm.iterator(); @@ -117,7 +125,7 @@ public SpanContext extract(Format format, C carrier) { if (traceId == null) { traceId = newTraceId(); } - return new SimplePropagatorSpanContext(traceId); + return new SimplePropagatorSpan(traceId); } @Override @@ -139,11 +147,15 @@ public SimplePropagatorSpanBuilder(ScopeManager scopeManager) { @Override public Span start() { if (parent != null) { - return new SimplePropagatorSpan(parent); + if (parent instanceof SimplePropagatorSpan) { + return (SimplePropagatorSpan) parent; + } else { + return NoopSpan.INSTANCE; + } } else if (scopeManager.activeSpan() != null) { - return new SimplePropagatorSpan(scopeManager.activeSpan().context()); + return scopeManager.activeSpan(); } else { - return new SimplePropagatorSpan(new SimplePropagatorSpanContext("")); + return new SimplePropagatorSpan(newTraceId()); } } @@ -194,11 +206,11 @@ public SpanBuilder withTag(Tag arg0, T arg1) { } } - private static final class SimplePropagatorSpanContext implements SpanContext { + private static final class SimplePropagatorSpan implements Span, SpanContext, NoopSpan { private final String traceId; - public SimplePropagatorSpanContext(String traceId) { + private SimplePropagatorSpan(String traceId) { this.traceId = traceId; } @@ -217,23 +229,9 @@ public Iterable> baggageItems() { return Collections.emptyList(); } - @Override - public String toString() { - return "SimplePropagatorSpanContext [traceId=" + traceId + "]"; - } - } - - static final class SimplePropagatorSpan implements Span { - - private final SpanContext ctx; - - private SimplePropagatorSpan(SpanContext ctx) { - this.ctx = ctx; - } - @Override public SpanContext context() { - return ctx; + return this; } @Override @@ -297,8 +295,4 @@ public Span setTag(Tag arg0, T arg1) { return this; } } - - private static String newTraceId() { - return TRACE_HOST_NAME + "-" + traceCounter.incrementAndGet(); - } } 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 8168e8607d0..069da747a34 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 java.util.List; import java.util.function.Consumer; import org.apache.solr.request.SolrQueryRequest; -import org.apache.solr.util.tracing.SimplePropagator.SimplePropagatorSpan; /** Utilities for distributed tracing. */ public class TraceUtils { @@ -34,7 +33,7 @@ public static void setDbInstance(SolrQueryRequest req, String coreOrColl) { } public static void ifNotNoop(Span span, Consumer consumer) { - if (span != null && !(span instanceof NoopSpan) && !(span instanceof SimplePropagatorSpan)) { + if (span != null && !(span instanceof NoopSpan)) { consumer.accept(span); } } From 89f267edcb3812c9e3628bfed1bf81cb5802a274 Mon Sep 17 00:00:00 2001 From: Alex Deparvu Date: Thu, 31 Aug 2023 13:36:17 -0700 Subject: [PATCH 3/7] PR feedback 2 --- .../java/org/apache/solr/util/tracing/SimplePropagator.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 b3607b9b67b..f2141f5aa63 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 @@ -152,8 +152,10 @@ public Span start() { } else { return NoopSpan.INSTANCE; } - } else if (scopeManager.activeSpan() != null) { - return scopeManager.activeSpan(); + } + var activeSpan = scopeManager.activeSpan(); + if (activeSpan != null) { + return activeSpan; } else { return new SimplePropagatorSpan(newTraceId()); } From 18ee8dc7bfbfd91c5cec501f96c051601652aacf Mon Sep 17 00:00:00 2001 From: Alex Deparvu Date: Fri, 1 Sep 2023 09:15:02 -0700 Subject: [PATCH 4/7] fixed log4j config --- solr/server/resources/log4j2.xml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 From 3eb477e31d0b9840670a0a84b918648f93a3badc Mon Sep 17 00:00:00 2001 From: Alex Deparvu Date: Wed, 6 Sep 2023 19:04:56 -0700 Subject: [PATCH 5/7] updated the distributed test to disable always on tracer --- .../org/apache/solr/util/tracing/TestDistributedTracing.java | 1 + 1 file changed, 1 insertion(+) diff --git a/solr/core/src/test/org/apache/solr/util/tracing/TestDistributedTracing.java b/solr/core/src/test/org/apache/solr/util/tracing/TestDistributedTracing.java index b28f15e18bd..8fe52fa515f 100644 --- a/solr/core/src/test/org/apache/solr/util/tracing/TestDistributedTracing.java +++ b/solr/core/src/test/org/apache/solr/util/tracing/TestDistributedTracing.java @@ -55,6 +55,7 @@ public static void beforeTest() throws Exception { configureCluster(4) .addConfig( "config", TEST_PATH().resolve("configsets").resolve("cloud-minimal").resolve("conf")) + .withTraceIdGenerationDisabled() .configure(); CollectionAdminRequest.createCollection(COLLECTION, "config", 2, 2) .setPerReplicaState(SolrCloudTestCase.USE_PER_REPLICA_STATE) From 9168ebf985484528fe1d8b9d1918be6682d072f3 Mon Sep 17 00:00:00 2001 From: Alex Deparvu Date: Wed, 6 Sep 2023 21:10:49 -0700 Subject: [PATCH 6/7] disabled default tracer --- .../solr/util/tracing/BasicAuthIntegrationTracingTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/solr/core/src/test/org/apache/solr/util/tracing/BasicAuthIntegrationTracingTest.java b/solr/core/src/test/org/apache/solr/util/tracing/BasicAuthIntegrationTracingTest.java index 2313cd808d7..0fd7a7cf77b 100644 --- a/solr/core/src/test/org/apache/solr/util/tracing/BasicAuthIntegrationTracingTest.java +++ b/solr/core/src/test/org/apache/solr/util/tracing/BasicAuthIntegrationTracingTest.java @@ -77,6 +77,7 @@ public static void beforeTest() throws Exception { .addConfig( "config", TEST_PATH().resolve("configsets").resolve("cloud-minimal").resolve("conf")) .withSecurityJson(SECURITY_JSON) + .withTraceIdGenerationDisabled() .configure(); CollectionAdminRequest.createCollection(COLLECTION, "config", 2, 2) .setPerReplicaState(SolrCloudTestCase.USE_PER_REPLICA_STATE) From 5b4b6ac42385b68b866feb9be8f9cc1cde381b21 Mon Sep 17 00:00:00 2001 From: Alex Deparvu Date: Thu, 7 Sep 2023 05:25:52 -0700 Subject: [PATCH 7/7] javadocs --- .../java/org/apache/solr/cloud/MiniSolrCloudCluster.java | 7 +++++++ 1 file changed, 7 insertions(+) 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 a7e55345093..6ebd4400ecf 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 @@ -1350,6 +1350,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;