[Actionable Observability] - Add alert annotation and threshold shade on the APM latency chart on the Alert Details page

[fkanout]

Summary

Closes #147779 by adding the following elements to the latency chart for the Latency threshold alert:

1. Alert annotation that indicates when the alert is triggered
2. Alert threshold rect (shade)
3. Alert threshold annotation (red line on Y axe)
4. Alert active rect (dark red shade where the alert is active)

[elasticmachine]

Pinging @elastic/actionable-observability (Team: Actionable Observability)

[elasticmachine]

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

[MiriamAparicio]

LGTM

[dgieselaar]

I'm not convinced the proposed change is the way to go - do you mind waiting a few days so we can get a proper review in? Thanks! Op do 29 dec. 2022 13:23 schreef Miriam ***@***.***>:

…

***@***.**** approved this pull request. LGTM — Reply to this email directly, view it on GitHub <#147848 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACWDXAUUMGJTPYGBESWXQ3WPV7FPANCNFSM6AAAAAATELTIYA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[fkanout]

@elasticmachine merge upstream

[sorenlouv]

Since #143298 is merged so I'll add a comment about the following code in this PR:

  const transactionTypes = useServiceTransactionTypesFetcher({
    serviceName,
    start,
    end,
  });

  const transactionType = getTransactionType({
    transactionType: String(alert.fields[TRANSACTION_TYPE]),
    transactionTypes,
    agentName,
  });

I don't understand the above code in this context. We don't want to get a default transaction type. We want the actual transaction type that caused the alert. The transaction type is optional when creating a rule but required on alerts. This means that if the transaction type is not specified for a rule, the rule executor will do a terms agg on transaction.type to ensure an alert is always associated with the correct transaction type. See:

kibana/x-pack/plugins/apm/server/routes/alerts/rule_types/transaction_duration/register_transaction_duration_rule_type.ts

Line 179 in 2e15fb5

{ field: TRANSACTION_TYPE },

tldr: I think the above code is redundant. Get the transaction type from the alert. If the alert doesn't have a transaction type we cannot show the chart (I don't think that's likely to happen anymore but it may have happened to alerts in the past).

[sorenlouv]

In the same vain, why is this needed?

const latencyAggregationType = getAggsTypeFromRule(params.aggregationType);

params.aggregationType is of type LatencyAggregationType and the returned value is LatencyAggregationType.

[sorenlouv]

This also looks incorrect:

const environment = String(params.environment) || ENVIRONMENT_ALL.value;

environment (like transaction.type) is required on the alert (optional on the rule).

[fkanout]

In the same vain, why is this needed?
const latencyAggregationType = getAggsTypeFromRule(params.aggregationType);
params.aggregationType is of type LatencyAggregationType and the returned value is LatencyAggregationType.

@sqren. Good catch. However, we still need the function getAggsTypeFromRule.
The issue is in AlertDetailsAppSectionProps type. The aggregationType is not LatencyAggregationType because it returns 95th and 99th instead of p95 and p99.

export interface AlertDetailsAppSectionProps {
  rule: Rule<{
    environment: string;
    aggregationType: LatencyAggregationType;
    windowSize: number;
    windowUnit: TIME_UNITS;
  }>;
  alert: TopAlert<{ [SERVICE_NAME]: string; [TRANSACTION_TYPE]: string }>;
  timeZone: string;
}

It should beaggregationType: string;

[fkanout]

@sqren, regarding the transactionType in https://github.com/elastic/kibana/pull/147848/files#r1062460375
I don't know the workflow and how it should be implemented.
I followed the same steps in the APMServiceContext to get the tansactionType

const transactionTypes = useServiceTransactionTypesFetcher({
    serviceName,
    start,
    end,
  });

  const currentTransactionType = getOrRedirectToTransactionType({
    transactionType: query.transactionType,
    transactionTypes,
    agentName,
    history,
  });

The only difference is that I'm using getTransactionType instead of getOrRedirectToTransactionType as I don't need/have access to the history argument because it's part of APM context

[sorenlouv]

@sqren, for the environment const environment = String(params.environment) || ENVIRONMENT_ALL.value; It's the rule environment, and it was addressed in this thread #143298 (comment)

We want to display the latency chart for an alert so the user can understand why an alert fired, right? In that case, we should get the environment from the alert - not the rule.

[fkanout]

We want to display the latency chart for an alert so the user can understand why an alert fired, right? In that case, we should get the environment from the alert - not the rule.

@sqren, you are right. I updated it. Thanks!

[fkanout]

BTW, the alert env is fetched from the rule in the createLifecycleRuleType with some null-checks. But still getting the env from the alert is better.

[sorenlouv]

I still see

  const transactionType = getTransactionType({
    transactionType: String(alert.fields[TRANSACTION_TYPE]),
    transactionTypes,
    agentName,
  });

This should be changed to:

const transactionType = alert.fields[TRANSACTION_TYPE];

[fkanout]

Done

[sorenlouv]

The annotation stuff lgtm. But I have some concerns about some of the existing code.

[sorenlouv]

I can't quite wrap my head around whether moving this to the context works out. But it's probably fine.

[fkanout]

@elasticmachine merge upstream

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: dd8b739

Failed CI Steps

• FTR Configs #15

Test Failures

• [job] [logs] FTR Configs #15 / dashboard sync colors should be possible to disable color sync
• [job] [logs] FTR Configs #15 / dashboard sync colors should sync colors on dashboard by default

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
apm	1346	1352	+6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
apm	3.2MB	3.2MB	+1.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
apm	28.9KB	29.1KB	+141.0B

History

• 💔 Build #98676 failed 7f73c95
• 💔 Build #98601 failed 5839a1d
• 💛 Build #98375 was flaky 8ac0b2a
• 💚 Build #98115 succeeded b5d9213
• 💛 Build #98059 was flaky d5e5938

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

cc @fkanout