migrationsv2: handle 413 errors and log the request details for unexpected ES failures #108213
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rudolf
changed the title
rudolf
added
Team:Core
rudolf
changed the title
rudolf
added
the
bug
|
Pinging @elastic/kibana-core (Team:Core) |
rudolf
added
release_note:fix
release_note:skip
mshustov
reviewed
Aug 12, 2021
| const req = getRequestDebugMeta(e.meta); | ||
| const failedRequestMessage = `Unexpected Elasticsearch ResponseError: statusCode: ${ | ||
| req.statusCode | ||
| }, error: ${getErrorMessage(e)}, method: ${req.method}, url: ${req.url}`; |
There was a problem hiding this comment.
Shouldn't we change the order to align with the error message in ES client: statusCode, method, URL, error message
kibana/src/core/server/elasticsearch/client/configure_client.test.ts
Lines 311 to 313 in 2a3113f
There was a problem hiding this comment.
yeah good point, I initially included the body and because the body might easily be large enough to cause the log line to truncate I wanted to put the error in the front of the log line. But after I removed the body for security reasons it makes sense to keep the same order.
mshustov
approved these changes
Aug 12, 2021
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
rudolf
added a commit
to rudolf/kibana
that referenced
this pull request
Aug 16, 2021
…ected ES failures (elastic#108213) * Log the failing request and response code when an action throws a response error * Provide useful log message when migrations fail due to ES 413 Request Entity Too Large * Don't log request body for unexpected ES request failures * Fix types * CR feedback: fix order of ES request debug log Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # src/core/server/elasticsearch/client/configure_client.ts # src/core/server/elasticsearch/index.ts
rudolf
added a commit
that referenced
this pull request
Aug 16, 2021
…ected ES failures (#108213) (#108650) * Log the failing request and response code when an action throws a response error * Provide useful log message when migrations fail due to ES 413 Request Entity Too Large * Don't log request body for unexpected ES request failures * Fix types * CR feedback: fix order of ES request debug log Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
rudolf
added a commit
that referenced
this pull request
Aug 16, 2021
…ected ES failures (#108213) (#108658) * Log the failing request and response code when an action throws a response error * Provide useful log message when migrations fail due to ES 413 Request Entity Too Large * Don't log request body for unexpected ES request failures * Fix types * CR feedback: fix order of ES request debug log Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # src/core/server/elasticsearch/client/configure_client.ts # src/core/server/elasticsearch/index.ts
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #107288
This doesn't fix the problem that a too large batch size causes migrations to fail, it only provides a log message with instructions that users can act on to fix the problem. Although long term I think we should be more dynamic and automatically adjust the batch size lower, we've only had such a large request payload fail migrations on a single cluster.
I didn't add release notes since we're not really addressing the problem.
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
For maintainers