[APM] Add anomaly detection API tests + fixes #73120

sorenlouv · 2020-07-23T19:07:51Z

Add anomaly detection API tests (read/write/no access users)
Remove error codes (not needed, since we can just set correct error messages)
use Boom errors where appropriate
only fetches anomaly detection jobs when available to user (Anomaly detection link)
requires correct ML read/write privileges to access get/create routes

elasticmachine · 2020-07-23T19:07:56Z

Pinging @elastic/apm-ui (Team:apm)

sorenlouv · 2020-07-23T22:03:29Z

x-pack/test/apm_api_integration/common/authentication.ts

-      indices: [
-        { names: ['observability-annotations'], privileges: ['read', 'view_index_metadata'] },
-      ],
-    },


These privileges are now part of the apm_user role.

sorenlouv · 2020-07-24T22:47:33Z

x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/create_jobs.ts

-        text: MLErrorMessages[res.errorCode],
-      });
-      return false;
-    }


Got rid of the errorCode in the response. Now the server side will throw the error and the client should simply read the error message (error.message).

x-pack/plugins/apm/public/components/shared/Links/apm/AnomalyDetectionSetupLink.tsx

sorenlouv · 2020-07-24T23:17:56Z

x-pack/plugins/apm/public/components/shared/Links/apm/AnomalyDetectionSetupLink.tsx

