[ML] Improves Data drift time range selection & show hints for analysis process #174049

qn895 · 2023-12-29T01:38:18Z

Summary

Enhances the selection behavior in the Data Drift view to allow time ranges which overlap in the Reference and Comparison data sets to be analyzed. Improvements include:

Allow for the Reference and Comparison time range to be able to overlap
Click on each chart to set the time range
Prompts and hints for user

Screen.Recording.2024-01-04.at.16.32.34.mov

Checklist

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
Flaky Test Runner was used on any tests changed
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This was checked for cross-browser compatibility

…ed-charts

peteharverson · 2024-01-03T12:39:45Z

...c/application/data_drift/document_count_chart_single_brush/document_count_chart_singular.tsx

+    ]
+  );
+
+  const onElementClick: ElementClickListener = useCallback(


I'm finding that the range selected on click doesn't coincide with the clicked on histogram bucket. Is this the expected behavior? Currently I always have to adjust the range that is created on click.

Updated here 42cedad

Screen.Recording.2024-01-04.at.16.32.34.mov

peteharverson · 2024-01-03T12:46:18Z

...c/application/data_drift/document_count_chart_single_brush/document_count_chart_singular.tsx

+          </div>
+        </div>
+      ) : (
+        <EuiText color="subdued" size="s" textAlign="center">


Should we only allow the analysis to be run when a selection is made in both charts? Currently it looks like a default range is set in the Comparison chart on selection in the Reference chart but the brush only appears when you perform a click in the Reference chart. The other option would be to add brushes to both charts when the first click is made in either chart.

Updated here 42cedad

Screen.Recording.2024-01-04.at.16.32.34.mov

walterra · 2024-01-03T15:48:01Z

x-pack/plugins/data_visualizer/public/application/data_drift/data_drift_view.tsx

+  }
+  if (showRunAnalysisHint) {
+    return (
+      <EuiEmptyPrompt


Nit: You could break that inline component out into its own component.

Updated here 7963cae

walterra · 2024-01-03T15:56:29Z

...c/application/data_drift/document_count_chart_single_brush/document_count_chart_singular.tsx

+            locale={i18n.getLocale()}
+          />
+          <Axis
+            id="data-drift-histogram-left-axis"


I wonder if we have to create more dynamic ids here since the chart will show up multiple times in the UI? Or is this just something internal to the code since it's canvas based and the id might not end up in the DOM?

Updated here 7963cae

walterra · 2024-01-03T16:02:18Z

..._visualizer/public/application/data_drift/document_count_chart_single_brush/single_brush.tsx

+
+/**
+ * SingleBrush React Component
+ * Dual brush component that overlays the document count chart


Dual brush component -> Single brush component

Updated here 7963cae

walterra · 2024-01-04T07:39:44Z

..._visualizer/public/application/data_drift/document_count_chart_single_brush/single_brush.tsx

+
+  return {
+    min: Math.round(baselineMin),
+    max: Math.round(baselineMax),


Suggest to use the deviation time range here. The baseline is a time range before the deviation and has quite an offset to where the user clicked. See the following example, I'm clicking on the spike in the dataset but the selected time range will be way behind it:

walterra

Overall I agree it's the right thing to do here to duplicate the charts/brush code from AIOps first to get this feature in. This way there's no risk to introduce regressions in the original code. However, there's some potential for code consolidation. Some of the code related to the brushes is quite complex and diverged versions will be tricky to maintain in the long run. Suggest to do the following:

Create an issue that documents which files have been duplicated, similar to what we did for AIOps here: [ML] AIOps Labs: Code consolidation. #136182
Add comments to the duplicated files that reference the original file, here's an example: https://github.com/elastic/kibana/blob/main/x-pack/plugins/aiops/public/application/utils/search_utils.ts#L8
As a follow up in the long run we should move the different chart and brush versions to a new package. There they might still be different components, but we can at least consolidate some types and utilities that overlap.

walterra · 2024-01-04T07:58:16Z

...c/application/data_drift/document_count_chart_single_brush/document_count_chart_singular.tsx

+ *
+ * @param windowParameters Baseline and deviation time ranges.
+ * @param force Force update
+ * @param logRateAnalysisType `spike` or `dip` based on median log rate bucket size


This can be removed.

Updated here 42cedad

walterra · 2024-01-04T07:58:46Z

...c/application/data_drift/document_count_chart_single_brush/document_count_chart_singular.tsx

+/**
+ * Callback function which gets called when the brush selection has changed
+ *
+ * @param windowParameters Baseline and deviation time ranges.


This mentions baseline/deviation which isn't applicable to the singular chart.

Updated here 42cedad

walterra · 2024-01-04T08:01:03Z

...c/application/data_drift/document_count_chart_single_brush/document_count_chart_singular.tsx

+
+  function onWindowParametersChange(
+    wp: SingleBrushWindowParameters,
+    wpPx: SingleBrushWindowParameters


The wpPx arg can be removed, it's not used.

Updated here 42cedad

walterra · 2024-01-04T08:03:15Z

...c/application/data_drift/document_count_chart_single_brush/document_count_chart_singular.tsx

+          {windowParameters && (
+            <>
+              <DualBrushAnnotation
+                id="aiopsBaseline"


aiopsBaseline -> data-drift-annotation

Updated here 42cedad

walterra · 2024-01-04T08:09:26Z

..._visualizer/public/application/data_drift/document_count_chart_single_brush/single_brush.tsx

+            return 'data-drift-' + b.id;
+          })
+          .attr('data-test-subj', (b: SingleBrush) => {
+            // Uppercase the first character of the `id` so we get aiopsBrushBaseline/aiopsBrushDeviation.


Comment still mentions aiops and both brushes.

Updated here 42cedad

…ed-charts

…-fix'

peteharverson

Tested latest changes and LGTM

walterra

Latest changes LGTM.

qn895 · 2024-01-05T17:28:59Z

Created a PR for the code consolidation here #174377, along with TODO comments 0fa5e00

…-fix'

qn895 · 2024-01-10T15:42:28Z

@elasticmachine merge upstream

kibana-ci · 2024-01-10T21:05:11Z

💚 Build Succeeded

Buildkite Build
Commit: f88bb8e

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dataVisualizer`	576	580	+4

Async chunks

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

id	before	after	diff
`dataVisualizer`	616.3KB	613.6KB	-2.7KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`dataVisualizer`	47	51	+4

Total ESLint disabled count

id	before	after	diff
`dataVisualizer`	47	51	+4

History

💔 Build #186880 failed d33969b99babacab8e0f8c64462d6cc92911245c
💔 Build #186832 failed 5dd4c0c
💔 Build #186180 failed 0f70e0b
💔 Build #186173 failed 0fa5e00
💔 Build #185988 failed 37877d0
💔 Build #185981 failed 04d392a

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

cc @qn895

…s process (elastic#174049) ## Summary Enhances the selection behavior in the Data Drift view to allow time ranges which overlap in the Reference and Comparison data sets to be analyzed. Improvements include: - Allow for the Reference and Comparison time range to be able to overlap - Click on each chart to set the time range - Prompts and hints for user https://github.com/elastic/kibana/assets/43350163/55f01e41-76e0-4a23-a41c-137349a84bf1 ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <[email protected]>

qn895 added 19 commits December 23, 2023 10:54

Working stuff

4c5cc0f

Revert changes for dual brush

97de650

Rename document count dual to single brush in data drift

535dda7

Update colors between

e5409ff

Refactor name to min/max for clarity

c527d7c

Fix bar style

fba2a04

Fix

cb370e8

Clean up

5a4456b

More clean up before refactoring

a90dfa5

Update theme

dd0b417

Remove object.freeze

7327a6d

Move single brush stuff over to data_viz

07f9d88

Update imports, clean up redundant code

99ee220

Fix types

c8fa2a6

Update colors

7099a80

Add help text

a852fae

Update welcome screen

62088eb

Update ids

f2adfe5

Revert isWindowParameters change and todos

2bf860c

qn895 self-assigned this Dec 29, 2023

qn895 added :ml release_note:feature Makes this part of the condensed release notes v8.13.0 labels Dec 29, 2023

qn895 requested a review from walterra January 2, 2024 16:30

qn895 added 3 commits January 2, 2024 17:11

Merge remote-tracking branch 'upstream/main' into ml-data-drift-updat…

75d501f

…ed-charts

Fix i18n

4711fca

Update translations

61faa07

qn895 marked this pull request as ready for review January 2, 2024 23:23

qn895 requested review from a team as code owners January 2, 2024 23:23

peteharverson reviewed Jan 3, 2024

View reviewed changes

walterra reviewed Jan 3, 2024

View reviewed changes

Refactor prompt/hint component

7963cae

walterra reviewed Jan 4, 2024

View reviewed changes

qn895 and others added 4 commits January 4, 2024 16:26

Disable button if there's only one time range selected

42cedad

Merge remote-tracking branch 'upstream/main' into ml-data-drift-updat…

04d392a

…ed-charts

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

6f33068

…-fix'

Remove scss

37877d0

qn895 removed the request for review from a team January 4, 2024 23:06

peteharverson approved these changes Jan 5, 2024

View reviewed changes

walterra approved these changes Jan 5, 2024

View reviewed changes

Add todo comments for future code consolidation

0fa5e00

qn895 mentioned this pull request Jan 5, 2024

[ML] Data drift: Code Consolidation #174377

Open

3 tasks

qn895 enabled auto-merge (squash) January 5, 2024 17:29

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

0f70e0b

…-fix'

kibanamachine and others added 2 commits January 10, 2024 10:42

Merge branch 'main' into ml-data-drift-updated-charts

5dd4c0c

Fix tests

f88bb8e

qn895 force-pushed the ml-data-drift-updated-charts branch from d33969b to f88bb8e Compare January 10, 2024 20:01

qn895 merged commit deebfaf into elastic:main Jan 10, 2024

kibanamachine added the backport:skip This commit does not require backporting label Jan 10, 2024

peteharverson added release_note:enhancement and removed release_note:feature Makes this part of the condensed release notes labels Feb 14, 2024

szabosteve changed the title ~~[ML] Improve Data drift time range selection & show hints for analysis process~~ [ML] Improves Data drift time range selection & show hints for analysis process Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Improves Data drift time range selection & show hints for analysis process #174049

[ML] Improves Data drift time range selection & show hints for analysis process #174049

qn895 commented Dec 29, 2023 •

edited

Loading

peteharverson Jan 3, 2024

qn895 Jan 4, 2024 •

edited

Loading

peteharverson Jan 3, 2024

qn895 Jan 4, 2024 •

edited

Loading

walterra Jan 3, 2024

qn895 Jan 4, 2024

walterra Jan 3, 2024

qn895 Jan 4, 2024

walterra Jan 3, 2024

qn895 Jan 4, 2024 •

edited

Loading

walterra Jan 4, 2024

walterra left a comment

walterra Jan 4, 2024

qn895 Jan 4, 2024

walterra Jan 4, 2024

qn895 Jan 4, 2024

walterra Jan 4, 2024

qn895 Jan 4, 2024

walterra Jan 4, 2024

qn895 Jan 4, 2024

walterra Jan 4, 2024

qn895 Jan 4, 2024

peteharverson left a comment

walterra left a comment

qn895 commented Jan 5, 2024

qn895 commented Jan 10, 2024

kibana-ci commented Jan 10, 2024

ESLint disabled line counts

Total ESLint disabled count

[ML] Improves Data drift time range selection & show hints for analysis process #174049

[ML] Improves Data drift time range selection & show hints for analysis process #174049

Conversation

qn895 commented Dec 29, 2023 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

qn895 Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qn895 Jan 4, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qn895 Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterra 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

qn895 commented Jan 5, 2024

qn895 commented Jan 10, 2024

kibana-ci commented Jan 10, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

qn895 commented Dec 29, 2023 •

edited

Loading

qn895 Jan 4, 2024 •

edited

Loading

qn895 Jan 4, 2024 •

edited

Loading

qn895 Jan 4, 2024 •

edited

Loading