[APM] Service maps anomaly detection status in popover #65217

ogupte · 2020-05-05T02:28:18Z

Partially addresses #64205 and closes #64278 & #65244.

Adds ML status to the service maps popover. Modifies the query a bit to get the actual vs typical values necessary to generate a proper anomaly description

get the actual vs typical values necessary to generate a proper anomaly description

ogupte · 2020-05-05T04:25:12Z

The main difference with the existing query was that i filtered by result_type: 'record' rather than checking for documents with bucket_span. The reason is that records have record_score which is the same as anomaly_score as well as the actual and typical fields which are necessary to calculate the description text (1.1x higher, 4x higher, etc.). I also pass along the job_id along with the anomaly data so that we can link directly to ML from service maps without requiring any knowledge of anomalous transaction type.

formgeist · 2020-05-05T07:54:33Z

@ogupte Great to see this in 🙌 I have some immediate feedback on the link and the value display;

We should change the link copy to "View anomalies" since we're linking to the time series explorer, not the anomaly explorer, which in hindsight I think is the right choice, but we need to change the label.
~~The link should open a new tab/window.~~ You fixed this as I was writing this 😉
The anomaly score should be rounded without a decimal point value.

If it's possible while you're in there, could you please remove the framework badge? #64278 - it will make the content of the popover a little easier on the eyes.

- changes anomaly score display to integer formatting - update link test to 'View anomalies'

jgowdyelastic

ML changes LGTM

x-pack/plugins/apm/public/components/app/ServiceMap/Popover/Contents.tsx

sorenlouv · 2020-05-05T10:45:08Z

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

+                sort: [
+                  {
+                    record_score: {
+                      order: 'desc' as const
+                    }
+                  }
+                ],


Suggested change

sort: [

{

record_score: {

order: 'desc' as const

}

}

],

sort: [{ record_score: { order: 'desc' as const } }],

sorenlouv · 2020-05-05T10:59:58Z

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

+            term: {
+              result_type: 'record'
+            }
+          }


Two things:

shouldn't the query be limited by a couple of things (environment*, date range etc.)? Was this left out intentionally in the previous PR (cc @smith )?

What is record_score exactly? Is the the record within a time range? Or the highest score ever recorded? We can only use it if it's the highest score within the time range we are querying for

* it doesn't seem like ML jobs take environment into account (only service name and transaction type). This seems like a big deal if i'm not mistaken.

we can make sure to add a date range filter, but i don't think we can filter by environment currently since it's not retained in the ml index.

this is a case where record_score seems appropriate to use after reading https://www.elastic.co/blog/machine-learning-anomaly-scoring-elasticsearch-how-it-works last paragraph

formgeist · 2020-05-05T11:15:39Z

@ogupte Not sure if this is feasible or not, but I opened a design issue for adding a message when anomaly detection job is not available for a given service, indicating to the user that they can create them in the Service details view. #65244 This is is to mitigate any confusion and because we cannot supply users with a flow to enable it from within the Service maps UI.

…nomalies detected - removes unecessary service framework name in service map queries - adds date range filter for anomaly detection

ogupte · 2020-05-05T17:01:52Z

x-pack/plugins/apm/public/components/app/ServiceMap/Cytoscape.tsx

+      // ensure all elements get latest data properties
+      elements.forEach(elementDefinition => {
+        const el = cy.getElementById(elementDefinition.data.id as string);
+        el.data(elementDefinition.data);
+      });


This was necessary to add since the data of each node wasn't being updated when existing elements were re-added. Before adding this, the displayed anomaly data was stale after updating date ranges.

kibanamachine · 2020-05-05T18:47:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 56b81f0

History

💚 Build #45411 succeeded 2f8f296
💛 Build #45384 was flaky ccf4bb8
💔 Build #45366 failed 4358232

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

* Adds ML status to the service maps popover. Modifies the query a bit to get the actual vs typical values necessary to generate a proper anomaly description * fixed failures and updated tests * component clean up * makes the ML link open in a new window * - Closes elastic#64278 by removing the framework badge. - changes anomaly score display to integer formatting - update link test to 'View anomalies' * - Closes elastic#65244 by displaying a message for services without anomalies detected - removes unecessary service framework name in service map queries - adds date range filter for anomaly detection

