Improve query metrics logging #6814
Conversation
|
Can one of the admins verify this patch? |
zkirksey
changed the title
| @@ -233,6 +234,8 @@ public QueryResponse federate(List<Source> sources, QueryRequest queryRequest) { | |||
| queryResponse = queryResponseQueue; | |||
| LOGGER.debug("returning returnResults: {}", queryResponse); | |||
| } | |||
| Map<String, Serializable> props = queryResponse.getProperties(); | |||
| props.put("qm.sort.elapsed", endTime - startTime); | |||
There was a problem hiding this comment.
Isn't this just the time it takes to submit a task to the executor service? That is, it's not the time to complete the task. Was that the intent?
There was a problem hiding this comment.
The intention is to capture the time that the query takes to sort the results. I suppose a more accurate place for this would be in SortedQueryMonitor. Will make that change
|
build now |
|
Internal build has been started, your results will be available at build completion. |
| Long prevSolrTime = (Long) responseProps.get(SQCMB + "solrquery" + QM_ELAPSED); | ||
| prevSolrTime = prevSolrTime == null ? 0 : prevSolrTime; |
There was a problem hiding this comment.
❓ What is this for? I don't see anywhere in this code where this gets set previously so wouldn't this always be null/0? The responseProps object is created as a new map earlier in this method and that particular key is not added anywhere else.
There was a problem hiding this comment.
Also, where is solrQueries ever being set such that it wouldn't always be null/0?
| @@ -235,6 +236,7 @@ public void run() { | |||
| returnProperties.put("hitsPerSource", hitsPerSource); | |||
| returnProperties.put( | |||
| ORIGINAL_SOURCE_PROPERTIES, (Serializable) Collections.unmodifiableMap(sourceProperties)); | |||
| returnProperties.put("qm.sort.elapsed", System.nanoTime() - startTime); | |||
There was a problem hiding this comment.
Either this key is misleading or it isn't measuring what you think it is measuring. By using the start time when the run method is called and then calculating the elapsed time here, that calculates the total search time for all sources, including sorting/trimming as necessary. The for loop above this is going through all of the sources in the query and waiting for them to be complete so that means their search time is included. Sorting happens each time an individual source completes so there is really no way to capture just the "sort" time unless you are going to do that per source, in which case I would expect the name to be different and also calculated in a different spot.
On the other hand, if this is meant to capture the full timing of the sorted query monitor for retrieving all of the source results, sorting them, and trimming to a page size then this is a fine calculation but as stated earlier, I think the name should be something different because just calling it "sort.elapsed" seems misleading if you don't know what it actually represents.
|
Build FAILURE See the job results in legacy Jenkins UI or in Blue Ocean UI. |
|
|
build now |
|
Internal build has been started, your results will be available at build completion. |
|
Build FAILURE See the job results in legacy Jenkins UI or in Blue Ocean UI. |
|
build now |
|
Internal build has been started, your results will be available at build completion. |
|
Build FAILURE See the job results in legacy Jenkins UI or in Blue Ocean UI. |
|
build now |
|
Internal build has been started, your results will be available at build completion. |
|
Build FAILURE See the job results in legacy Jenkins UI or in Blue Ocean UI. |
zkirksey
changed the title
zkirksey
requested review from
andrewkfiedler,
coyotesqrl,
bdeining,
rzwiefel and
ricklarsen
as code owners
Added additional query metrics Small bug fix Improved query metrics logging Adding new timing metrics Addressing comments Removed unused metrics Cleaned up more unused code
zkirksey
force-pushed
the
improve-logging
branch
from
89d0917 to
4a331ca
Compare
|
moved to #6822 |
What does this PR do?
Adds a few additional metrics to the existing query metric functionality and defines process for logging these metrics. In order to enable metric logging, the
metrics-enabledproperty must be set totrueon the query's request or response properties. In addition, additional metrics can be added to the log statement by adding a map to request or response properties viaadditional-query-metrics.Who is reviewing it?
Select relevant component teams:
Ask 2 committers to review/merge the PR and tag them here.
How should this be tested?
Any background context you want to provide?
What are the relevant tickets?
Fixes: #____
Screenshots
Checklist:
Notes on Review Process
Please see Notes on Review Process for further guidance on requirements for merging and abbreviated reviews.
Review Comment Legend: