You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by ds...@apache.org on 2023/10/12 22:20:03 UTC
[solr] branch main updated: SOLR-16943: Tracing: more thoroughly instrument Solr-to-Solr communication (#1856)
This is an automated email from the ASF dual-hosted git repository.
dsmiley pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/main by this push:
new 4e4c42c3303 SOLR-16943: Tracing: more thoroughly instrument Solr-to-Solr communication (#1856)
4e4c42c3303 is described below
commit 4e4c42c3303fe574466dad7d0408a75b7bcf4f1d
Author: David Smiley <ds...@salesforce.com>
AuthorDate: Thu Oct 12 18:19:55 2023 -0400
SOLR-16943: Tracing: more thoroughly instrument Solr-to-Solr communication (#1856)
* Move Jetty HttpClient tracing down into InstrumentedHttpListenerFactory from other places, thus covering more scenarios.
* Add Apache HttpClient tracing via InstrumentedHttpRequestExecutor.
* None of this affects SolrJ because these "Instrumented" things are only in solr-core.
* Reinstate Solr ExecutorService instrumentation, i.e. propagation to other threads in Solr. That was in 9x and briefly was torn out in main.
Co-authored-by: Alex Deparvu <st...@apache.org>
---
solr/CHANGES.txt | 3 +-
.../org/apache/solr/core/TracerConfigurator.java | 29 ++++++++++++++
.../solr/handler/component/HttpShardHandler.java | 2 -
.../org/apache/solr/update/SolrCmdDistributor.java | 2 -
.../stats/InstrumentedHttpListenerFactory.java | 20 +++++++++-
.../stats/InstrumentedHttpRequestExecutor.java | 2 +
.../solr/util/tracing/SolrRequestSetter.java | 32 ---------------
.../org/apache/solr/util/tracing/TraceUtils.java | 19 +++++++--
.../solr/opentelemetry/TestDistributedTracing.java | 46 ++++++++++++++++------
9 files changed, 102 insertions(+), 53 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 638316542b0..525a2fad720 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -8,7 +8,8 @@ https://github.com/apache/solr/blob/main/solr/solr-ref-guide/modules/upgrade-not
New Features
---------------------
-(No changes)
+
+* SOLR-16943: Extend Solr client tracing coverage to both Jetty Client and Apache HttpClient (Alex Deparvu, David Smiley)
Improvements
---------------------
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 47e5d32cb7b..54d718b9ca2 100644
--- a/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java
+++ b/solr/core/src/java/org/apache/solr/core/TracerConfigurator.java
@@ -18,10 +18,14 @@
package org.apache.solr.core;
import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.Scope;
import java.lang.invoke.MethodHandles;
import java.util.Locale;
import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.ExecutorUtil;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.util.plugin.NamedListInitializedPlugin;
import org.apache.solr.util.tracing.SimplePropagator;
@@ -46,12 +50,14 @@ public abstract class TracerConfigurator implements NamedListInitializedPlugin {
TracerConfigurator configurator =
loader.newInstance(info.className, TracerConfigurator.class);
configurator.init(info.initArgs);
+ ExecutorUtil.addThreadLocalProvider(new ContextThreadLocalProvider());
return configurator.getTracer();
}
if (shouldAutoConfigOTEL()) {
return autoConfigOTEL(loader);
}
if (TRACE_ID_GEN_ENABLED) {
+ ExecutorUtil.addThreadLocalProvider(new ContextThreadLocalProvider());
return SimplePropagator.load();
}
return TraceUtils.getGlobalTracer();
@@ -59,11 +65,34 @@ public abstract class TracerConfigurator implements NamedListInitializedPlugin {
protected abstract Tracer getTracer();
+ private static class ContextThreadLocalProvider
+ implements ExecutorUtil.InheritableThreadLocalProvider {
+
+ @Override
+ public void store(AtomicReference<Object> ctx) {
+ ctx.set(Context.current());
+ }
+
+ @Override
+ public void set(AtomicReference<Object> ctx) {
+ var traceContext = (Context) ctx.get();
+ var scope = traceContext.makeCurrent();
+ ctx.set(scope);
+ }
+
+ @Override
+ public void clean(AtomicReference<Object> ctx) {
+ var scope = (Scope) ctx.get();
+ scope.close();
+ }
+ }
+
private static Tracer autoConfigOTEL(SolrResourceLoader loader) {
try {
TracerConfigurator configurator =
loader.newInstance(DEFAULT_CLASS_NAME, TracerConfigurator.class);
configurator.init(new NamedList<>());
+ ExecutorUtil.addThreadLocalProvider(new ContextThreadLocalProvider());
return configurator.getTracer();
} catch (SolrException e) {
log.error(
diff --git a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java
index 17fb78a86c6..9ee84409290 100644
--- a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java
+++ b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java
@@ -47,7 +47,6 @@ import org.apache.solr.core.CoreDescriptor;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.request.SolrRequestInfo;
import org.apache.solr.security.AllowListUrlChecker;
-import org.apache.solr.util.tracing.TraceUtils;
@NotThreadSafe
public class HttpShardHandler extends ShardHandler {
@@ -166,7 +165,6 @@ public class HttpShardHandler extends ShardHandler {
@Override
public void onStart() {
- TraceUtils.injectContextIntoRequest(req);
SolrRequestInfo requestInfo = SolrRequestInfo.getRequestInfo();
if (requestInfo != null)
req.setUserPrincipal(requestInfo.getReq().getUserPrincipal());
diff --git a/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java b/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java
index b40c79a166f..d31ddc9ca55 100644
--- a/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java
+++ b/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java
@@ -49,7 +49,6 @@ import org.apache.solr.request.SolrRequestInfo;
import org.apache.solr.update.processor.DistributedUpdateProcessor;
import org.apache.solr.update.processor.DistributedUpdateProcessor.LeaderRequestReplicationTracker;
import org.apache.solr.update.processor.DistributedUpdateProcessor.RollupRequestReplicationTracker;
-import org.apache.solr.util.tracing.TraceUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -311,7 +310,6 @@ public class SolrCmdDistributor implements Closeable {
if (SolrRequestInfo.getRequestInfo() != null) {
req.uReq.setUserPrincipal(SolrRequestInfo.getRequestInfo().getReq().getUserPrincipal());
}
- TraceUtils.injectContextIntoRequest(req.uReq);
if (req.synchronous) {
blockAndDoRetries();
diff --git a/solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpListenerFactory.java b/solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpListenerFactory.java
index 0e90657f7d2..31523a5882f 100644
--- a/solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpListenerFactory.java
+++ b/solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpListenerFactory.java
@@ -20,18 +20,20 @@ package org.apache.solr.util.stats;
import static org.apache.solr.metrics.SolrMetricManager.mkName;
import com.codahale.metrics.Timer;
+import io.opentelemetry.api.trace.Span;
import java.util.Locale;
import java.util.Map;
import org.apache.solr.client.solrj.impl.HttpListenerFactory;
import org.apache.solr.common.util.CollectionUtil;
import org.apache.solr.metrics.SolrMetricProducer;
import org.apache.solr.metrics.SolrMetricsContext;
+import org.apache.solr.util.tracing.TraceUtils;
import org.eclipse.jetty.client.api.Request;
import org.eclipse.jetty.client.api.Result;
/**
- * A HttpListenerFactory tracks metrics interesting to solr Inspired and partially copied from
- * dropwizard httpclient library
+ * A HttpListenerFactory tracking metrics and distributed tracing. The Metrics are inspired and
+ * partially copied from dropwizard httpclient library.
*/
public class InstrumentedHttpListenerFactory implements SolrMetricProducer, HttpListenerFactory {
@@ -86,12 +88,23 @@ public class InstrumentedHttpListenerFactory implements SolrMetricProducer, Http
public RequestResponseListener get() {
return new RequestResponseListener() {
Timer.Context timerContext;
+ Span span = Span.getInvalid();
+
+ @Override
+ public void onQueued(Request request) {
+ // do tracing onQueued because it's called from Solr's thread
+ span = Span.current();
+ TraceUtils.injectTraceContext(request);
+ }
@Override
public void onBegin(Request request) {
if (solrMetricsContext != null) {
timerContext = timer(request).time();
}
+ if (span.isRecording()) {
+ span.addEvent("Client Send"); // perhaps delayed a bit after the span started in enqueue
+ }
}
@Override
@@ -99,6 +112,9 @@ public class InstrumentedHttpListenerFactory implements SolrMetricProducer, Http
if (timerContext != null) {
timerContext.stop();
}
+ if (result.isFailed() && span.isRecording()) {
+ span.addEvent(result.toString()); // logs failure info and interesting stuff
+ }
}
};
}
diff --git a/solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpRequestExecutor.java b/solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpRequestExecutor.java
index c796fe8f57c..b50751a1ce0 100644
--- a/solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpRequestExecutor.java
+++ b/solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpRequestExecutor.java
@@ -36,6 +36,7 @@ import org.apache.http.protocol.HttpRequestExecutor;
import org.apache.solr.common.util.CollectionUtil;
import org.apache.solr.metrics.SolrMetricProducer;
import org.apache.solr.metrics.SolrMetricsContext;
+import org.apache.solr.util.tracing.TraceUtils;
/**
* Sub-class of HttpRequestExecutor which tracks metrics interesting to solr Inspired and partially
@@ -138,6 +139,7 @@ public class InstrumentedHttpRequestExecutor extends HttpRequestExecutor
if (solrMetricsContext != null) {
timerContext = timer(request).time();
}
+ TraceUtils.injectTraceContext(request);
try {
return super.execute(request, conn, context);
} finally {
diff --git a/solr/core/src/java/org/apache/solr/util/tracing/SolrRequestSetter.java b/solr/core/src/java/org/apache/solr/util/tracing/SolrRequestSetter.java
deleted file mode 100644
index 0923d9db014..00000000000
--- a/solr/core/src/java/org/apache/solr/util/tracing/SolrRequestSetter.java
+++ /dev/null
@@ -1,32 +0,0 @@
-/*
- * 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.opentelemetry.context.propagation.TextMapSetter;
-import org.apache.solr.client.solrj.SolrRequest;
-
-/**
- * {@code SolrRequest} aware {@code TextMapSetter} that allows header data to be added to a request
- */
-public class SolrRequestSetter implements TextMapSetter<SolrRequest<?>> {
-
- @Override
- public void set(SolrRequest<?> request, String key, String value) {
- request.addHeader(key, value);
- }
-}
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 7edfe8fc55f..a153ae82bb6 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
@@ -25,12 +25,14 @@ import io.opentelemetry.api.trace.TraceId;
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapPropagator;
+import io.opentelemetry.context.propagation.TextMapSetter;
import java.util.List;
import java.util.function.Consumer;
import java.util.function.Predicate;
import javax.servlet.http.HttpServletRequest;
-import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.http.HttpRequest;
import org.apache.solr.request.SolrQueryRequest;
+import org.eclipse.jetty.client.api.Request;
/** Utilities for distributed tracing. */
public class TraceUtils {
@@ -144,9 +146,20 @@ public class TraceUtils {
return textMapPropagator.extract(Context.current(), req, new HttpServletRequestGetter());
}
- public static void injectContextIntoRequest(SolrRequest<?> req) {
+ private static final TextMapSetter<Request> REQUEST_INJECTOR =
+ (req, k, v) -> req.headers(httpFields -> httpFields.put(k, v));
+
+ public static void injectTraceContext(Request req) {
+ TextMapPropagator textMapPropagator = getTextMapPropagator();
+ textMapPropagator.inject(Context.current(), req, REQUEST_INJECTOR);
+ }
+
+ private static final TextMapSetter<HttpRequest> HTTP_REQUEST_INJECTOR =
+ (req, k, v) -> req.setHeader(k, v);
+
+ public static void injectTraceContext(HttpRequest req) {
TextMapPropagator textMapPropagator = getTextMapPropagator();
- textMapPropagator.inject(Context.current(), req, new SolrRequestSetter());
+ textMapPropagator.inject(Context.current(), req, HTTP_REQUEST_INJECTOR);
}
public static Span startHttpRequestSpan(HttpServletRequest request, Context context) {
diff --git a/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java b/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java
index 23cf76f1cbf..ab362068e2e 100644
--- a/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java
+++ b/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java
@@ -18,7 +18,6 @@
package org.apache.solr.opentelemetry;
import io.opentelemetry.api.GlobalOpenTelemetry;
-import io.opentelemetry.api.trace.SpanContext;
import io.opentelemetry.api.trace.TracerProvider;
import io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter;
import io.opentelemetry.sdk.trace.data.SpanData;
@@ -26,7 +25,6 @@ import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
-import java.util.stream.Collectors;
import org.apache.solr.client.solrj.SolrQuery;
import org.apache.solr.client.solrj.SolrRequest;
import org.apache.solr.client.solrj.SolrServerException;
@@ -34,6 +32,7 @@ import org.apache.solr.client.solrj.impl.CloudSolrClient;
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
import org.apache.solr.client.solrj.request.GenericSolrRequest;
import org.apache.solr.client.solrj.request.V2Request;
+import org.apache.solr.client.solrj.response.CollectionAdminResponse;
import org.apache.solr.client.solrj.response.V2Response;
import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.SolrDocumentList;
@@ -108,14 +107,7 @@ public class TestDistributedTracing extends SolrCloudTestCase {
|| !span.getAttributes().get(TraceUtils.TAG_HTTP_URL).endsWith("/select"));
// one from client to server, 2 for execute query, 2 for fetching documents
assertEquals(5, finishedSpans.size());
- assertEquals(1, finishedSpans.stream().filter(TestDistributedTracing::isRootSpan).count());
- var parentTraceId =
- finishedSpans.stream()
- .filter(TestDistributedTracing::isRootSpan)
- .collect(Collectors.toList())
- .get(0)
- .getSpanContext()
- .getTraceId();
+ var parentTraceId = getRootTraceId(finishedSpans);
for (var span : finishedSpans) {
if (isRootSpan(span)) {
continue;
@@ -177,8 +169,30 @@ public class TestDistributedTracing extends SolrCloudTestCase {
assertEquals(1, ((SolrDocumentList) v2Response.getResponse().get("response")).getNumFound());
}
+ /**
+ * Best effort test of the apache http client tracing. the test assumes the request uses the http
+ * client but there is no way to enforce it, so when the api will be rewritten this test will
+ * become obsolete
+ */
+ @Test
+ public void testApacheClient() throws Exception {
+ getAndClearSpans(); // reset
+ CollectionAdminRequest.ColStatus a1 = CollectionAdminRequest.collectionStatus(COLLECTION);
+ CollectionAdminResponse r1 = a1.process(cluster.getSolrClient());
+ assertEquals(0, r1.getStatus());
+ var finishedSpans = getAndClearSpans();
+ var parentTraceId = getRootTraceId(finishedSpans);
+ for (var span : finishedSpans) {
+ if (isRootSpan(span)) {
+ continue;
+ }
+ assertEquals(span.getParentSpanContext().getTraceId(), parentTraceId);
+ assertEquals(span.getTraceId(), parentTraceId);
+ }
+ }
+
private static boolean isRootSpan(SpanData span) {
- return span.getParentSpanContext() == SpanContext.getInvalid();
+ return !span.getParentSpanContext().isValid();
}
private static void assertCollectionName(SpanData span) {
@@ -208,4 +222,14 @@ public class TestDistributedTracing extends SolrCloudTestCase {
exporter.reset();
return result;
}
+
+ private String getRootTraceId(List<SpanData> finishedSpans) {
+ assertEquals(1, finishedSpans.stream().filter(TestDistributedTracing::isRootSpan).count());
+ return finishedSpans.stream()
+ .filter(TestDistributedTracing::isRootSpan)
+ .findFirst()
+ .get()
+ .getSpanContext()
+ .getTraceId();
+ }
}