* Adds ML status to the service maps popover. Modifies the query a bit to get the actual vs typical values necessary to generate a proper anomaly description * fixed failures and updated tests * component clean up * makes the ML link open in a new window * - Closes #64278 by removing the framework badge. - changes anomaly score display to integer formatting - update link test to 'View anomalies' * - Closes #65244 by displaying a message for services without anomalies detected - removes unecessary service framework name in service map queries - adds date range filter for anomaly detection

sorenlouv · 2020-05-05T23:57:21Z

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

+    if (serviceAnomalies) {
+      const maxScore = serviceAnomalies.max_score;
+      return {
+        ...service,
+        max_score: maxScore,
+        severity: getSeverity(maxScore),
+        actual_value: serviceAnomalies.actual_value,
+        typical_value: serviceAnomalies.typical_value,
+        job_id: serviceAnomalies.job_id
+      };
+    }
+    return service;


I don't think we need the conditional here since serviceAnomalies is not optional:

const servicesDataWithAnomalies = servicesData.map(service => { const serviceAnomalies = anomaliesMap[service[SERVICE_NAME]]; const maxScore = serviceAnomalies.max_score; return { ...service, max_score: maxScore, severity: getSeverity(maxScore), actual_value: serviceAnomalies.actual_value, typical_value: serviceAnomalies.typical_value, job_id: serviceAnomalies.job_id }; });

sorenlouv · 2020-05-06T00:04:05Z

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

