Skip to content

[Ingest Manager] Create default Error handler. Use on /setup & EPM routes#74409

Merged
jfsiii merged 5 commits intoelastic:masterfrom
jfsiii:ingest-default-error-handler
Sep 4, 2020
Merged

[Ingest Manager] Create default Error handler. Use on /setup & EPM routes#74409
jfsiii merged 5 commits intoelastic:masterfrom
jfsiii:ingest-default-error-handler

Conversation

@jfsiii
Copy link
Copy Markdown
Contributor

@jfsiii jfsiii commented Aug 5, 2020 •
edited
Loading

Summary

This adds an IngestErrorHandler type/function. It has same output as a RequestHandler and its inputs are the 3 RequestHandler inputs + error.

type IngestErrorHandler = (
  params: IngestErrorHandlerParams
) => IKibanaResponse | Promise<IKibanaResponse>;

interface IngestErrorHandlerParams {
  error: IngestManagerError | Boom | Error;
  response: KibanaResponseFactory;
  request?: KibanaRequest;
  context?: RequestHandlerContext;
}

It maps an Error and ResponseFactory to an IKibanaResponse. Now the handlers look roughly like:

async function handler(context, request, response): KibanaResponse {
  try {
    const result = await something();
    return response.ok({ body: result });
  } catch (error) {
    return defaultIngestErrorHandler({ error, response });
  }
}

I've started with defaultIngestErrorHandler and we can add different Error handlers when/if needed.

Checklist

  • The tests added in ensure the POST /setup case didn't regress, but I'm not sure what additional tests we want.
  • I'll add tests to confirm defaultIngestErrorHandler behaves as advertised
  defaultIngestErrorHandler
    IngestManagerError
      ✓ 502: RegistryError (7ms)
      ✓ 404: PackageNotFoundError (2ms)
      ✓ 400: IngestManagerError (2ms)
    Boom
      ✓ 500: constructor - one arg (2ms)
      ✓ custom: constructor - 2 args (1ms)
      ✓ 400: Boom.badRequest (2ms)
      ✓ 404: Boom.notFound (1ms)
    all other errors
      ✓ 500 (2ms)

@jfsiii
Copy link
Copy Markdown
Contributor Author

jfsiii commented Aug 20, 2020

@elasticmachine merge upstream

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 20, 2020

merge conflict between base and head

@jfsiii jfsiii added the Team:Fleet Team label for Observability Data Collection Fleet team label Aug 20, 2020
@jfsiii jfsiii marked this pull request as ready for review August 21, 2020 12:51
@jfsiii jfsiii requested a review from a team August 21, 2020 12:51
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 21, 2020

Pinging @elastic/ingest-management (Team:Ingest Management)

@jfsiii jfsiii marked this pull request as draft August 21, 2020 14:32
@jfsiii
Copy link
Copy Markdown
Contributor Author

jfsiii commented Aug 24, 2020

@elasticmachine merge upstream

@jfsiii jfsiii mentioned this pull request Aug 25, 2020
Closed
@jfsiii
Copy link
Copy Markdown
Contributor Author

jfsiii commented Aug 26, 2020

@elasticmachine merge upstream

2 similar comments
@jfsiii
Copy link
Copy Markdown
Contributor Author

jfsiii commented Aug 27, 2020

@elasticmachine merge upstream

@jfsiii
Copy link
Copy Markdown
Contributor Author

jfsiii commented Sep 2, 2020

@elasticmachine merge upstream

@jfsiii jfsiii mentioned this pull request Sep 2, 2020
Merged
1 task
skh
skh reviewed Sep 3, 2020
View reviewed changes
jfsiii
jfsiii commented Sep 3, 2020
View reviewed changes
@jfsiii jfsiii marked this pull request as ready for review September 3, 2020 19:30
@jfsiii jfsiii requested a review from skh September 3, 2020 20:17
@jfsiii jfsiii added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.9.2 v8.0.0 labels Sep 3, 2020
@jfsiii jfsiii changed the title [Ingest Manager] Create default Error handler. Use on POST /setup [Ingest Manager] Create default Error handler. Use on /setup & EPM routes Sep 3, 2020
@jfsiii jfsiii requested review from a team and neptunian September 3, 2020 20:32
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Sep 3, 2020

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@jfsiii jfsiii merged commit fc22c7d into elastic:master Sep 4, 2020
This was referenced Sep 4, 2020
Merged
Merged
Merged
jfsiii pushed a commit that referenced this pull request Sep 5, 2020
[Ingest Manager] Create default Error handler. Use on /setup & EPM ro…
b4309d6 
…utes (#74409) (#76754)

* Add default Error handler. Try on /*setup & /epm/*

* Add return type. Rename interface

* Export error handler type & add comments

* use default error handler in installPackageHandler

* (re)-add comment
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 8, 2020
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Sep 8, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 8, 2020
jfsiii pushed a commit that referenced this pull request Sep 8, 2020
@elasticmachine
[Ingest Manager] Create default Error handler. Use on /setup & EPM ro…
68d7c7c 
…utes (#74409) (#76770)

* Add default Error handler. Try on /*setup & /epm/*

* Add return type. Rename interface

* Export error handler type & add comments

* use default error handler in installPackageHandler

* (re)-add comment
# Conflicts:
#	x-pack/plugins/ingest_manager/server/routes/epm/handlers.ts

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
dgieselaar pushed a commit to dgieselaar/kibana that referenced this pull request Sep 9, 2020
@elasticmachine @dgieselaar
[Ingest Manager] Create default Error handler. Use on /setup & EPM ro…
28b99b9 
…utes (elastic#74409) (elastic#76770)

* Add default Error handler. Try on /*setup & /epm/*

* Add return type. Rename interface

* Export error handler type & add comments

* use default error handler in installPackageHandler

* (re)-add comment
# Conflicts:
#	x-pack/plugins/ingest_manager/server/routes/epm/handlers.ts

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@ghost
Copy link
Copy Markdown

ghost commented Sep 18, 2020

Hi @jfsiii

We have gone through the ticket and observed it required DEV_Validation to test the ticket .

Please let us know if anything is missing from our end.

@jfsiii jfsiii mentioned this pull request Sep 21, 2020
Merged
6 tasks
This was referenced Oct 1, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@neptunian neptunian Awaiting requested review from neptunian

1 more reviewer

@skh skh skh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jfsiii jfsiii

Labels

release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.9.2 v7.10.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jfsiii @elasticmachine @kibanamachine @skh