madrob commented on a change in pull request #115:
URL: https://github.com/apache/solr/pull/115#discussion_r630529230
##########
File path: solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java
##########
@@ -144,19 +146,31 @@ public IndexSchema getSchema() {
@Override
public Tracer getTracer() {
- if (core != null) {
- return core.getCoreContainer().getTracer(); // this is the common path
- }
final HttpSolrCall call = getHttpSolrCall();
if (call != null) {
final Tracer tracer = (Tracer)
call.getReq().getAttribute(Tracer.class.getName());
if (tracer != null) {
return tracer;
}
}
+ if (core != null) {
+ return core.getCoreContainer().getTracer();
Review comment:
Why do we prefer the call tracer to the core tracer now?
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -499,6 +495,7 @@ public void doFilter(ServletRequest _request,
ServletResponse _response, FilterC
if (log.isDebugEnabled()) {
log.debug("User principal: {}", request.getUserPrincipal());
}
+ span.setTag(Tags.DB_USER, String.valueOf(request.getUserPrincipal()));
Review comment:
What does this set to without authentication enabled?
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java
##########
@@ -171,23 +173,50 @@ public SolrQueryRequest parse( SolrCore core, String
path, HttpServletRequest re
ArrayList<ContentStream> streams = new ArrayList<>(1);
SolrParams params = parser.parseParamsAndFillStreams( req, streams );
- Span span = (Span) req.getAttribute(Span.class.getName()); // not null but
maybe in some tests?
- if (span != null && !(span instanceof NoopSpan)) {
- span.setTag("params", params.toString());
- }
SolrQueryRequest sreq = buildRequestFrom(core, params, streams,
getRequestTimer(req), req);
// Handlers and login will want to know the path. If it contains a ':'
// the handler could use it for RESTful URLs
- sreq.getContext().put(PATH, RequestHandlers.normalize(path));
+ String pathNormalized = RequestHandlers.normalize(path);
+ sreq.getContext().put(PATH, pathNormalized);
sreq.getContext().put("httpMethod", req.getMethod());
if(addHttpRequestToContext) {
sreq.getContext().put("httpRequest", req);
}
+
+ // add span info
+ TraceUtils.ifNotNoop(
Review comment:
I'm not sure, but I think this still has to do all of the object
construction even when the span is a nop. Maybe better to inline this.
##########
File path:
solr/core/src/test/org/apache/solr/util/tracing/TestDistributedTracing.java
##########
@@ -106,6 +109,50 @@ public void test() throws IOException,
SolrServerException, TimeoutException, In
fail("All spans must belong to single span, but:"+finishedSpans);
}
}
+ assertEquals("get:/select", finishedSpans.get(0).operationName());
+ assertDbInstanceColl(finishedSpans.get(0));
+ }
+
+ @Test
+ public void testAdminApi() throws Exception {
+ CloudSolrClient cloudClient = cluster.getSolrClient();
+ List<MockSpan> finishedSpans;
+
+ // Admin API call
+ cloudClient.request(new GenericSolrRequest(SolrRequest.METHOD.GET,
"/admin/metrics", params()));
+ finishedSpans = getAndClearSpans();
+ assertEquals("get:/admin/metrics", finishedSpans.get(0).operationName());
+
+ CollectionAdminRequest.listCollections(cloudClient);
+ finishedSpans = getAndClearSpans();
+ assertEquals("list:/admin/collections",
finishedSpans.get(0).operationName());
+ }
+
+ @Test
+ @Ignore // not supported yet
+ public void testV2Api() throws Exception {
+ CloudSolrClient cloudClient = cluster.getSolrClient();
+ List<MockSpan> finishedSpans;
+
+ new V2Request.Builder("/c/" + COLLECTION)
+ .withMethod(SolrRequest.METHOD.POST)
+ .withPayload("{\n" +
+ " \"reload\" : {}\n" +
+ "}")
+ .build()
+ .process(cloudClient);
+ finishedSpans = getAndClearSpans();
+ assertEquals("reload:/c/{collection}",
finishedSpans.get(0).operationName());
+ assertDbInstanceColl(finishedSpans.get(0));
+ }
+
+ private void assertDbInstanceColl(MockSpan mockSpan) {
+ MatcherAssert.assertThat(mockSpan.tags().get("db.instance"),
Matchers.equalTo("collection1"));
Review comment:
generally it's ok to do static imports on matcher methods.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
