Improve query error logging #11519
Improve query error logging #11519
Conversation
|
@maytasm can you add a more generic flag such as enableDebugLog or enableVerboseLogging etc.? this way we can use the same flag for similar situations. we could end up with too many flags otherwise. what do you think? |
That might be more confusing. A generic flag might not be obvious what the flag is for. For example, normally if you think about wanting debug log or more verbose logging you would change your log level (in your log file) to DEBUG level or TRACE level. So now the user might be confuse on what is the difference between flag such as enableDebugLog or enableVerboseLogging vs. changing log level. Also, you may require different detail logs for different scenario. For example, as someone developing a custom extension with new aggregator vs as a druid operator. A generic flag such as enableDebugLog or enableVerboseLogging that emits details log for one may not be useful at all for the other. |
|
we can also name the flag as queryLogLevel (this could have values such as TRACE, DEBUG, INFO) etc. This will give you as a developer or as a user to log more details in the future without introducing another flag for each code branch. |
How does that differs from the current use of log.warn(), log.debug(), log.trace() etc? You can change the log level for specific class in the log4j file already. Hmm.. actually I can just add an additional log.debug call to log the stack exception and remove the flag |
are you trying to set log level for a particular query run or for all all the queries? Former may be useful when you don't want the logs to be too noisy but still get the ability to troubleshoot an individual query. a change in log4j setting will apply to all the queries and require a restart of the service. |
That's a good point. I imagine this would be used for debugging during development / testing. Maybe it is not something that will change often and does not needs a query context? |
|
@maytasm In that case, I too will recommend logging error inside |
|
@abhishekagarwal87 |
There was a problem hiding this comment.
@maytasm The approach sounds good to me. I do have some reservations around the name of the flag itself. here are some suggestions from my side.
name | possible values
verbose | true,false
debug | true,false
debugMode | true,false
enableDebugging | true,false
the second option (debug) is quite common as a flag in CLI tools. so that is what I personally would prefer the most.
Improve query error logging
Description
When adding and developing new query functionality (such as adding a new Aggregator), it may be hard to debug due to limited logging information. This PR change the log level of logging query exception (in QueryLifecycle) from WARN to ERROR to make the Exception easier to find and identify. This PR also adds a query context
enableQueryDebuggingthat can make the above error logging (in QueryLifecycle) to include the stack trace of the Exception. The new context,enableQueryDebugging, would not only add the Exception stracktrace but can include other current and feature functionality that would help user debug when query doesn’t work. It would include things like additional metrics and logs (not part of this PR but will create followup PRs). This one config would let us avoid add many many other configs keys by rolling up all related query debugging functionalities into one easy to remember and use queryContext.This PR has: