[APM] Transaction duration anomaly alerting integration #75719

ogupte · 2020-08-22T07:42:59Z

Adds alerting integration for APM transaction duration anomalies.

…uration anomalies.

elasticmachine · 2020-08-22T07:43:05Z

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

sorenlouv · 2020-08-25T12:56:13Z

x-pack/plugins/apm/public/components/app/ServiceDetails/index.tsx

+  const canReadAnomalies = !!(
+    isMlPluginEnabled &&
+    capabilities.ml.canAccessML &&
+    capabilities.ml.canGetJobs
+  );


nit: looks like all three are booleans so there should be no reason to coerce with !!

I added the double negative because the logical and expression returns the type of the last operand (capabilities.ml.canGetJobs) which is: boolean | Readonly<{[x: string]: boolean;}>. Without the double NOT operator, canReadAnomalies fails the type assignment for the canReadAnomalies prop of the AlertIntegrations component.

...pm/public/components/shared/TransactionDurationAnomalyAlertTrigger/SelectAnomalySeverity.tsx

x-pack/plugins/apm/public/components/shared/TransactionDurationAnomalyAlertTrigger/index.tsx

sorenlouv · 2020-08-25T14:21:51Z

x-pack/plugins/apm/public/components/shared/TransactionDurationAnomalyAlertTrigger/index.tsx

