improve migration transform failure message and stacktrace #102478

pgayvallet · 2021-06-17T10:01:28Z

Summary

Improve the overall display of the migration failure messages when either corrupted documents or transform errors were encountered
In case of transform error specifically:
- log the version of the transform that caused the failure
- include the original error's stacktrace in the thrown error.

Example of new stacktrace:

Unable to complete saved object migrations for the [.kibana] index: Migrations failed. Reason: 2 transformation errors were encountered:
     - foo:3: Error: Migration function for version 7.14.0 threw an error
        at transform ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:685:13)
        at migrateProp ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:761:22)
        at applyMigrations ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:498:20)
        at DocumentMigrator.transformDoc ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:436:22)
        at transform ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:246:53)
        at Immediate._onImmediate ([...]/kibana/src/core/server/saved_objects/migrations/core/migrate_raw_docs.ts:152:19)
        at processImmediate (internal/timers.js:462:21)
    Caused by:
    Error: "number" attribute should be present
        at migrationFn ([...]/kibana/src/core/server/saved_objects/migrationsv2/integration_tests/type_migration_failure.test.ts:75:19)
        at transform ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:674:22)
        at migrateProp ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:761:22)
        at applyMigrations ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:498:20)
        at DocumentMigrator.transformDoc ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:436:22)
        at transform ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:246:53)
        at Immediate._onImmediate ([...]/kibana/src/core/server/saved_objects/migrations/core/migrate_raw_docs.ts:152:19)
        at processImmediate (internal/timers.js:462:21)
    - foo:4: Error: Migration function for version 7.14.0 threw an error
        at transform ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:685:13)
        at migrateProp ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:761:22)
        at applyMigrations ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:498:20)
        at DocumentMigrator.transformDoc ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:436:22)
        at transform ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:246:53)
        at Immediate._onImmediate ([...]/kibana/src/core/server/saved_objects/migrations/core/migrate_raw_docs.ts:152:19)
        at processImmediate (internal/timers.js:462:21)
    Caused by:
    Error: "number" attribute should be present
        at migrationFn ([...]/kibana/src/core/server/saved_objects/migrationsv2/integration_tests/type_migration_failure.test.ts:75:19)
        at transform ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:674:22)
        at migrateProp ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:761:22)
        at applyMigrations ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:498:20)
        at DocumentMigrator.transformDoc ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:436:22)
        at transform ([...]/kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts:246:53)
        at Immediate._onImmediate ([...]/kibana/src/core/server/saved_objects/migrations/core/migrate_raw_docs.ts:152:19)
        at processImmediate (internal/timers.js:462:21)
To allow migrations to proceed, please delete or fix these documents.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

pgayvallet · 2021-06-17T14:35:18Z

src/core/server/saved_objects/migrations/core/transform_saved_object_document_error.test.ts

+    expect(stackLines[0]).toEqual(`Error: Migration function for version 8.0.0 threw an error`);
+    expect(stackLines[stackLength - 2]).toEqual(`Caused by:`);
+    expect(stackLines[stackLength - 1]).toEqual(`some stack trace`);


Asserting against a stack trace is always fun.

pgayvallet · 2021-06-17T14:38:31Z

src/core/server/saved_objects/migrations/core/transform_saved_object_document_error.ts

+const appendCauseStack = (error: Error, cause: Error) => {
+  error.stack = (error.stack ?? '') + `\nCaused by:\n${cause.stack ?? cause.message}`;
+};


So, I choose to append the stack trace to the transform error instead of just replacing the top error's stack with the cause's stack. It felt like this made the most sense, as that way we still have the info about both errors, in case of.

(Also this is how languages properly handling nested errors are usually doing it, at least regarding the output)

Makes sense 👍

pgayvallet · 2021-06-17T14:39:25Z

src/core/server/saved_objects/migrationsv2/integration_tests/type_migration_failure.test.ts

