[Security Solution][Case][Bug] Improve case logging #91924

jonathan-buttner · 2021-02-18T19:44:17Z

This PR improves the logging throughout the case plugin. This is to address feedback that about it being difficult to triage future issues relating to case because of the lack of logging of errors and failures.

I created a CaseError class to wrap the original error and store the original error object so that we can access the stacktrace if we need to in the future. The CaseError also allows converting itself to a Boom to be returned in the API handlers. If the original error was a Boom then it uses that error's statusCode otherwise it defaults to 500.

A lot of the changes look worse than they are because I added a try/catch and indented the code.

There were a couple jest tests that were expecting boom errors that I needed to update and a couple places where the tests would through a unhandled promise rejects which I fixed by adding return.

Reference: https://jestjs.io/docs/en/asynchronous#promises

If your code uses promises, there is a more straightforward way to handle asynchronous tests. Return a promise from your test, and Jest will wait for that promise to resolve. If the promise is rejected, the test will automatically fail.

This adds a lot of logging, some duplicative too. A call to log.error will happen each time we create CaseError. This may be unnecessary but more logging is better probably and we can always reduce it later.

Testing

The easiest way to test this PR is to run the integration tests and observe the console output for the kibana server. Here's some examples:

│ proc [kibana] server    log   [10:30:43.844] [error][case][plugins] Error on GET case fake-id: Error: Saved object [cases/fake-id] not found
   │ proc [kibana] server    log   [10:30:43.844] [error][case][plugins] Failed to get case id: fake-id: Error: Saved object [cases/fake-id] not found
   │ proc [kibana] server    log   [10:30:43.844] [error][case][plugins] Error getting case and/or connector and/or user actions: Failed to get case id: fake-id: Error: Saved object [cases/fake-id] not found
   │ proc [kibana] server    log   [10:30:43.844] [error][case][plugins] Failed to push case in route: CaseError: Error getting case and/or connector and/or user actions: Failed to get case id: fake-id: Error: Saved object [cases/fake-id] not found
   │ proc [kibana] server    log   [10:30:45.867] [error][case][plugins] Error getting case and/or connector and/or user actions: Saved object [action/fake-connector] not found
   │ proc [kibana] server    log   [10:30:45.867] [error][case][plugins] Failed to push case in route: CaseError: Error getting case and/or connector and/or user actions: Saved object [action/fake-connector] not found

elasticmachine · 2021-02-19T15:53:13Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

jonathan-buttner · 2021-02-19T15:55:18Z

x-pack/plugins/case/server/client/cases/create.test.ts

