Add option to disable source logging for search slow log closes #22683

[animageofmine]

• Have you signed the contributor license agreement?
• Have you followed the contributor guidelines?
• If submitting code, have you built your formula locally prior to submission with gradle check?
• If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
• If submitting code, have you checked that your submission is for an OS that we support?

[abeyad]

@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?

[jasontedor]

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.

[animageofmine]

@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?

[abeyad]

@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?

[jasontedor]

We simply want to get the latency number for the queries.

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.

[animageofmine]

@jasontedor
Will remember that an issue is not mandatory for a PR, good to know! Thank you.

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.

[clintongormley]

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 X-Opaque-Id), the contents of which would be added to the search and indexing slow logs. (see #9669)

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.

[animageofmine]

@clintongormley : Thank you for taking this to FixitFriday. A couple of quick clarifications

1. You would accept this merge request only after Include client IP/host in slowlog entries #9669. Do I have to create a separate merge request because you closed this one?
2. When would you guys add Include client IP/host in slowlog entries #9669 ?

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:

1. Query Latency (search / index, etc..)
2. Number of queries in progress (search / index, etc...)

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!