haythemkh commented on a change in pull request #115:
URL: https://github.com/apache/solr/pull/115#discussion_r633554291
##########
File path: solr/core/src/java/org/apache/solr/api/V2HttpCall.java
##########
@@ -360,6 +366,85 @@ protected void execute(SolrQueryResponse rsp) {
SolrCore.postDecorateResponse(handler, solrReq, rsp);
}
+ @Override
+ protected void populateTracingSpan(Span span) {
+ // Set db.instance
+ String coreOrColName = this.origCorename;
+ if (coreOrColName == null) {
+ Map<String, String> pathTemplateValues = getUrlParts(); // ==
solrReq.getPathTemplateValues()
+ coreOrColName = pathTemplateValues.get("collection");
+ if (coreOrColName == null) {
+ coreOrColName = pathTemplateValues.get("core");
+ }
+ }
+ if (coreOrColName != null) {
+ span.setTag(Tags.DB_INSTANCE, coreOrColName);
+ }
+
+ // Get the templatize-ed path
+ String path;
+ if (api instanceof AnnotatedApi) {
+ // ideal scenario; eventually everything might be AnnotatedApi?
+ var aapi = (AnnotatedApi) api;
+ path = aapi.getEndPoint().path()[0]; // consider first to be primary
+ } else {
+ path = computeEndpointPath();
+ // TODO consider getValidators, looking for command & path?
+ }
+
+ String verb = req.getMethod().toLowerCase(Locale.ROOT);
+ // if this api has commands ...
+ final Map<String, JsonSchemaValidator> validators = getValidators(); //
should be cached
+ if (validators != null && validators.isEmpty() == false && solrReq !=
null) {
+ // does this request have one command?
+ List<CommandOperation> cmds = solrReq.getCommands(false);
+ if (cmds.size() == 1) {
+ verb = cmds.get(0).name;
+ }
+ }
+
+ span.setOperationName(verb + ":" + path);
+
+ if (req.getQueryString() != null) {
+ span.setTag("params", req.getQueryString());
Review comment:
would "params" need to be added as a constant? for example `Tags.PARAMS`
##########
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:
Would it be more interesting to specify that the user is not
authenticated via a value of None for example instead of bypassing this setting?
##########
File path: solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java
##########
@@ -603,6 +608,44 @@ public Action call() throws IOException {
}
+ /** Get the span for this request. Not null. */
+ protected Span getSpan() {
+ // Span was put into the request by SolrDispatchFilter
+ return (Span)
Objects.requireNonNull(req.getAttribute(Span.class.getName()));
+ }
+
+ // called after init().
+ protected void populateTracingSpan(Span span) {
+ // Set db.instance
+ String coreOrColName = HttpSolrCall.this.origCorename;
+ if (coreOrColName == null && getCore() != null) {
+ coreOrColName = getCore().getName();
+ }
+ if (coreOrColName != null) {
+ span.setTag(Tags.DB_INSTANCE, coreOrColName);
+ }
+
+ // Set operation name.
+ String path = getPath();
+ if (coreOrColName != null) {
+ // prefix path by core or collection name
+ if (getCore() != null && getCore().getName().equals(coreOrColName)) {
Review comment:
why not nest this block in the block above (line 621)?
##########
File path: solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java
##########
@@ -603,6 +608,44 @@ public Action call() throws IOException {
}
+ /** Get the span for this request. Not null. */
+ protected Span getSpan() {
+ // Span was put into the request by SolrDispatchFilter
+ return (Span)
Objects.requireNonNull(req.getAttribute(Span.class.getName()));
+ }
+
+ // called after init().
+ protected void populateTracingSpan(Span span) {
+ // Set db.instance
+ String coreOrColName = HttpSolrCall.this.origCorename;
+ if (coreOrColName == null && getCore() != null) {
+ coreOrColName = getCore().getName();
+ }
+ if (coreOrColName != null) {
+ span.setTag(Tags.DB_INSTANCE, coreOrColName);
+ }
+
+ // Set operation name.
+ String path = getPath();
+ if (coreOrColName != null) {
+ // prefix path by core or collection name
+ if (getCore() != null && getCore().getName().equals(coreOrColName)) {
+ path = "/{core}" + path;
+ } else {
+ path = "/{collection}" + path;
+ }
+ }
+ String verb =
+ getQueryParams()
+ .get(CoreAdminParams.ACTION, req.getMethod())
+ .toLowerCase(Locale.ROOT);
+ span.setOperationName(verb + ":" + path);
Review comment:
is it possible that verb setting is null? is it tolerable?
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -533,10 +533,23 @@ public void doFilter(ServletRequest _request,
ServletResponse _response, FilterC
if (accepted) {
rateLimitManager.decrementActiveRequests(request);
}
+ span.setTag(Tags.HTTP_STATUS, response.getStatus());
span.finish();
}
}
-
+
+ protected Span buildSpan(HttpServletRequest request, Tracer tracer) {
+ return tracer
+ .buildSpan(
+ "http.request") // will be changed later
+ .asChildOf(tracer.extract(Format.Builtin.HTTP_HEADERS, new
HttpServletCarrier(request)))
+ .withTag(Tags.SPAN_KIND, Tags.SPAN_KIND_SERVER)
Review comment:
do we need to add port and host as tags to keep alignment with the
deleted code? or is it not necessary as it is included in the url? or were they
deleted for some reason?
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -533,10 +533,23 @@ public void doFilter(ServletRequest _request,
ServletResponse _response, FilterC
if (accepted) {
rateLimitManager.decrementActiveRequests(request);
}
+ span.setTag(Tags.HTTP_STATUS, response.getStatus());
span.finish();
}
}
-
+
+ protected Span buildSpan(HttpServletRequest request, Tracer tracer) {
+ return tracer
+ .buildSpan(
+ "http.request") // will be changed later
+ .asChildOf(tracer.extract(Format.Builtin.HTTP_HEADERS, new
HttpServletCarrier(request)))
+ .withTag(Tags.SPAN_KIND, Tags.SPAN_KIND_SERVER)
+ .withTag(Tags.HTTP_METHOD, request.getMethod())
Review comment:
do we need to add port and host as tags to keep alignment with the
deleted code? or is it not necessary as it is included in the url? or were they
deleted for some reason?
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java
##########
@@ -171,23 +169,22 @@ 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);
Review comment:
why did you need to split the above row in half?
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java
##########
@@ -171,23 +169,22 @@ 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);
Review comment:
why did you need to split the above row in two?
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java
##########
@@ -171,23 +169,22 @@ 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);
Review comment:
why did you need to split the above row in two? we only use the variable
introduced `pathNormalized` for the setting below
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java
##########
@@ -171,23 +169,22 @@ 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);
Review comment:
why did you need to split the above row in two? we only use the variable
introduced `pathNormalized` for the below setting
--
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