-          expect(e.isBoom).toBe(true);
-          expect(e.output.statusCode).toBe(400);
-        });
+      return (


Adding return to tests so that the test will wait correctly.

See: https://jestjs.io/docs/en/asynchronous#promises

jonathan-buttner · 2021-02-19T15:56:53Z

x-pack/plugins/case/server/client/cases/push.ts

@@ -84,7 +78,7 @@ export const push = async ({
    ]);
  } catch (e) {
    const message = `Error getting case and/or connector and/or user actions: ${e.message}`;
-    throw createError(e, message);
+    throw createCaseError({ message, error: e, logger });


Using a CaseError for these so that we can boomify it once we get to the route handler and keeping things consistent.

jonathan-buttner · 2021-02-19T15:59:49Z

x-pack/plugins/case/server/routes/api/utils.ts

-  const boom = isBoom(error) ? error : boomify(error, options);
+  let boom: Boom;
+
+  if (isCaseError(error)) {


Handle unwrapping the CaseError and converting it to a boom.

jonathan-buttner · 2021-02-22T14:31:57Z

@elasticmachine merge upstream

jonathan-buttner · 2021-02-22T22:43:18Z

@elasticmachine merge upstream

michaelolo24 · 2021-02-25T14:12:19Z

x-pack/plugins/case/server/client/cases/update.ts

-  return CasesResponseRt.encode(returnUpdatedCase);
+    return CasesResponseRt.encode(returnUpdatedCase);
+  } catch (error) {
+    const idVersions = cases.cases.map((caseInfo) => ({


cases.cases lol. Thoughts on updating this naming pattern in the future?

haha 💯 I'll add it to the refactoring list.

michaelolo24 · 2021-02-25T14:21:18Z

x-pack/plugins/case/server/client/client.ts

+        id: caseInfo.id,
+        version: caseInfo.version,
+      }));
+      throw createCaseError({


I initially thought the createCaseError identified errors for the createCase path, but it looks like a catch all for all case related errors. Think it's worth changing the name to just caseError or logCaseErrors or something?

Yeah good idea. I'll switch it to caseError 👍

michaelolo24 · 2021-02-25T14:27:37Z

x-pack/plugins/case/server/client/client.ts

+      throw createCaseError({
+        message: `Failed to update alerts status using client ids: ${JSON.stringify(
+          args.ids
+        )} indices: ${JSON.stringify([...args.indices])} status: ${args.status} ${error}`,


More of a personal preference here. Could help readability depending on the size of any of these fields to include line breaks between the output string, so

`Failed to update alerts status using client ids: ${JSON.stringify(args.ids)} \n indices: ${JSON.stringify([...args.indices])} \n status: ${args.status} ${error}`

michaelolo24 · 2021-02-25T14:31:50Z

x-pack/plugins/case/server/client/client.ts

+      });
+    } catch (error) {
+      throw createCaseError({
+        message: `Failed to get alerts using client ids: ${JSON.stringify(


same thing here potentially re the line breaks

michaelolo24 · 2021-02-25T14:34:59Z

x-pack/plugins/case/server/client/mocks.ts

@@ -47,7 +47,6 @@ export const createCaseClientWithMockSavedObjectsClient = async ({
  };
 }> => {
  const esClient = elasticsearchServiceMock.createElasticsearchClient();
-  // const actionsMock = createActionsClient();


michaelolo24 · 2021-02-25T14:36:03Z

x-pack/plugins/case/server/common/error.ts

+ * Create a CaseError that wraps the original thrown error. This also logs the message that will be placed in the CaseError
+ * if the logger was defined.
+ */
+export function createCaseError({


Okay, you can ignore my previous message. I thought of it as createCase Error and you mean it as create caseError 🤪

haha no worries

michaelolo24 · 2021-02-25T14:42:40Z

x-pack/plugins/case/server/routes/api/cases/get_case.ts

@@ -38,6 +38,9 @@ export function initGetCaseApi({ caseConfigureService, caseService, router }: Ro
          }),
        });
      } catch (error) {
+        logger.error(
+          `Failed to retrieve case in route case id: ${request.params.case_id} include comments: ${request.query.includeComments} include sub comments: ${request.query.includeSubCaseComments}: ${error}`


might be worth doing line breaks here too, if you decide it's worth it

michaelolo24 · 2021-02-25T14:47:30Z

x-pack/plugins/case/server/services/alerts/index.ts

  }: UpdateAlertsStatusArgs) {
    const sanitizedIndices = getValidIndices(indices);
    if (sanitizedIndices.length <= 0) {
-      // log that we only had invalid indices
+      logger.warn(`Empty alert indices when updateAlertsStatus ids: ${JSON.stringify(ids)}`);


michaelolo24

Really nice job with this! Only had a few comments, but they were all really preference things

kibanamachine · 2021-02-26T17:41:47Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #109423 succeeded 543f807
💚 Build #108614 succeeded 451bcdb
💛 Build #108405 was flaky 4114cc3
💚 Build #108103 succeeded 3b64e60
💔 Build #108059 failed a7d6fe5

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

…bana into task-manager/docs-monitoring * 'task-manager/docs-monitoring' of github.com:gmmorris/kibana: (40 commits) [Security Solution][Case][Bug] Improve case logging (elastic#91924) [Alerts][Doc] Added README documentation for alerts plugin status and framework health checks configuration options. (elastic#92761) Add warning for EQL and Threshold rules if exception list contains value list items (elastic#92914) [Security Solution][Case] Fix subcases bugs on detections and case view (elastic#91836) [APM] Always allow access to Profiling via URL (elastic#92889) [Vega] Allow image loading without CORS policy by changing the default to crossOrigin=null (elastic#91991) skip flaky suite (elastic#92114) [APM] Fix for default fields in correlations view (elastic#91868) (elastic#92090) chore(NA): bump bazelisk to v1.7.5 (elastic#92905) [Maps] fix selecting EMS basemap does not populate input (elastic#92711) API docs (elastic#92827) [kbn/test] add import/export support to KbnClient (elastic#92526) Test fix management scripted field filter functional test and unskip it (elastic#92756) [App Search] Create Curation view/functionality (elastic#92560) [Reporting/Discover] include the document's entire set of fields (elastic#92730) [Fleet] Add new index to fleet for artifacts being served out of fleet-server (elastic#92860) [Alerts][Doc] Added README documentation for API key invalidation configuration options. (elastic#92757) [Discover][docs] Add search for relevance (elastic#90611) [Alerts][Docs] Extended README.md and the user docs with the licensing information. (elastic#92564) [7.12][Telemetry] Security telemetry allowlist fix. (elastic#92850) ...

* First pass at bringing in more logging * Adding more logging to routes * Adding more logging fixing tests * Removing duplicate case string in logs * Removing unneeded export * Fixing type error * Adding line breaks to make the messages more readable * Fixing type errors Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* First pass at bringing in more logging * Adding more logging to routes * Adding more logging fixing tests * Removing duplicate case string in logs * Removing unneeded export * Fixing type error * Adding line breaks to make the messages more readable * Fixing type errors Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

First pass at bringing in more logging

df8a3c0

jonathan-buttner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 Team:Threat Hunting Security Solution Threat Hunting Team Feature:Cases Cases feature labels Feb 18, 2021

jonathan-buttner requested a review from cnasikas February 18, 2021 19:44

This was referenced Feb 18, 2021

[Security Solution][Case][Bug] Allowing deletion of collections #91926

Merged

[Security Solution][Case] Allowing deletion of collections #91526

Closed

jonathan-buttner added 4 commits February 18, 2021 16:20

Adding more logging to routes

9ad4215

Merge branch 'master' of github.com:elastic/kibana into add-case-logging

6bbafb7

Adding more logging fixing tests

c15b992

Removing duplicate case string in logs

f825ede

jonathan-buttner marked this pull request as ready for review February 19, 2021 15:53

jonathan-buttner requested a review from a team as a code owner February 19, 2021 15:53

jonathan-buttner commented Feb 19, 2021

View reviewed changes

Removing unneeded export

a7d6fe5

jonathan-buttner mentioned this pull request Feb 19, 2021

[Security Solution][Case] Subcases bugs #91843

Closed

30 tasks

Fixing type error

3b64e60

Merge branch 'master' into add-case-logging

4114cc3

Merge branch 'master' into add-case-logging

451bcdb

michaelolo24 reviewed Feb 25, 2021

View reviewed changes

michaelolo24 approved these changes Feb 25, 2021

View reviewed changes

jonathan-buttner added 4 commits February 25, 2021 10:07

Merge branch 'master' of github.com:elastic/kibana into add-case-logging

108c861

Adding line breaks to make the messages more readable

543f807

Merge branch 'master' of github.com:elastic/kibana into add-case-logging

676d9c9

Fixing type errors

8b5c2ee

jonathan-buttner merged commit 993ac50 into elastic:master Feb 26, 2021

jonathan-buttner deleted the add-case-logging branch February 26, 2021 17:48

jonathan-buttner mentioned this pull request Mar 1, 2021

[7.x] [Security Solution][Case][Bug] Improve case logging (#91924) #93022

Merged

jonathan-buttner mentioned this pull request Mar 1, 2021

[7.12] [Security Solution][Case][Bug] Improve case logging (#91924) #93023

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Case][Bug] Improve case logging #91924

[Security Solution][Case][Bug] Improve case logging #91924

jonathan-buttner commented Feb 18, 2021 •

edited

Loading

elasticmachine commented Feb 19, 2021

jonathan-buttner Feb 19, 2021

jonathan-buttner Feb 19, 2021

jonathan-buttner Feb 19, 2021

jonathan-buttner commented Feb 22, 2021

jonathan-buttner commented Feb 22, 2021

michaelolo24 Feb 25, 2021

jonathan-buttner Feb 25, 2021

michaelolo24 Feb 25, 2021

jonathan-buttner Feb 25, 2021

michaelolo24 Feb 25, 2021

michaelolo24 Feb 25, 2021

michaelolo24 Feb 25, 2021

michaelolo24 Feb 25, 2021

jonathan-buttner Feb 25, 2021

michaelolo24 Feb 25, 2021 •

edited

Loading

michaelolo24 Feb 25, 2021

michaelolo24 left a comment

kibanamachine commented Feb 26, 2021

[Security Solution][Case][Bug] Improve case logging #91924

[Security Solution][Case][Bug] Improve case logging #91924

Conversation

jonathan-buttner commented Feb 18, 2021 • edited Loading

Testing

elasticmachine commented Feb 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-buttner commented Feb 22, 2021

jonathan-buttner commented Feb 22, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelolo24 Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelolo24 left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 26, 2021

💚 Build Succeeded

Metrics [docs]

History

jonathan-buttner commented Feb 18, 2021 •

edited

Loading

michaelolo24 Feb 25, 2021 •

edited

Loading