+      </EuiButtonEmpty>
+
+      {canGetJobs && hasValidLicense ? (
+        <MissingJobsAlert environment={environment} />


We should only show ML warnings if the user can act on them. Which means: the user should have read privileges to ML and a valid license

sorenlouv · 2020-07-25T21:28:25Z

x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts

@@ -29,16 +29,12 @@ export async function createAnomalyDetectionJobs(
  const { ml, indices } = setup;

  if (!ml) {
-    throw new AnomalyDetectionError(ErrorCode.ML_NOT_AVAILABLE);
+    throw Boom.serverUnavailable(ML_ERRORS.ML_NOT_AVAILABLE);


Use Boom errors over the custom AnomalyDetectionError (which I removed everywhere)

I'm not sure if a 503 is appropriate here. 503 implies that it is a temporary error, and should be retried. Maybe a 500 makes more sense?

sorenlouv · 2020-07-25T21:30:13Z

x-pack/plugins/apm/server/lib/anomaly_detection/has_legacy_jobs.ts

@@ -12,7 +15,12 @@ export async function hasLegacyJobs(setup: Setup) {
  const { ml } = setup;

  if (!ml) {
-    return false;
+    throw Boom.serverUnavailable(ML_ERRORS.ML_NOT_AVAILABLE);


Throwing early instead of returning a default value.

See other comment, we could consider a 500 instead of a 503.

sorenlouv · 2020-07-25T21:31:37Z

x-pack/plugins/apm/server/routes/settings/anomaly_detection.ts


 // get ML anomaly detection jobs for each environment
 export const anomalyDetectionJobsRoute = createRoute(() => ({
  method: 'GET',
  path: '/api/apm/settings/anomaly-detection',
+  options: {
+    tags: ['access:apm', 'access:ml:canGetJobs'],
+  },


Require ML read privileges to access route

sorenlouv · 2020-07-25T21:31:55Z

x-pack/plugins/apm/server/routes/settings/anomaly_detection.ts

@@ -65,7 +52,7 @@ export const createAnomalyDetectionJobsRoute = createRoute(() => ({
  method: 'POST',
  path: '/api/apm/settings/anomaly-detection/jobs',
  options: {
-    tags: ['access:apm', 'access:apm_write'],
+    tags: ['access:apm', 'access:apm_write', 'access:ml:canCreateJob'],


require ML write privileges to access route

sorenlouv · 2020-07-26T22:15:19Z

x-pack/plugins/apm/public/components/shared/Links/apm/AnomalyDetectionSetupLink.test.tsx

    });
+
+    expect(toolTipAnchor).not.toBeInTheDocument();
+    expect(toolTipText).toBe(undefined);
  });


I think the tests became very clean and easy to read. It did come at the cost of making renderTooltipAnchor slightly more complex. All in all I think it's worth it. LMK what you think

It's a good compromise. It's unfortunate that we can't encapsulate all this logic into the original showAlert function, since that was simple to use & test. But then we'd be making extra API calls that might not be necessary, so this works well.

smith · 2020-07-27T20:14:48Z

x-pack/plugins/apm/public/components/shared/Links/apm/AnomalyDetectionSetupLink.test.tsx

-    it('should return true when there are no jobs', () => {
-      const result = showAlert(dataWithoutJobs, undefined);
-      expect(result).toBe(true);
+  it('when no jobs exists for the selected environment - it should show warning', async () => {


We should use describe('when no jobs exists for the selected environment', () => { it('shows a warning', () => >..

…tion-api-tests

dgieselaar · 2020-07-27T20:29:21Z

@sqren I've got some feedback pending, give me a couple hours tomorrow before merging

Waiting on other reviewers.

ogupte · 2020-07-27T21:36:56Z

x-pack/test/apm_api_integration/trial/tests/settings/anomaly_detection/write_user.ts

+        await createJobs(['production', 'staging']);
+
+        const { body } = await getJobs();
+        expect(body.hasLegacyJobs).to.be(false);


since we don't have any legacy jobs in the test data, we can't test that part of the response here. But i think it's ok to do this in a unit test since it's only testing for a particular pattern in the a job id.

dgieselaar

I'll submit the feedback that I have for now - I'm waiting on ML to work on the obs test clusters to complete my review.

dgieselaar · 2020-07-27T14:35:45Z

x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts

@@ -29,16 +29,12 @@ export async function createAnomalyDetectionJobs(
  const { ml, indices } = setup;

  if (!ml) {
-    throw new AnomalyDetectionError(ErrorCode.ML_NOT_AVAILABLE);
+    throw Boom.serverUnavailable(ML_ERRORS.ML_NOT_AVAILABLE);


I'm not sure if a 503 is appropriate here. 503 implies that it is a temporary error, and should be retried. Maybe a 500 makes more sense?

dgieselaar · 2020-07-27T14:40:13Z

x-pack/plugins/apm/server/lib/anomaly_detection/get_anomaly_detection_jobs.ts

  if (!ml) {
-    return [];
+    throw Boom.serverUnavailable(ML_ERRORS.ML_NOT_AVAILABLE);


See other comment, we could consider a 500 instead of a 503.

I think 501 is probably better than 500 or 503. From the RFC:

10.5.1 500 Internal Server Error
The server encountered an unexpected condition which prevented it from fulfilling the request.

10.5.2 501 Not Implemented
The server does not support the functionality required to fulfill the request. This is the appropriate response when the server does not recognize the request method and is not capable of supporting it for any resource.

dgieselaar · 2020-07-27T14:40:29Z

x-pack/plugins/apm/server/lib/anomaly_detection/has_legacy_jobs.ts

@@ -12,7 +15,12 @@ export async function hasLegacyJobs(setup: Setup) {
  const { ml } = setup;

  if (!ml) {
-    return false;
+    throw Boom.serverUnavailable(ML_ERRORS.ML_NOT_AVAILABLE);


See other comment, we could consider a 500 instead of a 503.

x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts

dgieselaar · 2020-07-27T14:45:50Z

x-pack/plugins/apm/server/lib/service_map/get_service_anomalies.ts

@@ -31,29 +35,14 @@ export async function getServiceAnomalies({
  const { ml, start, end } = setup;

  if (!ml) {
-    logger.warn('Anomaly detection plugin is not available.');
-    return DEFAULT_ANOMALIES;
+    throw Boom.serverUnavailable(ML_ERRORS.ML_NOT_AVAILABLE);


Can we perhaps handle this in the route, and use a helper function that will either throw these errors or refine { ml?: ... } to { ml: ... }?

That's a good idea but I'm going to leave these as-is to avoid too many changes this late. If you really think they're needed mark this PR with changes requested and I can get them in.

x-pack/test/apm_api_integration/basic/tests/settings/anomaly_detection/write_user.ts

dgieselaar · 2020-07-27T18:02:23Z

x-pack/test/apm_api_integration/basic/tests/settings/anomaly_detection/write_user.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+/*


Do you think that this test might be enough? i.e., if this fails I think it's safe to assume that other users (read, no access) won't have access either.

You're right but I think I'll just leave these for now and err on the side of having too many tests.

dgieselaar · 2020-07-27T18:02:59Z

x-pack/plugins/apm/server/routes/settings/anomaly_detection.ts

-      return {
-        errorCode: mlErrorCode,
-      };
+      throw e;


do we need the log before this line if the route handler will throw anyway?

dgieselaar · 2020-07-27T18:03:32Z

x-pack/plugins/apm/server/routes/settings/anomaly_detection.ts

+      context.logger.warn(
+        `Error while retrieving ML jobs: "${e.name}:${e.message}"`
+      );


Should we log if the route throws?

I don't follow what you're saying here.

From the comment below I think you mean let's remove the try/catch and logging since we'll get the error anyway if it fails.

x-pack/plugins/apm/public/components/shared/Links/apm/AnomalyDetectionSetupLink.tsx

…tion-api-tests

kibanamachine · 2020-07-30T01:54:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: f7b3772

Build metrics

async chunks size

id	value	diff	baseline
apm	3.7MB	-721.0B	3.7MB

History

💚 Build #65392 succeeded 6640c14
💚 Build #64496 succeeded c8cac1b
💚 Build #64237 succeeded 744373f
💔 Build #64232 failed ba7fac45e1704943d4e9cdf33cd355af22ca09e9
💚 Build #64211 succeeded d16f3e8d29268a9e69bbe5e348f4eae5ba96c754

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

Co-authored-by: Nathan L Smith <[email protected]>

kibanamachine · 2020-07-30T14:10:57Z

💔 Backport was not successful

The PR was attempted backported to the following branches:

✅ 7.x
❌ 7.9: Commit could not be cherrypicked due to conflicts

Co-authored-by: Nathan L Smith <[email protected]> # Conflicts: # x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/jobs_list.tsx

Co-authored-by: Nathan L Smith <[email protected]> Co-authored-by: Søren Louv-Jansen <[email protected]>

sorenlouv requested a review from a team as a code owner July 23, 2020 19:07

botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Jul 23, 2020

sorenlouv added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.9.0 labels Jul 23, 2020

sorenlouv commented Jul 23, 2020

View reviewed changes

sorenlouv commented Jul 24, 2020

View reviewed changes

x-pack/plugins/apm/public/components/shared/Links/apm/AnomalyDetectionSetupLink.tsx Show resolved Hide resolved

sorenlouv force-pushed the add-anomaly-detection-api-tests branch 2 times, most recently from de82143 to a32d817 Compare July 24, 2020 23:17

sorenlouv commented Jul 24, 2020

View reviewed changes

sorenlouv commented Jul 25, 2020

View reviewed changes

sorenlouv force-pushed the add-anomaly-detection-api-tests branch from d16f3e8 to d7735c4 Compare July 26, 2020 22:14

sorenlouv commented Jul 26, 2020

View reviewed changes

sorenlouv force-pushed the add-anomaly-detection-api-tests branch from d7735c4 to ba7fac4 Compare July 26, 2020 22:17

[APM] Add anomaly detection API tests

744373f

sorenlouv force-pushed the add-anomaly-detection-api-tests branch from ba7fac4 to 744373f Compare July 27, 2020 05:54

dgieselaar self-requested a review July 27, 2020 13:55

smith reviewed Jul 27, 2020

View reviewed changes

smith previously approved these changes Jul 27, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into add-anomaly-detec…

e9f7b88

…tion-api-tests

add more describes

c8cac1b

smith self-requested a review July 27, 2020 20:38

ogupte changed the title ~~[APM] Add anomaly detection API tests~~ [APM] Add anomaly detection API and UI unit tests Jul 27, 2020

ogupte changed the title ~~[APM] Add anomaly detection API and UI unit tests~~ [APM] Add anomaly detection API tests + fixes Jul 27, 2020

ogupte reviewed Jul 27, 2020

View reviewed changes

dgieselaar reviewed Jul 28, 2020

View reviewed changes

dgieselaar reviewed Jul 29, 2020

View reviewed changes

x-pack/plugins/apm/public/components/shared/Links/apm/AnomalyDetectionSetupLink.tsx Outdated Show resolved Hide resolved

dgieselaar reviewed Jul 29, 2020

View reviewed changes

x-pack/plugins/apm/public/components/shared/Links/apm/AnomalyDetectionSetupLink.tsx Show resolved Hide resolved

smith added 4 commits July 29, 2020 16:26

Use 501; remove duplicate license headers

ee18624

Merge remote-tracking branch 'upstream/master' into add-anomaly-detec…

6640c14

…tion-api-tests

More updates based on feedback

a777f41

Merge remote-tracking branch 'upstream/master' into add-anomaly-detec…

f7b3772

…tion-api-tests

dgieselaar approved these changes Jul 30, 2020

View reviewed changes

smith merged commit aa68e3b into elastic:master Jul 30, 2020

kibanamachine pushed a commit that referenced this pull request Jul 30, 2020

[APM] Add anomaly detection API tests + fixes (#73120)

494307b

Co-authored-by: Nathan L Smith <[email protected]>

kibanamachine mentioned this pull request Jul 30, 2020

[7.x] [APM] Add anomaly detection API tests + fixes (#73120) #73806

Merged

smith mentioned this pull request Jul 30, 2020

[7.9] [APM] Add anomaly detection API tests + fixes (#73120) #73824

Merged

smith pushed a commit that referenced this pull request Jul 30, 2020

[7.x] [APM] Add anomaly detection API tests + fixes (#73120) (#73806)

fab1ef2

smith added a commit that referenced this pull request Jul 30, 2020

[7.9] [APM] Add anomaly detection API tests + fixes (#73120) (#73824)

ee176cf

Co-authored-by: Nathan L Smith <[email protected]> Co-authored-by: Søren Louv-Jansen <[email protected]>

sorenlouv deleted the add-anomaly-detection-api-tests branch August 15, 2020 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Add anomaly detection API tests + fixes #73120

[APM] Add anomaly detection API tests + fixes #73120

sorenlouv commented Jul 23, 2020 •

edited by ogupte

Loading

elasticmachine commented Jul 23, 2020

sorenlouv Jul 23, 2020 •

edited

Loading

sorenlouv Jul 24, 2020 •

edited

Loading

sorenlouv Jul 24, 2020 •

edited

Loading

sorenlouv Jul 25, 2020 •

edited

Loading

dgieselaar Jul 27, 2020

sorenlouv Jul 25, 2020 •

edited

Loading

dgieselaar Jul 27, 2020

sorenlouv Jul 25, 2020 •

edited

Loading

sorenlouv Jul 25, 2020 •

edited

Loading

sorenlouv Jul 26, 2020

ogupte Jul 27, 2020

smith Jul 27, 2020

dgieselaar commented Jul 27, 2020

ogupte Jul 27, 2020

dgieselaar left a comment

dgieselaar Jul 27, 2020

dgieselaar Jul 27, 2020

smith Jul 29, 2020

dgieselaar Jul 27, 2020

dgieselaar Jul 27, 2020

smith Jul 29, 2020

dgieselaar Jul 27, 2020

smith Jul 29, 2020

dgieselaar Jul 27, 2020

dgieselaar Jul 27, 2020

smith Jul 29, 2020

smith Jul 29, 2020

kibanamachine commented Jul 30, 2020

kibanamachine commented Jul 30, 2020

[APM] Add anomaly detection API tests + fixes #73120

[APM] Add anomaly detection API tests + fixes #73120

Conversation

sorenlouv commented Jul 23, 2020 • edited by ogupte Loading

elasticmachine commented Jul 23, 2020

sorenlouv Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

sorenlouv Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

sorenlouv Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

sorenlouv Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

sorenlouv Jul 25, 2020 • edited Loading

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

dgieselaar commented Jul 27, 2020

Choose a reason for hiding this comment

dgieselaar 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

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

kibanamachine commented Jul 30, 2020

💚 Build Succeeded

Build metrics

async chunks size

History

kibanamachine commented Jul 30, 2020

💔 Backport was not successful

sorenlouv commented Jul 23, 2020 •

edited by ogupte

Loading

sorenlouv Jul 23, 2020 •

edited

Loading

sorenlouv Jul 24, 2020 •

edited

Loading

sorenlouv Jul 24, 2020 •

edited

Loading

sorenlouv Jul 25, 2020 •

edited

Loading

sorenlouv Jul 25, 2020 •

edited

Loading

sorenlouv Jul 25, 2020 •

edited

Loading

sorenlouv Jul 25, 2020 •

edited

Loading