Telemetry for manual rule run by nkhristinin · Pull Request #186364 · elastic/kibana

nkhristinin · 2024-06-18T11:41:21Z

Summary

report following events:

open modal window for manual rule run
execute manual rule run + save time range in ms
cancel backfill job
filter in event log by run type
show source event date range

Epic - https://github.com/elastic/security-team/issues/2840

How to test

enable feature flag - manualRuleRunEnabled

You can see feature demo here - #184500

Check that events appears here after some time - https://telemetry-v2-staging.elastic.dev/s/securitysolution/app/r/s/7YYlg

yctercero · 2024-06-18T11:59:21Z

@nkhristinin Great to see telemetry added! Could you include steps on how to test the PR and link to the manual rule runs epic? Since this is a new feature, reviewers may not know what it's referencing.

nkhristinin · 2024-06-18T13:47:33Z

@elasticmachine merge upstream

nkhristinin · 2024-06-19T07:36:31Z

@elasticmachine merge upstream

nkhristinin · 2024-06-20T08:48:33Z

@elasticmachine merge upstream

xcrzx

Hey @nkhristinin, thanks for the PR! Leaving a couple of minor suggestions and a few questions, please take a look.

xcrzx · 2024-06-20T08:49:12Z

...ection_engine/rule_details_ui/pages/rule_details/execution_log_table/execution_log_table.tsx

  });

