[APM] Add transaction error rate alert #76933

sorenlouv · 2020-09-08T12:58:07Z

Closes #76367

Adds a new alert for Transaction Error Rate
Renames the existing alert error_rate to error_count to avoid confusion with transaction error rate
Made service name visible as read only for every alerting flyout
Made transaction type visible for every alerting flyout
Extracted "actionVariables" into a separate, shared file
extracted field types for servicename, environment and transaction type into separate, shared components
Made all alert parameters available to the alert description

It is now possible to write a message like the one below with the new alert parameters

Transaction type will be read-only if there's only a single value

Transaction type will be a dropdown if there's multiple values

Service name is now visible for all alerts even though it's not editable

elasticmachine · 2020-09-08T12:58:13Z

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

sorenlouv · 2020-09-08T13:00:03Z

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

@@ -96,17 +109,22 @@ export function AlertIntegrations(props: Props) {
          : []),
      ],
    },
+
+    // transaction duration panel


I can remove these comments again. Added them for my own sake but I can see how they are somewhat noisy/redundant.

x-pack/plugins/apm/public/plugin.ts

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

sorenlouv · 2020-09-08T15:11:33Z

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

+        }
+      )}
+      value={transactionType}
+    />,


Transaction type used to be hidden. Now it's visible (read-only)

x-pack/plugins/apm/server/lib/rum_client/get_page_view_trends.ts

shahzad31

Uptime code owner changes LGTM

dgieselaar

If I try to create an anomaly alert I get an empty form:

If I try to create a transaction alert from the errors or metrics tab, type is empty and non-editable:

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

x-pack/plugins/apm/public/components/alerting/TransactionDurationAlertTrigger/index.tsx

x-pack/plugins/apm/public/components/alerting/TransactionErrorRateAlertTrigger/index.tsx

x-pack/plugins/apm/public/components/alerting/TransactionDurationAlertTrigger/index.tsx

sorenlouv · 2020-09-16T11:57:31Z

@dgieselaar All comments addressed

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

dgieselaar · 2020-09-16T13:47:32Z

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

+      const environmentFilter =
+        alertParams.environment === ENVIRONMENT_ALL.value
+          ? []
+          : [{ term: { [SERVICE_ENVIRONMENT]: alertParams.environment } }];


shouldn't we use something like getEnvironmentUiFilterES?

Totally. I was lazy and copied the alert from below and didn't notice:

https://github.com/sqren/kibana/blob/9caf99a1b18791a38adf66a78ca6eea8d580385c/x-pack/plugins/apm/server/lib/alerts/register_transaction_duration_alert_type.ts#L89-L92

Will update the other alerts as well

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

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

sorenlouv · 2020-09-17T10:34:45Z

x-pack/plugins/apm/public/components/alerting/ErrorCountAlertTrigger/index.tsx