+        onChange={(e) =>
+          setAlertParams(
+            'transactionType',
+            e.target.value as Params['transactionType']


Would be nice to avoid as Params['transactionType']. Is it because e.target.value can be other types than string?

We can probably safely avoid the type assertion. It's a holdover from TransactionDurationAlertTrigger which this component was based on.

sorenlouv · 2020-08-25T14:23:37Z

x-pack/plugins/apm/server/lib/alerts/register_transaction_duration_anomaly_alert_type.ts

+      }
+      const alertParams = params as TypeOf<typeof paramsSchema>;
+      const mlClient = services.getLegacyScopedClusterClient(ml.mlClient);
+      const request = { params: 'DummyKibanaRequest' } as KibanaRequest;


Is "DummyKibanaRequest" some leftover boilerplate?

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

sorenlouv · 2020-08-25T14:49:38Z

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

  // to filter out legacy jobs we are filtering by the existence of `apm_ml_version` in `custom_settings`
  // and checking that it is compatable.
  const mlJobs = response.jobs.filter(
    (job) => (job.custom_settings?.job_tags?.apm_ml_version ?? 0) >= 2
  );
-  if (environment) {
+  if (environment && environment !== ENVIRONMENT_ALL) {


Was it a bug that we didn't take this into account previously?
also: this would go away if we got rid of "all" envs.

This wasn't a bug before since the environment value was omitted (in the uiFilters object) when "All" was the selected environment filter. If we get rid of the "All" as a environment filter option from the alert creation overlay, then we can get rid of this. We could also omit this check if we do this check in the alert executor function up the call stack.

sorenlouv · 2020-08-25T14:57:40Z

x-pack/plugins/ml/server/shared_services/providers/anomaly_detectors.ts

+      //  APM is using this service in anomaly alert, kibana alerting doesn't provide request object
+      // So we are adding a dummy request for now
+      // TODO: Remove this once kibana alerting provides request object
+      const hasMlCapabilities =
+        request.params !== 'DummyKibanaRequest'
+          ? getHasMlCapabilities(request)
+          : (_caps: string[]) => Promise.resolve();


Okay, I see now why you used "DummyKibanaRequest". This doesn't seem like the right approach. Either request should be optional or we should not use anomalyDetectorsProvider. Passing in a dummy request object is going to cause confusion down the line I think.

DummyKibanaRequest will not be needed after this PR is merged #74965
However a fake request object (e.g. {} as KibanaRequest) will still be needed until #64588 is fixed.
Making request optional will require more changes to undo our work arounds once this issue is resolved.

@jgowdyelastic do i need to hold off on merging this PR until yours is merged first? LMK if i should make any changes to the DummyKibanaRequest pattern in the meantime.

that PR is a few days away from being merged. so i'm happy incorporate and update the changes in this PR if you merge first.

…-detection-based-alerts

…d of the anomaly score.

…on/environment_filter_values - utilize getEnvironmentLabel in the alerting trigger components and added support for the 'All' label

x-pack/plugins/apm/common/environment_filter_values.ts

sorenlouv · 2020-08-26T09:57:06Z

...pm/public/components/shared/TransactionDurationAnomalyAlertTrigger/SelectAnomalySeverity.tsx

+      {label}
+    </EuiHealth>
+  );
+}


Nice with a dedicated component 👍

formgeist · 2020-08-26T11:20:00Z

A possible bug: I noticed that the alerts popover doesn't close when the flyout is displayed.

I understand that because the anomaly detection job is on the environment, we have to write out the service name in the condition, but I also find it not useful for the end-user. The alerts are only currently available within the selected service view, so unless we're thinking of adding this alert type to the general APM view (no service selected), I think we can do with hiding it.

Example from duration threshold alert

Existing duration anomaly alert

sorenlouv

Overall this lgtm.
Would be good if we can find a better solution than the DummyKibanaRequest but not something blocking this PR

jgowdyelastic

ML changes LGTM

…ent and clean

- default environment alert settings to the selected filter

… anomaly detection jobs - Removes Service name and transaction type from the set of expressions in the alerting setup

ogupte · 2020-08-27T00:16:50Z

@formgeist, i reorganized the alerting menu to better support different alerting types for specific metrics:

ogupte · 2020-08-27T05:12:52Z

@elasticmachine merge upstream

formgeist · 2020-08-27T07:11:08Z

@formgeist, i reorganized the alerting menu to better support different alerting types for specific metrics:

Looks good, I think this makes sense. Minor nit; would you remove the icons from the "Create..." options inside the nested menu? They don't really belong there any longer.

ogupte · 2020-08-27T07:51:25Z

@formgeist you suggested removing the service name read-only expression in the transaction duration anomaly alert setup. I also did the same for the transaction type since it will always be either request or page-load depending on the service. LMK if you think we should change it back:

formgeist · 2020-08-27T08:28:49Z

@ogupte I had a think after our meeting yesterday, and this morning I put together some proposals for improvements. I don't want to unnecessarily block this PR, since what we have works. But in regards to the service name expression, I've suggested to keep it in order to allow for broader scoping of the alert. We discussed this yesterday briefly, but I've materialized the UX a bit more in my proposal #76068 Look forward to hear everyone's thoughts on these improvements.

… expression

kibanamachine · 2020-08-27T23:20:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9d3a35a

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
apm	945	+2	943

async chunks size

id	value	diff	baseline
apm	4.7MB	+25.8KB	4.7MB

page load bundle size

id	value	diff	baseline
apm	41.7KB	+1.1KB	40.6KB

distributable file count

id	value	diff	baseline
total	53244	+9	53235

History

💚 Build #70789 succeeded 7ec5155
💚 Build #70762 succeeded 936ae05
💔 Build #70742 failed b1f3b25
💔 Build #70444 failed 94c3b48
💚 Build #69862 succeeded ec08dca

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

* Closes elastic#72636. Adds alerting integration for APM transaction duration anomalies. * Code review feedback * Display alert summary with the selected anomaly severity label instead of the anomaly score. * - refactored ALL_OPTION and NOT_DEFINED_OPTION to be shared from common/environment_filter_values - utilize getEnvironmentLabel in the alerting trigger components and added support for the 'All' label * refactor get_all_environments to minimize exports and be more consistent and clean * - Reorg the alerts menu for different alert types (threshold/anomaly) - default environment alert settings to the selected filter * - Filters default transaction type to only those supported in the APM anomaly detection jobs - Removes Service name and transaction type from the set of expressions in the alerting setup * - remove bell icon from alerts menu * Adds target service back into the anomaly alert setup as a ready-only expression Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

#76224) * [APM] Transaction duration anomaly alerting integration (#75719) * Closes #72636. Adds alerting integration for APM transaction duration anomalies. * Code review feedback * Display alert summary with the selected anomaly severity label instead of the anomaly score. * - refactored ALL_OPTION and NOT_DEFINED_OPTION to be shared from common/environment_filter_values - utilize getEnvironmentLabel in the alerting trigger components and added support for the 'All' label * refactor get_all_environments to minimize exports and be more consistent and clean * - Reorg the alerts menu for different alert types (threshold/anomaly) - default environment alert settings to the selected filter * - Filters default transaction type to only those supported in the APM anomaly detection jobs - Removes Service name and transaction type from the set of expressions in the alerting setup * - remove bell icon from alerts menu * Adds target service back into the anomaly alert setup as a ready-only expression Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * Fixes prettier linting * Fixes prettier linting again Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…s-for-710 * 'master' of github.com:elastic/kibana: (43 commits) [APM] Chart units don't update when toggling the chart legends (elastic#74931) [ILM] Add support for frozen phase in UI (elastic#75968) Hides advanced json for count metric (elastic#74636) add client-side feature usage API (elastic#75486) [Maps] add drilldown support map embeddable (elastic#75598) [Enterprise Search] Request handler refactors/enhancements + update existing routes to use shared handler (elastic#76106) [Resolver] model `location.search` in redux (elastic#76140) [APM] Prevent imports of public in server code (elastic#75979) fix eslint issue skip flaky suite (elastic#76223) [APM] Transaction duration anomaly alerting integration (elastic#75719) [Transforms] Avoid using "Are you sure" (elastic#75932) [Security Solution][Exceptions] - Fix bug of alerts not updating after closure from exceptions modal (elastic#76145) [plugin-helpers] improve 3rd party KP plugin support (elastic#75019) [docs/getting-started] link to yarn v1 specifically (elastic#76169) [Security_Solution][Resolver] Resolver loading and error state (elastic#75600) Fixes App Search documentation links (elastic#76133) Fix alerts unable to create / update when the name has trailing whitepace(s) (elastic#76079) [Resolver] Fix useSelector usage (elastic#76129) [Enterprise Search] Migrate util and components from ent-search (elastic#76051) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/data_tier_allocation/node_allocation.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/services/policies/types.ts # x-pack/plugins/index_lifecycle_management/public/application/services/policies/warm_phase.ts

cauemarcondes · 2020-10-13T07:56:35Z

Tests ok

Closes elastic#72636. Adds alerting integration for APM transaction d…

ec08dca

…uration anomalies.

ogupte added release_note:roadmap v7.10.0 labels Aug 22, 2020

ogupte requested review from a team as code owners August 22, 2020 07:42

botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 22, 2020

This comment has been minimized.

Sign in to view

sorenlouv reviewed Aug 25, 2020

View reviewed changes

...pm/public/components/shared/TransactionDurationAnomalyAlertTrigger/SelectAnomalySeverity.tsx Outdated Show resolved Hide resolved

sorenlouv reviewed Aug 25, 2020

View reviewed changes

x-pack/plugins/apm/public/components/shared/TransactionDurationAnomalyAlertTrigger/index.tsx Outdated Show resolved Hide resolved

sorenlouv reviewed Aug 25, 2020

View reviewed changes

x-pack/plugins/apm/server/lib/service_map/get_service_anomalies.ts Outdated Show resolved Hide resolved

sorenlouv reviewed Aug 25, 2020

View reviewed changes

ogupte added 4 commits August 25, 2020 22:37

Merge remote-tracking branch 'upstream/master' into apm-72636-anomaly…

717d296

…-detection-based-alerts

Code review feedback

ac4bde1

Display alert summary with the selected anomaly severity label instea…

dd31e8b

…d of the anomaly score.

- refactored ALL_OPTION and NOT_DEFINED_OPTION to be shared from comm…

94c3b48

…on/environment_filter_values - utilize getEnvironmentLabel in the alerting trigger components and added support for the 'All' label

sorenlouv reviewed Aug 26, 2020

View reviewed changes

x-pack/plugins/apm/common/environment_filter_values.ts Outdated Show resolved Hide resolved

sorenlouv reviewed Aug 26, 2020

View reviewed changes

x-pack/plugins/apm/common/environment_filter_values.ts Outdated Show resolved Hide resolved

sorenlouv reviewed Aug 26, 2020

View reviewed changes

sorenlouv approved these changes Aug 26, 2020

View reviewed changes

jgowdyelastic approved these changes Aug 26, 2020

View reviewed changes

ogupte added 3 commits August 26, 2020 15:41

refactor get_all_environments to minimize exports and be more consist…

c798812

…ent and clean

- Reorg the alerts menu for different alert types (threshold/anomaly)

d19427c

- default environment alert settings to the selected filter

- Filters default transaction type to only those supported in the APM…

b1f3b25

… anomaly detection jobs - Removes Service name and transaction type from the set of expressions in the alerting setup

Merge branch 'master' into apm-72636-anomaly-detection-based-alerts

936ae05

- remove bell icon from alerts menu

7ec5155

formgeist mentioned this pull request Aug 27, 2020

[APM] Improvements to transaction duration anomaly alert flow #76068

Closed

Adds target service back into the anomaly alert setup as a ready-only…

9d3a35a

… expression

ogupte merged commit b1cb8b8 into elastic:master Aug 28, 2020

ogupte mentioned this pull request Aug 28, 2020

[7.x] [APM] Transaction duration anomaly alerting integration (#75719) #76224

Merged

sorenlouv added the apm:test-plan-7.10.0 label Oct 7, 2020

cauemarcondes self-assigned this Oct 13, 2020

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

bmorelli25 mentioned this pull request Oct 15, 2020

[docs] 7.10 APM docs updates #80605

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Transaction duration anomaly alerting integration #75719

[APM] Transaction duration anomaly alerting integration #75719

ogupte commented Aug 22, 2020 •

edited

Loading

elasticmachine commented Aug 22, 2020

This comment has been minimized.

sorenlouv Aug 25, 2020

ogupte Aug 26, 2020

sorenlouv Aug 25, 2020

ogupte Aug 26, 2020

sorenlouv Aug 25, 2020 •

edited

Loading

sorenlouv Aug 25, 2020

ogupte Aug 26, 2020

sorenlouv Aug 25, 2020

jgowdyelastic Aug 25, 2020 •

edited

Loading

ogupte Aug 26, 2020 •

edited

Loading

jgowdyelastic Aug 26, 2020

sorenlouv Aug 26, 2020

formgeist commented Aug 26, 2020

sorenlouv left a comment

jgowdyelastic left a comment

ogupte commented Aug 27, 2020

ogupte commented Aug 27, 2020

formgeist commented Aug 27, 2020

ogupte commented Aug 27, 2020

formgeist commented Aug 27, 2020

kibanamachine commented Aug 27, 2020

cauemarcondes commented Oct 13, 2020

[APM] Transaction duration anomaly alerting integration #75719

[APM] Transaction duration anomaly alerting integration #75719

Conversation

ogupte commented Aug 22, 2020 • edited Loading

elasticmachine commented Aug 22, 2020

This comment has been minimized.

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

sorenlouv Aug 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

jgowdyelastic Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

ogupte Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

formgeist commented Aug 26, 2020

sorenlouv left a comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

ogupte commented Aug 27, 2020

ogupte commented Aug 27, 2020

formgeist commented Aug 27, 2020

ogupte commented Aug 27, 2020

formgeist commented Aug 27, 2020

kibanamachine commented Aug 27, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

distributable file count

History

cauemarcondes commented Oct 13, 2020

ogupte commented Aug 22, 2020 •

edited

Loading

sorenlouv Aug 25, 2020 •

edited

Loading

jgowdyelastic Aug 25, 2020 •

edited

Loading

ogupte Aug 26, 2020 •

edited

Loading