Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[KP] log details of the failed elasticsearch client requests when logQueries:true #73672

Merged
merged 7 commits into from
Dec 10, 2020
Merged

[KP] log details of the failed elasticsearch client requests when logQueries:true #73672

merged 7 commits into from
Dec 10, 2020

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Jul 29, 2020 •
edited
Loading

Summary

closes #84447
before:

[version_conflict_engine_exception]: [task:Alerting-alerting_telemetry]: version conflict, document already exists (current version [4])

after (requires logQueries: true):

409
PUT /.kibana_task_manager/_create/task%3AAlerting-alerting_telemetry 
{} [version_conflict_engine_exception]: [task:Alerting-alerting_telemetry]: version conflict, document already exists (current version [4])

Checklist

For maintainers

@mshustov mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Jul 29, 2020
@mshustov mshustov requested a review from a team as a code owner July 29, 2020 14:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@epixa epixa mentioned this pull request Jul 29, 2020
Closed
14 tasks
joshdover
joshdover approved these changes Jul 29, 2020
View reviewed changes
Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel an integration test is necessary on this

src/core/server/elasticsearch/client/configure_client.test.ts Show resolved Hide resolved
@mshustov
Copy link
Contributor Author

I didn't manage to reproduce response error logging in the legacy client. It might make sense not to log them by default, since it can be a part of the logic flow, so it shouldn't bother the end-user. @spalger was it muted in the LP due to the same reasoning?

@spalger
Copy link
Contributor

spalger commented Aug 24, 2020

@spalger was it muted in the LP due to the same reasoning?

I have no idea actually, sorry, I don't remember this happening.

@mshustov mshustov added v7.11.0 and removed v7.10.0 labels Oct 8, 2020
@mshustov mshustov mentioned this pull request Dec 10, 2020
Closed
@mshustov mshustov changed the title [KP] log details of the failed elasticsearch client requests [KP] log details of the failed elasticsearch client requests when logQueries:true Dec 10, 2020
pgayvallet
pgayvallet approved these changes Dec 10, 2020
View reviewed changes
src/core/server/elasticsearch/client/configure_client.ts
Comment on lines +67 to +68
const body = params.body ? `\n${ensureString(params.body)}` : '';
return `${event.statusCode}\n${params.method} ${url}${body}`;
Copy link
Contributor

@pgayvallet pgayvallet Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's consistent with the legacy client format (and was not introduced by this PR), but I wonder if this line break between the status code and the rest of the log message really makes sense.

400 GET /_path [undefined]: Response Error

seems better than

400
GET /_path [undefined]: Response Error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pgayvallet I added a comment about the format 41dad79

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 46990 47750 +760

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mshustov mshustov merged commit a2813c8 into elastic:master Dec 10, 2020
@mshustov mshustov deleted the improve-es-errors-logs branch December 10, 2020 20:38
@mshustov mshustov mentioned this pull request Dec 10, 2020
Merged
mshustov added a commit to mshustov/kibana that referenced this pull request Dec 10, 2020
@mshustov
[KP] log details of the failed elasticsearch client requests when log…
6940eb1 
…Queries:true (elastic#73672)

* log request details in case of errors

* update test

* log errors in the new client only when logQueries: true

* add comment about log format
mshustov added a commit that referenced this pull request Dec 11, 2020
@mshustov
[KP] log details of the failed elasticsearch client requests when log…
639e8ef 
…Queries:true (#73672) (#85641)

* log request details in case of errors

* update test

* log errors in the new client only when logQueries: true

* add comment about log format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgayvallet pgayvallet pgayvallet approved these changes

@joshdover joshdover joshdover approved these changes

@rudolf rudolf Awaiting requested review from rudolf

Assignees
No one assigned
Labels
backported release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ElasticsearchClient should allow plugins to opt out of error logging
7 participants
@mshustov @elasticmachine @spalger @kibanamachine @rudolf @pgayvallet @joshdover