SOLR-16586: Log start of SearchHandler#handleRequestBody#1237
SOLR-16586: Log start of SearchHandler#handleRequestBody#1237patsonluk wants to merge 3 commits intoapache:mainfrom
Conversation
| if (log.isInfoEnabled()) { | ||
| log.info("Start Search Query: {}", req.getParamString()); | ||
| } |
There was a problem hiding this comment.
Not sure I'm a fan of this approach. This could have some huge implications on logging requirements and storage. This is logging every single request right?
Previously it looks like this was logged only for timeouts:
} catch (ExitableDirectoryReader.ExitingReaderException ex) {
log.warn("Query: {}; ", req.getParamString(), ex);
This should at least be disabled by default.
There was a problem hiding this comment.
Thank you for the review! I do understand your concern of extra log info, it's important for our use case as queries can sometimes be totally stuck hence timeout wouldn't get printed.
I do agree that it's probably quite specific to our own scenarios (and we might not really seeing issues like that anymore), perhaps changing it to debug will be more suitable?
There was a problem hiding this comment.
it's important for our use case as queries can sometimes be totally stuck hence timeout wouldn't get printed.
yea I totally understand that. Its always nice to have logging but at the same time this subtle change has the ability to blow up existing installs. debug is a decent idea. I thought I saw a jira/pr go around about adding markers to control logging? maybe that makes sense here to try?
I just think we need to be careful about default logging just surprising folks operating Solr.
There was a problem hiding this comment.
Yes! this is the marker PR #1229 , could definitely use some review on that one too 🙇🏼
As for this one, perhaps we can keep it simple and change it to debug instead? 😊 (change committed)
|
Doesn't this allow to achieve essentially the same thing (pre-logging requests), by configuring logging to: |
That's a good point @magibney ! I guess for our own usage, we were having issues with Search queries in particular and didn't want to enable debug for SolrCore for that, which could create quite a bit of noise. However, from general Solr perspective, it does not bring much values to have 2 different debug pieces with very similar info. The scope difference - Thank you for all the comments and help! |
3 participants
https://issues.apache.org/jira/browse/SOLR-16586
Description
Add logging to start of
SearchHandler#handleRequestBodyso if processing hangs we know which query is causing thatSolution
Added info logging to the beginning of
SearchHandler#handleRequestBodywith the query paramsTests
No test added
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.