[AO] - Add scaffolding and the main chart to the Logs threshold Alert Details page #153081

fkanout · 2023-03-10T08:24:27Z

Summary

This is a kickoff PR; more PRs will follow up.

It closes #152738

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

...k/plugins/infra/public/alerting/log_threshold/components/alert_details_app_section/index.tsx

x-pack/plugins/infra/server/routes/log_alerts/chart_preview_data.ts

…ails-page

…-ref HEAD~1..HEAD --fix'

…ails-page

kdelemme · 2023-03-24T13:35:39Z

...ck/plugins/infra/public/alerting/log_threshold/components/alert_details_app_section/types.ts

+export interface AlertDetailsAppSectionProps {
+  rule: Rule<PartialRuleParams>;
+  alert: TopAlert;
+  timeZone: string;
+  setAlertSummaryFields: React.Dispatch<React.SetStateAction<AlertSummaryField[] | undefined>>;
+}


Are we not supposed to export the Props directly from the component file?

Not sure if it is the same in the infra plugin.

oh yes indeed, it might be different

x-pack/plugins/infra/server/lib/alerting/log_threshold/log_threshold_executor.test.ts

maryam-saeidi · 2023-03-24T13:39:44Z

x-pack/plugins/infra/common/alerting/logs/log_threshold/types.ts

@@ -401,3 +401,8 @@ export const isOptimizableGroupedThreshold = (
    return false;
  }
 };
+
+export interface ExecutionTimeRange {
+  gte?: number;


Why is this optional?

Because this is not used on the server side, it's used only if we define it for the Logs chart.

From what I see, we use both on the server side: https://github.com/elastic/kibana/pull/153081/files#diff-a71ca536380c1fde8805744b23566ce795707f92b94a03af73347cac46ccac63R654-R655

@maryam-saeidi, only the Alert Details page (frontend) will fill up the gte, which is sent to the server side.
Something to mention is ExecutionTimeRange is not only used to get the chart data (where we could use gte). But also in the log threshold executor (where gte is not used)

kdelemme

LGTM, If I want to test this locally, what feature flag do I need to enable?

maryam-saeidi · 2023-03-24T13:52:37Z

LGTM, If I want to test this locally, what feature flag do I need to enable?

xpack.observability.unsafe.alertDetails.logs.enabled

maryam-saeidi · 2023-03-24T16:05:01Z

When checked the chart, it first showed no data:

Then loading:

And then chart:

Is there a ticket to fix the loading?

fkanout · 2023-03-27T10:10:29Z

@maryam-saeidi here is the issue related to the no data message #153749

...k/plugins/infra/public/alerting/log_threshold/components/alert_details_app_section/index.tsx

tonyghiani

LGTM, the main doubts were already covered by other comments (no-data chart) and I just left a nit totally non-blocking!
Thanks for this work!

maryam-saeidi

Checked locally, and it worked as expected.
I left some comments in the PR.

maryam-saeidi · 2023-03-27T15:05:01Z

x-pack/plugins/infra/common/alerting/logs/log_threshold/types.ts

@@ -401,3 +401,8 @@ export const isOptimizableGroupedThreshold = (
    return false;
  }
 };