+    <ServiceField value={serviceName} />,
+    <EnvironmentField
+      currentValue={params.environment}
+      options={environmentOptions}
+      onChange={(e) => setAlertParams('environment', e.target.value)}
+    />,
+    <IsAboveField
+      value={params.threshold}
+      unit={i18n.translate('xpack.apm.errorCountAlertTrigger.errors', {
+        defaultMessage: ' errors',


Extracting commonly-used field types into separate components. I was a little torn whether this improves readability by hiding boilerplate or hurts readability by adding an additional abstraction.
In the end I decided it was better.

I'm not entirely sure if the space should be in the label.

You are right. Was hoping you wouldn't notice 🙈
I'll fix it in a follow-up. For now I'll merge to avoid additional conflicts.

sorenlouv · 2020-09-17T10:35:44Z

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

+
+import { i18n } from '@kbn/i18n';
+
+export const apmActionVariables = {


Extracted the action variables and their descriptions to avoid duplication and improve readability of alerts.

sorenlouv · 2020-09-17T10:37:01Z

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

@@ -88,6 +73,7 @@ export function registerTransactionDurationAnomalyAlertType({

      const anomalySearchParams = {
        body: {
+          terminateAfter: 1,


@ogupte I added terminateAfter to improve perf of this alert since we only need a single document. Do you agree this is fine?

sorenlouv · 2020-09-17T10:49:26Z

x-pack/plugins/apm/public/components/alerting/fields.tsx

+import { getEnvironmentLabel } from '../../../common/environment_filter_values';
+import { PopoverExpression } from './ServiceAlertTrigger/PopoverExpression';
+
+export function ServiceField({ value }: { value?: string }) {


This file contains the commonly-used alerting fields. I intend for these fields to be almost 1:1 in their use across alerts. If they start to diverge we should abandon this approach (but I don't think that'll be the case)

sorenlouv · 2020-09-17T12:38:39Z

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

+          threshold: alertParams.threshold,
+          triggerValue: errorCount,


All threshold alerts now expose the threshold and the value that triggered the alert. They are consistently named threshold and triggerValue to make it easier to reuse templates between alerts.

dgieselaar

nice improvements 👍🏻

dgieselaar · 2020-09-17T12:56:58Z

x-pack/plugins/apm/public/components/alerting/ErrorCountAlertTrigger/index.tsx

+    <ServiceField value={serviceName} />,
+    <EnvironmentField
+      currentValue={params.environment}
+      options={environmentOptions}
+      onChange={(e) => setAlertParams('environment', e.target.value)}
+    />,
+    <IsAboveField
+      value={params.threshold}
+      unit={i18n.translate('xpack.apm.errorCountAlertTrigger.errors', {
+        defaultMessage: ' errors',


I'm not entirely sure if the space should be in the label.

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

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

dgieselaar · 2020-09-17T13:10:57Z

x-pack/plugins/apm/server/lib/helpers/convert_ui_filters/get_environment_ui_filter_es.ts

 import { SERVICE_ENVIRONMENT } from '../../../../common/elasticsearch_fieldnames';

 export function getEnvironmentUiFilterES(environment?: string): ESFilter[] {
-  if (!environment) {
+  if (!environment || environment === ENVIRONMENT_ALL.value) {


What's the use case for having environment as an optional argument here?

getEnvironmentUiFilterES is called from places where environment is optional. Those callers would have to implement this themselves if it was required:

const envFilter = environment ? getEnvironmentUiFilterES(environment) : []

(is my guess - haven't checked)

I see. I guess what I'm arguing is that environment should never be undefined. It should always be ENVIRONMENT_ALL.value, ENVIRONMENT_NOT_DEFINED.value, or another non-empty string. But obviously that's a bigger and potentially painful change.

Agree. I think right now the client will not send environment when "all" is selected. We should ofc always send a value regardless if "not defined", "all" or "" is selected.

kibanamachine · 2020-09-17T18:07:39Z

💚 Build Succeeded

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

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
apm	1264	+3	1261

async chunks size

id	value	diff	baseline
apm	5.0MB	+32.8KB	5.0MB

page load bundle size

id	value	diff	baseline
apm	42.3KB	+80.0B	42.3KB

distributable file count

id	value	diff	baseline
default	45914	+5	45909

History

💔 Build #75132 failed 0d61470
💔 Build #75113 failed a9ce803
💔 Build #75103 failed 8dc074dedf4260221aaf667c9185c42b2068ae3c

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

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

* master: (92 commits) [ILM] Data tiers for 7.10 (elastic#76126) [ML] Transforms: Fixes styling of preview grid pagination in summary step (elastic#77789) [Drilldowns] Beta badge support. Mark URL Drilldown as Beta (elastic#75654) Re-enable session lifespan, idle timeout api integration tests and use unique names for the security test reports. (elastic#77746) [Alerting] renames code in alerting RBAC exemption to make it easier to maintain (elastic#77598) [Alerting & Actions] Overwrite SOs when updating instead of partially updating (elastic#73688) fixed react warning in Suspense in alert flyout (elastic#77777) [APM] Track usage of Gold+ features (elastic#77630) Visualize: Bad request when working with histogram aggregation (elastic#77684) remove legacy ES plugin (elastic#77703) [Lens] change name of custom query to filters (elastic#77725) skip flaky suite (elastic#76239) remove visual aspects of baseline job (elastic#77815) skip flaky suite (elastic#77835) Fixes typo in data recognizer text (elastic#77691) management/update trusted_apps jest snapshot [build] Use Elastic hosted UBI minimal base image (elastic#77776) [APM] Add transaction error rate alert (elastic#76933) [Security Solution] [Detections] Remove file validation on import route (elastic#77770) [Enterprise Search][tech debt] Add Kea logic paths for easier debugging/defaults (elastic#77698) ...

cauemarcondes · 2020-10-12T09:52:49Z

@sqren @formgeist should we validate if a user tries to create an alert with more than 100%?

sorenlouv · 2020-10-12T10:09:24Z

@cauemarcondes Good question. I think it's a nice-to-have. Is it easy to do?

cauemarcondes · 2020-10-12T10:49:09Z

@cauemarcondes Good question. I think it's a nice-to-have. Is it easy to do?

We can add isInvalid={value > 100} property to the field, but I don't think we have control over the Save button in the flyout, so there's no way to disable or enable it. I'd only show the field as red.

Maybe for 7.10 we should leave it like it is, and maybe change it for 7.11 if needed?

cauemarcondes · 2020-10-12T11:09:12Z

Tests ok:
Chrome ✅
FF ✅
Safari ✅

sorenlouv · 2020-10-12T15:03:31Z

Okay, then let's leave it for now.

sorenlouv requested a review from a team as a code owner September 8, 2020 12:58

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

sorenlouv commented Sep 8, 2020

View reviewed changes

x-pack/plugins/apm/public/plugin.ts Show resolved Hide resolved

sorenlouv commented Sep 8, 2020

View reviewed changes

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

sorenlouv commented Sep 8, 2020

View reviewed changes

sorenlouv added release_note:roadmap v7.10.0 labels Sep 8, 2020

sorenlouv force-pushed the add-transaction-error-rate-alert branch 2 times, most recently from ac6cade to 2eb1cd6 Compare September 8, 2020 20:50

sorenlouv requested a review from a team as a code owner September 8, 2020 21:35

sorenlouv commented Sep 9, 2020

View reviewed changes

x-pack/plugins/apm/server/lib/rum_client/get_page_view_trends.ts Outdated Show resolved Hide resolved

shahzad31 approved these changes Sep 9, 2020

View reviewed changes

dgieselaar reviewed Sep 10, 2020

View reviewed changes

sorenlouv force-pushed the add-transaction-error-rate-alert branch 2 times, most recently from 3843ec0 to 5336dbb Compare September 15, 2020 19:19

elastic deleted a comment from cla-checker-service bot Sep 15, 2020

sorenlouv force-pushed the add-transaction-error-rate-alert branch from 67c4599 to 9caf99a Compare September 15, 2020 22:14

elastic deleted a comment from kibanamachine Sep 16, 2020

dgieselaar reviewed Sep 16, 2020

View reviewed changes

[APM] Add transaction error rate alert

a9ce803

sorenlouv force-pushed the add-transaction-error-rate-alert branch from 8dc074d to a9ce803 Compare September 17, 2020 09:08

Refactor field types

0d61470

sorenlouv commented Sep 17, 2020

View reviewed changes

dgieselaar approved these changes Sep 17, 2020

View reviewed changes

Fix tsc, i18n and use constants instead of strings for processor event

ee67276

sorenlouv merged commit fb62929 into elastic:master Sep 17, 2020

sorenlouv deleted the add-transaction-error-rate-alert branch September 17, 2020 20:38

sorenlouv mentioned this pull request Sep 17, 2020

[7.x] [APM] Add transaction error rate alert (#76933) #77824

Merged

sorenlouv added a commit that referenced this pull request Sep 18, 2020

[APM] Add transaction error rate alert (#76933) (#77824)

5bcc433

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

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

cauemarcondes self-assigned this Oct 12, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Add transaction error rate alert #76933

[APM] Add transaction error rate alert #76933

sorenlouv commented Sep 8, 2020 •

edited

Loading

elasticmachine commented Sep 8, 2020

sorenlouv Sep 8, 2020 •

edited

Loading

sorenlouv Sep 8, 2020

shahzad31 left a comment

dgieselaar left a comment

sorenlouv commented Sep 16, 2020

dgieselaar Sep 16, 2020

sorenlouv Sep 16, 2020

sorenlouv Sep 17, 2020

dgieselaar Sep 17, 2020

sorenlouv Sep 17, 2020

sorenlouv Sep 17, 2020

sorenlouv Sep 17, 2020

sorenlouv Sep 17, 2020

sorenlouv Sep 17, 2020

dgieselaar left a comment

dgieselaar Sep 17, 2020

dgieselaar Sep 17, 2020

sorenlouv Sep 17, 2020

sorenlouv Sep 17, 2020

dgieselaar Sep 17, 2020

sorenlouv Sep 17, 2020 •

edited

Loading

kibanamachine commented Sep 17, 2020

cauemarcondes commented Oct 12, 2020

sorenlouv commented Oct 12, 2020

cauemarcondes commented Oct 12, 2020 •

edited

Loading

cauemarcondes commented Oct 12, 2020

sorenlouv commented Oct 12, 2020


		import { i18n } from '@kbn/i18n';

		export const apmActionVariables = {

[APM] Add transaction error rate alert #76933

[APM] Add transaction error rate alert #76933

Conversation

sorenlouv commented Sep 8, 2020 • edited Loading

elasticmachine commented Sep 8, 2020

sorenlouv Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzad31 left a comment

Choose a reason for hiding this comment

dgieselaar left a comment

Choose a reason for hiding this comment

sorenlouv commented Sep 16, 2020

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

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

sorenlouv Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Sep 17, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

distributable file count

History

cauemarcondes commented Oct 12, 2020

sorenlouv commented Oct 12, 2020

cauemarcondes commented Oct 12, 2020 • edited Loading

cauemarcondes commented Oct 12, 2020

sorenlouv commented Oct 12, 2020

sorenlouv commented Sep 8, 2020 •

edited

Loading

sorenlouv Sep 8, 2020 •

edited

Loading

sorenlouv Sep 17, 2020 •

edited

Loading

cauemarcondes commented Oct 12, 2020 •

edited

Loading