+  const handleShowSourceEventTimeRange = useCallback(
+    (e) => {


Please provide the type for the event argument: e: EuiSwitchEvent, otherwise it will be any.

xcrzx · 2024-06-20T08:58:06Z

...etection_engine/rule_monitoring/components/basic/filters/execution_run_type_filter/index.tsx

+  const handleSelectionChange = useCallback(
+    (types: RuleRunType[]) => {
+      onChange(types);
+      telemetry.reportEventLogFilterByRunType({ runType: types?.join(',') });


Shouldn't the runType argument be an array of strings instead of a string of comma-separated values? How will that look in a dashboard if someone wants to display a chart of the most used run types?

xcrzx · 2024-06-20T09:02:07Z

...k/plugins/security_solution/public/detection_engine/rule_gaps/logic/use_schedule_rule_run.ts

        const results = await mutateAsync(options);
+        telemetry.reportManualRuleRunExecute({
+          rangeInMs: options.timeRange.endDate.diff(options.timeRange.startDate),
+        });


This will only capture successful manual executions, do we need to capture failures as well?

x-pack/plugins/security_solution/public/common/lib/telemetry/events/manual_rule_run/index.ts

…bana into telemetry-manual-rule-run

nkhristinin · 2024-06-24T14:40:02Z

@elasticmachine merge upstream

rylnd · 2024-06-25T21:20:39Z

x-pack/plugins/security_solution/public/common/lib/telemetry/constants.ts

  OnboardingHubStepLinkClicked = 'Onboarding Hub Step Link Clicked',
+  ManualRuleRunOpenModal = 'Manual Rule Run Open Modal',
+  ManualRuleRunExecute = 'Manual Rule Run Execute',
+  ManualRuleRunCancelJob = 'Manual Rule Cancel Job',


Without "Run" this description is sorta ambiguous (or less precise than it could be):

Suggested change

ManualRuleRunCancelJob = 'Manual Rule Cancel Job',

ManualRuleRunCancelJob = 'Manual Rule Run Cancel Job',

rylnd

There are several places where telemetry is being added here, but I only see a single unit test asserting that any of this telemetry code is being executed. An integration test would be great to see (interact with app, observe telemetry being collected), but even a few more unit tests would help ensure this behavior is both documented and verified.

rylnd · 2024-06-25T21:22:12Z

x-pack/plugins/security_solution/public/common/lib/telemetry/constants.ts

+  ManualRuleRunExecute = 'Manual Rule Run Execute',
+  ManualRuleRunCancelJob = 'Manual Rule Cancel Job',
+  EventLogFilterByRunType = 'Event Log Filter By Run Type',
+  EventLogShowSourceEventDateRange = 'Event Log Show Source Event Date Range',


These are just strings, right? Is it possible/preferable to add some more structure, here? There are a lot of descriptive words here and it's hard to untangle exactly what's being expressed.

Suggested change

EventLogShowSourceEventDateRange = 'Event Log Show Source Event Date Range',

EventLogShowSourceEventDateRange = 'Event Log -> Show Source Event -> Date Range',

I was suggesting that we add some kind of hierarchical delimiter to all of these descriptions, not just the one 😅 . I'd value consistency over improving a single item, here, so IMO I'd revert this change.

Stepping in here cause it took me some time to understand what 'Event Log -> Show Source Event -> Date Range' meant. But if I understand correctly, it's the action of clicking on the Show Source Event Date Range on the Execution Results tab:

So yes, I would either:

revert to the version without the arrows: 'Event Log Show Source Event Date Range'

delimit only Event log from the rest. Something like: 'Event Log -> Show Source Event Date Range' or 'Event Log | Show Source Event Date Range'

Maybe even adding the suffix Toggle Switch at the end: 'Event Log | Show Source Event Date Range Toggle Switch

And the same for EventLogFilterByRunType

rylnd · 2024-06-25T21:26:58Z

x-pack/plugins/security_solution/public/common/lib/telemetry/events/manual_rule_run/index.ts

+    totalTasks: {
+      type: 'integer',
+      _meta: {
+        description: 'Total number of tasks at the moment of job cancellation',


What is a "task" in this context? What is a "job," for that matter?

rylnd · 2024-06-25T21:28:44Z

x-pack/plugins/security_solution/public/common/lib/telemetry/events/manual_rule_run/index.ts

+    rangeInMs: {
+      type: 'integer',
+      _meta: {
+        description: 'Execute manual rule run with range in milliseconds',


Should this be describing rangeInMs exclusively, or the full event?

Suggested change

description: 'Execute manual rule run with range in milliseconds',

description: 'The time range (expressed in milliseconds) against which the manual rule run was executed',

rylnd · 2024-06-25T21:29:34Z

x-pack/plugins/security_solution/public/common/lib/telemetry/events/manual_rule_run/index.ts

+    status: {
+      type: 'keyword',
+      _meta: {
+        description: 'Execute manual rule run status (success|error)',


Similarly:

Suggested change

description: 'Execute manual rule run status (success|error)',

description: 'Outcome state of the manual rule run. Possible values are "success" and "error"',

…bana into telemetry-manual-rule-run

rylnd

Changes look great! The additional test coverage is much appreciated.

nkhristinin · 2024-07-02T05:57:00Z

@elasticmachine merge upstream

kibana-ci · 2024-07-02T07:21:48Z

💚 Build Succeeded

Buildkite Build
Commit: fa53425

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	5583	5587	+4

Async chunks

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

id	before	after	diff
`securitySolution`	15.5MB	15.6MB	+999.0B

Page load bundle

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

id	before	after	diff
`securitySolution`	83.7KB	85.9KB	+2.2KB

History

💛 Build #218553 was flaky 214a33c
💔 Build #218478 failed 39b4b16
💛 Build #218422 was flaky b6e6c10
💛 Build #217452 was flaky fccc0b1
💔 Build #217070 failed b9a1eb8
💛 Build #216843 was flaky 65d5afe

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

jpdjere

Thanks for addressing all comments left by @xcrzx

I left one more comment regarding the TelemetryEventTypes enum, but overall LGTM.

nkhristinin added 2 commits June 18, 2024 12:11

Add telemety for manual rule run

405207d

Add telemetry for event log

6172fd6

nkhristinin requested review from a team as code owners June 18, 2024 11:41

nkhristinin requested review from rylnd and xcrzx June 18, 2024 11:41

nkhristinin added the release_note:skip Skip the PR/issue when compiling release notes label Jun 18, 2024

Merge branch 'main' into telemetry-manual-rule-run

f619966

kibanamachine and others added 3 commits June 19, 2024 09:36

Merge branch 'main' into telemetry-manual-rule-run

fdf335c

fix types

f90c1a7

Fix tests

09f7799

Merge branch 'main' into telemetry-manual-rule-run

65d5afe

xcrzx reviewed Jun 20, 2024

View reviewed changes

nkhristinin added 2 commits June 20, 2024 15:13

PR fixes

487e349

Merge branch 'telemetry-manual-rule-run' of github.com:nkhristinin/ki…

b9a1eb8

…bana into telemetry-manual-rule-run

nkhristinin requested a review from xcrzx June 21, 2024 13:24

Fix tests

e58b757

Merge branch 'main' into telemetry-manual-rule-run

fccc0b1

rylnd reviewed Jun 25, 2024

View reviewed changes

rylnd requested changes Jun 25, 2024

View reviewed changes

nkhristinin requested a review from e40pud June 28, 2024 09:21

nkhristinin added 3 commits June 28, 2024 13:13

Merge branch 'main' into telemetry-manual-rule-run

89df3e2

Merge branch 'telemetry-manual-rule-run' of github.com:nkhristinin/ki…

b6e6c10

…bana into telemetry-manual-rule-run

Fix telemetry titles

82decab

nkhristinin added 2 commits June 28, 2024 16:11

Add unit tets

ec57d7e

Add bulk telemetry

39b4b16

nkhristinin requested a review from rylnd June 28, 2024 14:45

fix tests

214a33c

rylnd approved these changes Jun 28, 2024

View reviewed changes

Merge branch 'main' into telemetry-manual-rule-run

fa53425

banderror requested review from jpdjere and removed request for xcrzx July 2, 2024 09:35

jpdjere approved these changes Jul 2, 2024

View reviewed changes

nkhristinin merged commit 32e7bf9 into elastic:main Jul 2, 2024

kibanamachine added v8.15.0 backport:skip This PR does not require backporting labels Jul 2, 2024

	ManualRuleRunCancelJob = 'Manual Rule Cancel Job',
	ManualRuleRunCancelJob = 'Manual Rule Run Cancel Job',

	EventLogShowSourceEventDateRange = 'Event Log Show Source Event Date Range',
	EventLogShowSourceEventDateRange = 'Event Log -> Show Source Event -> Date Range',

	description: 'Execute manual rule run with range in milliseconds',
	description: 'The time range (expressed in milliseconds) against which the manual rule run was executed',

	description: 'Execute manual rule run status (success\|error)',
	description: 'Outcome state of the manual rule run. Possible values are "success" and "error"',

Conversation

nkhristinin commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How to test

Uh oh!

yctercero commented Jun 18, 2024

Uh oh!

nkhristinin commented Jun 18, 2024

Uh oh!

nkhristinin commented Jun 19, 2024

Uh oh!

nkhristinin commented Jun 20, 2024

Uh oh!

xcrzx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkhristinin commented Jun 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpdjere Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

nkhristinin commented Jul 2, 2024

Uh oh!

kibana-ci commented Jul 2, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

Uh oh!

jpdjere left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nkhristinin commented Jun 18, 2024 •

edited

Loading

jpdjere Jul 2, 2024 •

edited

Loading