You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by st...@apache.org on 2023/11/03 16:41:52 UTC
(solr) branch branch_9x updated: [9.x] SOLR-16943 Move Jetty HttpClient tracing into InstrumentedHttpListenerFactory (#2041)
This is an automated email from the ASF dual-hosted git repository.
stillalex pushed a commit to branch branch_9x
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/branch_9x by this push:
new 3cc283e30cf [9.x] SOLR-16943 Move Jetty HttpClient tracing into InstrumentedHttpListenerFactory (#2041)
3cc283e30cf is described below
commit 3cc283e30cfe1116ac0f1a6479df72345201cffe
Author: Alex D <st...@apache.org>
AuthorDate: Fri Nov 3 09:41:46 2023 -0700
[9.x] SOLR-16943 Move Jetty HttpClient tracing into InstrumentedHttpListenerFactory (#2041)
* 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>
Co-authored-by: David Smiley <ds...@salesforce.com>
---
solr/CHANGES.txt | 2 ++
.../solr/handler/component/HttpShardHandler.java | 11 ------
.../org/apache/solr/update/SolrCmdDistributor.java | 11 ------
.../stats/InstrumentedHttpListenerFactory.java | 23 ++++++++++--
.../stats/InstrumentedHttpRequestExecutor.java | 2 ++
...rier.java => SolrApacheHttpRequestCarrier.java} | 14 ++++----
...stCarrier.java => SolrJettyRequestCarrier.java} | 14 ++++----
.../org/apache/solr/util/tracing/TraceUtils.java | 21 +++++++++++
.../solr/util/tracing/TestDistributedTracing.java | 41 +++++++++++++++++-----
9 files changed, 93 insertions(+), 46 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 3f1da9ebb37..bce1321cf25 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -20,6 +20,8 @@ Improvements
* SOLR-17046: SchemaCodecFactory is now the implicit default codec factory. (hossman)
+* SOLR-16943: Extend Solr client tracing coverage to both Jetty Client and Apache HttpClient (Alex Deparvu, David Smiley)
+
Optimizations
---------------------
(No changes)
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 0ad76294335..c4b8873b681 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
@@ -16,9 +16,6 @@
*/
package org.apache.solr.handler.component;
-import io.opentracing.Span;
-import io.opentracing.Tracer;
-import io.opentracing.propagation.Format;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -50,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.SolrRequestCarrier;
@NotThreadSafe
public class HttpShardHandler extends ShardHandler {
@@ -128,9 +124,6 @@ public class HttpShardHandler extends ShardHandler {
final ShardRequest sreq, final String shard, final ModifiableSolrParams params) {
// do this outside of the callable for thread safety reasons
final List<String> urls = getURLs(shard);
- final Tracer tracer = sreq.tracer; // not null
- final Span span = tracer.activeSpan(); // probably not null?
-
params.remove(CommonParams.WT); // use default (currently javabin)
params.remove(CommonParams.VERSION);
QueryRequest req = makeQueryRequest(sreq, params, shard);
@@ -171,10 +164,6 @@ public class HttpShardHandler extends ShardHandler {
@Override
public void onStart() {
- if (span != null) {
- tracer.inject(
- span.context(), Format.Builtin.HTTP_HEADERS, new SolrRequestCarrier(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 b7687d4eb1b..d31ddc9ca55 100644
--- a/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java
+++ b/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java
@@ -16,9 +16,6 @@
*/
package org.apache.solr.update;
-import io.opentracing.Span;
-import io.opentracing.Tracer;
-import io.opentracing.propagation.Format;
import java.io.Closeable;
import java.io.IOException;
import java.io.InputStream;
@@ -52,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.SolrRequestCarrier;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -315,13 +311,6 @@ public class SolrCmdDistributor implements Closeable {
req.uReq.setUserPrincipal(SolrRequestInfo.getRequestInfo().getReq().getUserPrincipal());
}
- Tracer tracer = req.cmd.getTracer();
- Span parentSpan = tracer.activeSpan();
- if (parentSpan != null) {
- tracer.inject(
- parentSpan.context(), Format.Builtin.HTTP_HEADERS, new SolrRequestCarrier(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..7975499b5a2 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,22 @@ package org.apache.solr.util.stats;
import static org.apache.solr.metrics.SolrMetricManager.mkName;
import com.codahale.metrics.Timer;
+import io.opentracing.Span;
+import io.opentracing.Tracer;
+import io.opentracing.util.GlobalTracer;
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 +90,24 @@ public class InstrumentedHttpListenerFactory implements SolrMetricProducer, Http
public RequestResponseListener get() {
return new RequestResponseListener() {
Timer.Context timerContext;
+ Tracer tracer = GlobalTracer.get();
+ Span span;
+
+ @Override
+ public void onQueued(Request request) {
+ // do tracing onQueued because it's called from Solr's thread
+ span = tracer.activeSpan();
+ TraceUtils.injectTraceContext(request, span);
+ }
@Override
public void onBegin(Request request) {
if (solrMetricsContext != null) {
timerContext = timer(request).time();
}
+ if (span != null) {
+ span.log("Client Send"); // perhaps delayed a bit after the span started in enqueue
+ }
}
@Override
@@ -99,6 +115,9 @@ public class InstrumentedHttpListenerFactory implements SolrMetricProducer, Http
if (timerContext != null) {
timerContext.stop();
}
+ if (result.isFailed() && span != null) {
+ span.log(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/SolrRequestCarrier.java b/solr/core/src/java/org/apache/solr/util/tracing/SolrApacheHttpRequestCarrier.java
similarity index 75%
copy from solr/core/src/java/org/apache/solr/util/tracing/SolrRequestCarrier.java
copy to solr/core/src/java/org/apache/solr/util/tracing/SolrApacheHttpRequestCarrier.java
index 6f4917e1b8f..7611623f328 100644
--- a/solr/core/src/java/org/apache/solr/util/tracing/SolrRequestCarrier.java
+++ b/solr/core/src/java/org/apache/solr/util/tracing/SolrApacheHttpRequestCarrier.java
@@ -20,15 +20,15 @@ package org.apache.solr.util.tracing;
import io.opentracing.propagation.TextMap;
import java.util.Iterator;
import java.util.Map;
-import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.http.HttpRequest;
-/** An OpenTracing Carrier for injecting Span context through SolrRequest */
-public class SolrRequestCarrier implements TextMap {
+/** An OpenTracing Carrier for injecting Span context through an Apache HttpRequest */
+public class SolrApacheHttpRequestCarrier implements TextMap {
- private final SolrRequest<?> solrRequest;
+ private final HttpRequest httpRequest;
- public SolrRequestCarrier(SolrRequest<?> solrRequest) {
- this.solrRequest = solrRequest;
+ public SolrApacheHttpRequestCarrier(HttpRequest httpRequest) {
+ this.httpRequest = httpRequest;
}
@Override
@@ -38,6 +38,6 @@ public class SolrRequestCarrier implements TextMap {
@Override
public void put(String key, String value) {
- solrRequest.addHeader(key, value);
+ httpRequest.setHeader(key, value);
}
}
diff --git a/solr/core/src/java/org/apache/solr/util/tracing/SolrRequestCarrier.java b/solr/core/src/java/org/apache/solr/util/tracing/SolrJettyRequestCarrier.java
similarity index 76%
rename from solr/core/src/java/org/apache/solr/util/tracing/SolrRequestCarrier.java
rename to solr/core/src/java/org/apache/solr/util/tracing/SolrJettyRequestCarrier.java
index 6f4917e1b8f..8d8ee783950 100644
--- a/solr/core/src/java/org/apache/solr/util/tracing/SolrRequestCarrier.java
+++ b/solr/core/src/java/org/apache/solr/util/tracing/SolrJettyRequestCarrier.java
@@ -20,15 +20,15 @@ package org.apache.solr.util.tracing;
import io.opentracing.propagation.TextMap;
import java.util.Iterator;
import java.util.Map;
-import org.apache.solr.client.solrj.SolrRequest;
+import org.eclipse.jetty.client.api.Request;
-/** An OpenTracing Carrier for injecting Span context through SolrRequest */
-public class SolrRequestCarrier implements TextMap {
+/** An OpenTracing Carrier for injecting Span context through a Jetty Request */
+public class SolrJettyRequestCarrier implements TextMap {
- private final SolrRequest<?> solrRequest;
+ private final Request request;
- public SolrRequestCarrier(SolrRequest<?> solrRequest) {
- this.solrRequest = solrRequest;
+ public SolrJettyRequestCarrier(Request request) {
+ this.request = request;
}
@Override
@@ -38,6 +38,6 @@ public class SolrRequestCarrier implements TextMap {
@Override
public void put(String key, String value) {
- solrRequest.addHeader(key, value);
+ request.headers(httpFields -> httpFields.put(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 c64dcffa88a..0cc9ed43bb3 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
@@ -17,12 +17,17 @@
package org.apache.solr.util.tracing;
import io.opentracing.Span;
+import io.opentracing.Tracer;
import io.opentracing.noop.NoopSpan;
+import io.opentracing.propagation.Format;
import io.opentracing.tag.Tags;
+import io.opentracing.util.GlobalTracer;
import java.util.List;
import java.util.function.Consumer;
import java.util.function.Predicate;
+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 {
@@ -48,6 +53,22 @@ public class TraceUtils {
}
}
+ public static void injectTraceContext(Request req, Span span) {
+ Tracer tracer = GlobalTracer.get();
+ if (span != null) {
+ tracer.inject(span.context(), Format.Builtin.HTTP_HEADERS, new SolrJettyRequestCarrier(req));
+ }
+ }
+
+ public static void injectTraceContext(HttpRequest req) {
+ Tracer tracer = GlobalTracer.get();
+ Span span = tracer.activeSpan();
+ if (span != null) {
+ tracer.inject(
+ span.context(), Format.Builtin.HTTP_HEADERS, new SolrApacheHttpRequestCarrier(req));
+ }
+ }
+
public static void setOperations(SolrQueryRequest req, String clazz, List<String> ops) {
if (!ops.isEmpty()) {
req.getSpan().setTag("ops", String.join(",", ops));
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 8fe52fa515f..3c56a45d999 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
@@ -31,6 +31,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;
@@ -98,14 +99,7 @@ public class TestDistributedTracing extends SolrCloudTestCase {
finishedSpans.removeIf(x -> !x.tags().get("http.url").toString().endsWith("/select"));
// one from client to server, 2 for execute query, 2 for fetching documents
assertEquals(5, finishedSpans.size());
- assertEquals(1, finishedSpans.stream().filter(s -> s.parentId() == 0).count());
- long parentId =
- finishedSpans.stream()
- .filter(s -> s.parentId() == 0)
- .collect(Collectors.toList())
- .get(0)
- .context()
- .spanId();
+ var parentId = getRootTraceId(finishedSpans);
for (MockSpan span : finishedSpans) {
if (span.parentId() != 0 && parentId != span.parentId()) {
fail("All spans must belong to single span, but:" + finishedSpans);
@@ -166,6 +160,27 @@ 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 (span.parentId() == 0) {
+ continue;
+ }
+ assertEquals(span.parentId(), parentTraceId);
+ }
+ }
+
private void assertDbInstanceColl(MockSpan mockSpan) {
MatcherAssert.assertThat(mockSpan.tags().get("db.instance"), Matchers.equalTo("collection1"));
}
@@ -193,4 +208,14 @@ public class TestDistributedTracing extends SolrCloudTestCase {
tracer.reset();
return result;
}
+
+ private long getRootTraceId(List<MockSpan> finishedSpans) {
+ assertEquals(1, finishedSpans.stream().filter(s -> s.parentId() == 0).count());
+ return finishedSpans.stream()
+ .filter(s -> s.parentId() == 0)
+ .collect(Collectors.toList())
+ .get(0)
+ .context()
+ .spanId();
+ }
}