+const expectMatchOrder = (lines: string[], patterns: FindInOrderPattern[]) => {
+  let lineIdx = 0;
+  let patternIdx = 0;
+
+  while (lineIdx < lines.length && patternIdx < patterns.length) {
+    const line = lines[lineIdx];
+    const pattern = patterns[patternIdx];
+    if (lineMatch(line, pattern)) {
+      patternIdx++;
+    }
+    lineIdx++;


A new interview exercice.

pgayvallet · 2021-06-17T14:40:47Z

src/core/server/saved_objects/migrationsv2/integration_tests/type_migration_failure.test.ts

+      expectMatchOrder(errorLines, [
+        {
+          mode: 'equal',
+          value: '- foo:3: Error: Migration function for version 7.14.0 threw an error',
+        },
+        {
+          mode: 'contain',
+          value: 'at transform',
+        },


Best okay-ish approach I found to check the content of the error stack.

pgayvallet · 2021-06-17T14:49:23Z

src/core/server/saved_objects/migrationsv2/model.test.ts

+          "Migrations failed. Reason: 1 corrupt saved object documents were found: a:b
+          To allow migrations to proceed, please delete or fix these documents."


So, I did not handle singular/plural in the message, as it didn't really feel that important, but if we really want it, I could do it.

elasticmachine · 2021-06-17T14:54:18Z

Pinging @elastic/kibana-core (Team:Core)

Bamieh

LGTM with a small nit in the comments

Bamieh · 2021-06-18T14:50:19Z

src/core/server/saved_objects/migrations/core/transform_saved_object_document_error.ts

  }
 }
+
+const appendCauseStack = (error: Error, cause: Error) => {
+  error.stack = (error.stack ?? '') + `\nCaused by:\n${cause.stack ?? cause.message}`;


if there is no error.stack we'd be starting with an empty new line \nCaused by:

Yea, OTOH until this function is extracted from this file, I think this is alright, as the only call to appendCauseStack is performed inside the constructor after the call to super, so we shouldn't really encounter a situation where the stack is empty (and we got unit tests to assert that)

Bamieh · 2021-06-18T14:50:28Z

src/core/server/saved_objects/migrations/core/transform_saved_object_document_error.ts

+const appendCauseStack = (error: Error, cause: Error) => {
+  error.stack = (error.stack ?? '') + `\nCaused by:\n${cause.stack ?? cause.message}`;
+};


Makes sense 👍

…c-transform-failure-cause

kibanamachine · 2021-06-21T08:25:50Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #132028 succeeded a63bf0c
💔 Build #131987 failed e74174b
💔 Build #131980 failed 8e5c486

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

…02478) * improve migration failure message and stacktrace * remove console.log * fix unit tests * fix integration tests * fix more unit tests

…2478) (#102702) * improve migration transform failure message and stacktrace (#102478) * improve migration failure message and stacktrace * remove console.log * fix unit tests * fix integration tests * fix more unit tests * skip 8.0 test in `7.x`

pgayvallet added 2 commits June 17, 2021 12:00

improve migration failure message and stacktrace

1bc7c65

remove console.log

8e5c486

pgayvallet added 3 commits June 17, 2021 12:10

fix unit tests

9c4c031

fix integration tests

e74174b

fix more unit tests

a63bf0c

pgayvallet commented Jun 17, 2021

View reviewed changes

pgayvallet marked this pull request as ready for review June 17, 2021 14:54

pgayvallet requested a review from a team as a code owner June 17, 2021 14:54

Bamieh approved these changes Jun 19, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-102336-log-do…

d6540bf

…c-transform-failure-cause

pgayvallet merged commit 0561648 into elastic:master Jun 21, 2021

pgayvallet mentioned this pull request Jun 21, 2021

[7.x] improve migration transform failure message and stacktrace (#102478) #102702

Merged

pgayvallet mentioned this pull request Jul 2, 2021

[SO Migration] improve the overall log output #104245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve migration transform failure message and stacktrace #102478

improve migration transform failure message and stacktrace #102478

pgayvallet commented Jun 17, 2021 •

edited

Loading

pgayvallet Jun 17, 2021

pgayvallet Jun 17, 2021

Bamieh Jun 18, 2021

pgayvallet Jun 17, 2021

pgayvallet Jun 17, 2021

pgayvallet Jun 17, 2021

elasticmachine commented Jun 17, 2021

Bamieh left a comment

Bamieh Jun 18, 2021

pgayvallet Jun 21, 2021

Bamieh Jun 18, 2021

kibanamachine commented Jun 21, 2021

		"Migrations failed. Reason: 1 corrupt saved object documents were found: a:b
		To allow migrations to proceed, please delete or fix these documents."

improve migration transform failure message and stacktrace #102478

improve migration transform failure message and stacktrace #102478

Conversation

pgayvallet commented Jun 17, 2021 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 17, 2021

Bamieh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jun 21, 2021

💚 Build Succeeded

Metrics [docs]

History

pgayvallet commented Jun 17, 2021 •

edited

Loading