+
+export interface ExecutionTimeRange {
+  gte?: number;


From what I see, we use both on the server side: https://github.com/elastic/kibana/pull/153081/files#diff-a71ca536380c1fde8805744b23566ce795707f92b94a03af73347cac46ccac63R654-R655

maryam-saeidi · 2023-03-27T15:05:14Z

...ck/plugins/infra/public/alerting/log_threshold/components/alert_details_app_section/types.ts

+export interface AlertDetailsAppSectionProps {
+  rule: Rule<PartialRuleParams>;
+  alert: TopAlert;
+  timeZone: string;


You don't use these two fields in this PR:

timeZone: string; setAlertSummaryFields: React.Dispatch<React.SetStateAction<AlertSummaryField[] | undefined>>;

@maryam-saeidi, Really! 😄 Okay, I will delete them.

maryam-saeidi · 2023-03-27T15:05:26Z

x-pack/plugins/infra/public/types.ts

@@ -112,6 +112,11 @@ export interface LensOptions {
  breakdownSize: number;
 }

+export interface ExecutionTimeRange {


Is this interface needed? There is also one in alerting/logs/log_threshold/types

Yes. One with gte required a type for the frontend, and the other one gte is optional for backend.

maryam-saeidi · 2023-03-27T15:06:19Z

x-pack/plugins/infra/server/lib/alerting/log_threshold/log_threshold_executor.test.ts

@@ -140,7 +140,9 @@ const baseRuleParams: Pick<RuleParams, 'count' | 'timeSize' | 'timeUnit' | 'logV

 const TIMESTAMP_FIELD = '@timestamp';
 const FILEBEAT_INDEX = 'filebeat-*';
-const EXECUTION_TIMESTAMP = new Date('2022-01-01T00:00:00.000Z').valueOf();
+const EXECUTION_TIMERANGE = {
+  lte: new Date('2022-01-01T00:00:00.000Z').valueOf(),


Since we might have gte shall we add a test for that case too?

const to = executionTimeRange?.lte || Date.now(); const from = executionTimeRange?.gte || to - intervalAsMs;

@maryam-saeidi, no need here. The executor doesn't use gte

...k/plugins/infra/public/alerting/log_threshold/components/alert_details_app_section/index.tsx

…ails-page

fkanout · 2023-03-28T12:36:40Z

@elasticmachine merge upstream

…ails-page

maryam-saeidi

LGTM! 👍🏻

kibana-ci · 2023-03-28T16:10:26Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: a065a5f

Failed CI Steps

FTR Configs #36

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`infra`	1291	1292	+1

Async chunks

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

id	before	after	diff
`infra`	1.3MB	1.4MB	+11.7KB

Page load bundle

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

id	before	after	diff
`infra`	88.0KB	88.3KB	+207.0B

Unknown metric groups

async chunk count

id	before	after	diff
`infra`	24	25	+1

ESLint disabled line counts

id	before	after	diff
`infra`	47	48	+1
`securitySolution`	433	436	+3
total			+4

Total ESLint disabled count

id	before	after	diff
`infra`	55	56	+1
`securitySolution`	513	516	+3
total			+4

History

💔 Build #116026 failed 138972a
💚 Build #115318 succeeded 2d9831e
💚 Build #115028 succeeded 06a0bd4
💔 Build #115016 failed f402c01
💔 Build #114959 failed c13fdf0

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

cc @fkanout

Add chart and appSection

9e7fdae

maryam-saeidi reviewed Mar 10, 2023

View reviewed changes

...k/plugins/infra/public/alerting/log_threshold/components/alert_details_app_section/index.tsx Outdated Show resolved Hide resolved

maryam-saeidi reviewed Mar 10, 2023

View reviewed changes

x-pack/plugins/infra/server/routes/log_alerts/chart_preview_data.ts Outdated Show resolved Hide resolved

fkanout and others added 6 commits March 22, 2023 13:26

Merge branch 'main' into 152738-scaffolding-main-chart-logs-alert-det…

a4e2754

…ails-page

Add executionTimeRange

00ed7c1

Merge branch 'main' into 152738-scaffolding-main-chart-logs-alert-det…

a70ba16

…ails-page

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

553eb96

…-ref HEAD~1..HEAD --fix'

Fix type import and tests

9ced201

Add a chart per criteria

c13fdf0

fkanout self-assigned this Mar 23, 2023

fkanout added v8.8.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 23, 2023

fkanout added 2 commits March 23, 2023 15:22

declare type in one place

f402c01

Merge branch 'main' into 152738-scaffolding-main-chart-logs-alert-det…

06a0bd4

…ails-page

fkanout marked this pull request as ready for review March 23, 2023 14:45

fkanout requested a review from a team as a code owner March 23, 2023 14:45

kdelemme reviewed Mar 24, 2023

View reviewed changes

x-pack/plugins/infra/server/lib/alerting/log_threshold/log_threshold_executor.test.ts Outdated Show resolved Hide resolved

maryam-saeidi reviewed Mar 24, 2023

View reviewed changes

kdelemme reviewed Mar 24, 2023

View reviewed changes

codereview - var name

2d9831e

fkanout mentioned this pull request Mar 27, 2023

[AO] - Update the Logs chart history that shows No data while loading #153749

Open

tonyghiani reviewed Mar 27, 2023

View reviewed changes

...k/plugins/infra/public/alerting/log_threshold/components/alert_details_app_section/index.tsx Outdated Show resolved Hide resolved

tonyghiani approved these changes Mar 27, 2023

View reviewed changes

maryam-saeidi requested changes Mar 27, 2023

View reviewed changes

fkanout added 2 commits March 28, 2023 14:15

code review

62d24bb

code review

d39c7bb

fkanout requested a review from maryam-saeidi March 28, 2023 12:30

Merge branch 'main' into 152738-scaffolding-main-chart-logs-alert-det…

3cca372

…ails-page

kibanamachine and others added 2 commits March 28, 2023 08:36

Merge branch 'main' into 152738-scaffolding-main-chart-logs-alert-det…

138972a

…ails-page

Put the chart within flex item

a065a5f

maryam-saeidi approved these changes Mar 28, 2023

View reviewed changes

fkanout merged commit c1a8b90 into elastic:main Mar 28, 2023

kibanamachine added the backport:skip This commit does not require backporting label Mar 28, 2023

elasticmachine unassigned fkanout Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AO] - Add scaffolding and the main chart to the Logs threshold Alert Details page #153081

[AO] - Add scaffolding and the main chart to the Logs threshold Alert Details page #153081

fkanout commented Mar 10, 2023 •

edited

Loading

kdelemme Mar 24, 2023

maryam-saeidi Mar 24, 2023

kdelemme Mar 24, 2023

maryam-saeidi Mar 24, 2023

fkanout Mar 24, 2023

maryam-saeidi Mar 27, 2023

fkanout Mar 28, 2023

kdelemme left a comment

maryam-saeidi commented Mar 24, 2023

maryam-saeidi commented Mar 24, 2023

fkanout commented Mar 27, 2023

tonyghiani left a comment

maryam-saeidi left a comment

maryam-saeidi Mar 27, 2023

maryam-saeidi Mar 27, 2023

fkanout Mar 28, 2023

maryam-saeidi Mar 27, 2023

fkanout Mar 28, 2023

maryam-saeidi Mar 27, 2023

fkanout Mar 28, 2023

fkanout commented Mar 28, 2023

maryam-saeidi left a comment

kibana-ci commented Mar 28, 2023

async chunk count

ESLint disabled line counts

Total ESLint disabled count

[AO] - Add scaffolding and the main chart to the Logs threshold Alert Details page #153081

[AO] - Add scaffolding and the main chart to the Logs threshold Alert Details page #153081

Conversation

fkanout commented Mar 10, 2023 • edited Loading

Summary

This is a kickoff PR; more PRs will follow up.

Checklist

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

kdelemme left a comment

Choose a reason for hiding this comment

maryam-saeidi commented Mar 24, 2023

maryam-saeidi commented Mar 24, 2023

fkanout commented Mar 27, 2023

tonyghiani left a comment

Choose a reason for hiding this comment

maryam-saeidi 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

fkanout commented Mar 28, 2023

maryam-saeidi left a comment

Choose a reason for hiding this comment

kibana-ci commented Mar 28, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

ESLint disabled line counts

Total ESLint disabled count

History

fkanout commented Mar 10, 2023 •

edited

Loading