-
Notifications
You must be signed in to change notification settings - Fork 24.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to disable source logging for search slow log closes #22683 #22685
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@animageofmine thank you for contributing again!
However, I'm not so sure we want to add the ability to truncate the query JSON output here. For indexing, it makes sense because you could potentially be sending very large JSON documents in the indexing request, and the IndexingSlowLog
logs the document source. On the other hand, the SearchSlowLog
is simply logging the query request's source, and I don't think you would want that truncated in the slow logs because (1) truncating the query source may chop off a part of it that is critical in helping understand why the query was slow and (2) query source in general should not grow as huge as the document source when indexing large documents.
What do you think?
Please add a description to the body of the PR, and mark it as closing the issue that you opened (note for future reference that you do not need to open an issue to open a PR). Also, this should not be interpreted as tacit approval of this PR, this is just a request for you to formalize it so that we can consider it. |
@abeyad Thank you for looking into this request. I think you have a very good point and a reason for logging search requests, however, the search query could contain potentially sensitive information (PII) and also makes the log message quite verbose. We simply want to get the latency number for the queries. Since there is an option to disable slow logs altogether, I think there is business value in providing an option to disable source for the search query as well. If it makes more sense, I can change the code to make it a boolean flag for logging search queries, which means either the query is logged or not. There would not be an option to log part of the query. Thoughts? |
@animageofmine I prefer the idea of just having a boolean flag to turn the query source on or off in the slow log reporting, the counter argument being it may be better to have symmetry of options between the search log and indexing log. Still, I feel for the slow query log, either we should have the entire query or none of it, for example for your use case where you are only considered with the metrics, not the query itself. @clintongormley thoughts on this one? |
I think that using the search slow log for this is the wrong tool for the job. I think that if you want to collect latencies then you should write a plugin that implements a search operation listener from which you can collect the necessary stats. |
@jasontedor As for your 2nd post, I think that's a fair argument. I personally would opt for instrumentation in ES. Latency metrics should be instrumented (and they might be actually which I am not aware of) and those who are interested can consume it. There is a telegraf plugin that we use today for KPIs, however, search/index latencies are not in the list. Here is the plugin: https://github.com/influxdata/telegraf/tree/master/plugins/inputs/elasticsearch @abeyad I am fine either way. Please let me know and I will make the changes. |
We discussed this in FixItFriday today. We don't like truncating the source as that renders it useless, but we do see some benefit to being able to disable source output. However, adding only this option still renders the slow log pretty much useless as there is very little information to identify the slow query or its origins. Instead, we should allow the requestor to specify a header (eg So @animageofmine, thanks for the PR but we won't merge it in its current state. We would accept a PR for disabling source logging, but #9669 would be need to be fixed before we'd merge it. |
@clintongormley : Thank you for taking this to FixitFriday. A couple of quick clarifications
On a side note, since we were talking about metrics in this thread, I wanted to check what are some metrics that we should use for the following:
Telegraf provides some stats (mostly node stats and cluster health), but they don't seem to include the above metrics. I can go ahead and update telegraf code, but would like to know what APIs would expose these stats. (FYI: I already posted this on discuss.elastic.co, but haven't heard back from anyone). Thank you! |
gradle check
?