-  children
-}) => {
-  const jobId = getMlJobId(serviceName, transactionType);
+export const MLJobLink: React.FC<Props> = props => {


In this case, are there any advantages to having a union over making some of the props optional?

interface Props { serviceName: string; transactionType?: string; jobId?: string; external?: boolean; } export const MLJobLink: React.FC<Props> = props => { const jobId = props.jobId ? props.jobId : getMlJobId(props.serviceName, props.transactionType);

sorenlouv · 2020-05-06T00:11:42Z

x-pack/plugins/apm/public/components/app/ServiceMap/Popover/Contents.tsx

+  const maxScore = selectedNodeData.max_score;
+  const actualValue = selectedNodeData.actual_value;
+  const typicalValue = selectedNodeData.typical_value;
+  const jobId = selectedNodeData.job_id;


These are all any because selectedNodeData is vaguely typed. Currently:

selectedNodeData: cytoscape.NodeDataDefinition;

Should be changed to something like:

selectedNodeData: ServiceNode;

where ServiceNode an intersection type:

type ServiceNode = cytoscape.NodeDataDefinition & TypeWithAnomalyDetectionFields

I expect TypeWithAnomalyDetectionFields is defined somewhere on the backend like:

interface TypeWithAnomalyDetectionFields { severity?: string; maxScore?: string; ... }

(or perhaps it can be inferred from a ReturnType)

sorenlouv · 2020-05-06T00:25:08Z

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

@@ -129,38 +124,32 @@ async function getServicesData(options: IEnvOptions) {
        [SERVICE_NAME]: bucket.key as string,
        [AGENT_NAME]:
          (bucket.agent_name.buckets[0]?.key as string | undefined) || '',


It seems like a hack to default agent name to an empty string (''). What's wrong with making it optional and handle that explicitly downstream?

sorenlouv · 2020-05-06T00:28:32Z

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

      };
    }) || []
  );
 }

 function getAnomaliesData(options: IEnvOptions) {
-  const { client } = options.setup;
+  const { start, end, client } = options.setup;
+  const rangeQuery = { range: rangeFilter(start, end, 'timestamp') };


I don't think that extracting rangeQuery from the es query improves the readability. Additionally, what about just writing the query out without the helper?

{ range: { timestamp: { gte: start, lte: end, format: 'epoch_millis' } } }

I think the helper is best if it's kept super simple, and only solves one case. if we extend it and try to make it more generic it loses its value.

cauemarcondes · 2020-05-14T09:04:27Z

Tested:

Chrome ✅
FF ✅
Safari ✅

Adds ML status to the service maps popover. Modifies the query a bit to

4358232

get the actual vs typical values necessary to generate a proper anomaly description

ogupte added release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels May 5, 2020

fixed failures and updated tests

7751307

ogupte marked this pull request as ready for review May 5, 2020 04:25

ogupte requested review from a team as code owners May 5, 2020 04:25

ogupte added 2 commits May 4, 2020 21:48

component clean up

ccf4bb8

makes the ML link open in a new window

d56debc

- Closes elastic#64278 by removing the framework badge.

2f8f296

- changes anomaly score display to integer formatting - update link test to 'View anomalies'

jgowdyelastic approved these changes May 5, 2020

View reviewed changes

sorenlouv reviewed May 5, 2020

View reviewed changes

x-pack/plugins/apm/public/components/app/ServiceMap/Popover/Contents.tsx Show resolved Hide resolved

sorenlouv reviewed May 5, 2020

View reviewed changes

- Closes elastic#65244 by displaying a message for services without a…

56b81f0

…nomalies detected - removes unecessary service framework name in service map queries - adds date range filter for anomaly detection

ogupte commented May 5, 2020

View reviewed changes

ogupte requested a review from sorenlouv May 5, 2020 18:48

smith approved these changes May 5, 2020

View reviewed changes

ogupte merged commit 4896b65 into elastic:master May 5, 2020

ogupte mentioned this pull request May 5, 2020

[7.x] [APM] Service maps anomaly detection status in popover (#65217) #65345

Merged

sorenlouv reviewed May 5, 2020

View reviewed changes

sorenlouv reviewed May 6, 2020

View reviewed changes

sorenlouv added the apm:test-plan-7.8.0 label May 13, 2020

cauemarcondes self-assigned this May 13, 2020

cauemarcondes added the apm:test-plan-done Pull request that was successfully tested during the test plan label May 14, 2020

sorenlouv mentioned this pull request Jun 8, 2020

[APM] Service maps - Fix missing ML status for services with jobs but no anomalies #68486

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Service maps anomaly detection status in popover #65217

[APM] Service maps anomaly detection status in popover #65217

ogupte commented May 5, 2020 •

edited

Loading

ogupte commented May 5, 2020

formgeist commented May 5, 2020 •

edited

Loading

jgowdyelastic left a comment

sorenlouv May 5, 2020

sorenlouv May 5, 2020 •

edited

Loading

ogupte May 5, 2020

formgeist commented May 5, 2020

ogupte May 5, 2020 •

edited

Loading

kibanamachine commented May 5, 2020

sorenlouv May 5, 2020 •

edited

Loading

sorenlouv May 6, 2020 •

edited

Loading

sorenlouv May 6, 2020 •

edited

Loading

sorenlouv May 6, 2020 •

edited

Loading

sorenlouv May 6, 2020 •

edited

Loading

cauemarcondes commented May 14, 2020

[APM] Service maps anomaly detection status in popover #65217

[APM] Service maps anomaly detection status in popover #65217

Conversation

ogupte commented May 5, 2020 • edited Loading

ogupte commented May 5, 2020

formgeist commented May 5, 2020 • edited Loading

jgowdyelastic left a comment

Choose a reason for hiding this comment

sorenlouv May 5, 2020

Choose a reason for hiding this comment

sorenlouv May 5, 2020 • edited Loading

Choose a reason for hiding this comment

ogupte May 5, 2020

Choose a reason for hiding this comment

formgeist commented May 5, 2020

ogupte May 5, 2020 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented May 5, 2020

💚 Build Succeeded

History

sorenlouv May 5, 2020 • edited Loading

Choose a reason for hiding this comment

sorenlouv May 6, 2020 • edited Loading

Choose a reason for hiding this comment

sorenlouv May 6, 2020 • edited Loading

Choose a reason for hiding this comment

sorenlouv May 6, 2020 • edited Loading

Choose a reason for hiding this comment

sorenlouv May 6, 2020 • edited Loading

Choose a reason for hiding this comment

cauemarcondes commented May 14, 2020

ogupte commented May 5, 2020 •

edited

Loading

formgeist commented May 5, 2020 •

edited

Loading

sorenlouv May 5, 2020 •

edited

Loading

ogupte May 5, 2020 •

edited

Loading

sorenlouv May 5, 2020 •

edited

Loading

sorenlouv May 6, 2020 •

edited

Loading

sorenlouv May 6, 2020 •

edited

Loading

sorenlouv May 6, 2020 •

edited

Loading

sorenlouv May 6, 2020 •

edited

Loading