[ML] AIOps: Adds expanded rows to pattern analysis table #175320

jgowdyelastic · 2024-01-23T15:21:50Z

Changes the syntax highlighting of the example text so that it is now based on the pattern's tokens, rather than being the default "log" format supplied by EuiCode
Adds an expanded row to the able which lists the tokens, the regex and 4 examples so we can see how the non-token parts of each example vary.

Highlighting colors have been copied from the EuiCode component.

jgowdyelastic · 2024-01-24T11:37:37Z

/ci

jgowdyelastic · 2024-01-24T17:21:20Z

/ci

elasticmachine · 2024-01-24T17:32:58Z

Pinging @elastic/ml-ui (:ml)

walterra

Great improvement! Left a view comments:

walterra · 2024-01-26T08:33:25Z

x-pack/plugins/aiops/public/components/log_categorization/category_table/category_table.tsx

+      isExpander: true,
+      render: (item: Category) => (
+        <EuiButtonIcon
+          data-test-subj="aiopsColumnsButton"


Nit: Suggest to use aiopsLogPatternsColumnsButton.

Updated in 5165088

walterra · 2024-01-26T08:51:43Z

x-pack/plugins/aiops/public/components/log_categorization/category_table/expanded_row.tsx

+
+export const ExpandedRow: FC<ExpandedRowProps> = ({ category }) => {
+  return (
+    <div css={{ marginRight: '40px', width: '100%' }}>


Suggest to break that out and use EUI theme vars, something like:

import { css } from '@emotion/react'; import { useEuiTheme } from '@elastic/eui'; export const ExpandedRow: FC<ExpandedRowProps> = ({ category }) => { const { euiTheme } = useEuiTheme(); const cssExpandedRow = css({ marginRight: euiTheme.size.xxl, // =40px width: '100%' }); return ( <div css={cssExpandedRow}> ...

We use a similar pattern in x-pack/plugins/aiops/public/components/mini_histogram/mini_histogram.tsx.

Updated in 5165088

walterra · 2024-01-26T09:07:45Z

x-pack/plugins/aiops/public/components/log_categorization/format_category.test.tsx

+      expect(<FormattedPatternExamples category={category} />).toMatchSnapshot();
+      expect(<FormattedRegex category={category} />).toMatchSnapshot();
+      expect(<FormattedTokens category={category} />).toMatchSnapshot();


I'd like to suggest that we try to avoid relying on snapshot testing for new unit tests. A while ago we also documented old tests that we wanted to refactor and no longer rely on snapshot tests (#153288). The snapshots render the component with its props but do not assert what's actually rendered.

An alternative with react-testing-lib would look something like:

import {render, screen} from '@testing-library/react'; import '@testing-library/jest-dom'; ... describe('Format category components', () => { it('renders FormattedPatternExamples correctly', async () => { render(<FormattedPatternExamples category={category} />); expect(screen.getByText(/some text we expect to be rendered/i)).toBeInTheDocument(); }); ... });

You could also adapt this to assert just certain pattern elements you'd expect to be rendered within span elements for the custom highlighting.

All I'm really interested here is the number of elements created, as in, how the line has been split up.
My original version of this test did just that, I'd count the elements and ensure that the line had been split up correctly.
Then I noticed that I'd get the same result by using snapshot.
I can revert this to counting the elements. Shame I deleted the code.

I've updated the test in ff9785b
Reverting it back to a test to count the number of elements returned from createFormattedExample

walterra · 2024-01-26T09:09:21Z

x-pack/plugins/aiops/public/components/log_categorization/format_category.tsx

+    return elements;
+  }
+
+  return { createFormattedExample };


Nit: This could be just return createFormattedExample;.

Updated in 5165088

peteharverson · 2024-01-26T11:08:50Z

x-pack/plugins/aiops/public/components/log_categorization/format_category.tsx

+const tokenStyleLight = css`
+  color: #765b96;
+`;
+const wildcardStyleLight = css`


What do you think about dropping the font-weight for the wildcard style? With equal weights given to the tokens and wildcards, visually the tokens don't stand out from the variable parts of the message for me.

This is with font-weight: 500 for example:

I'm happy to change stying here, but before I do I think it's worth discussing which parts of the text do we consider to be more important?
The tokens, i.e. the shared words in each example, or the wildcard matches, i.e. the text which differs in each example?
I think there is an argument that the varying parts of the examples are more (or equally) interesting to the user. They'd want to see why these examples are different to each other.

Yes good point. It was just an initial impression. I did think the tokens were more important as we're displaying patterns here, but highlighting the variable parts of the messages is also valuable. Happy to stick with this for now and see if there's any future feedback.

…ub.com:jgowdyelastic/kibana into adding-expanded-rows-to-pattern-analysis-table

peteharverson

Tested and LGTM. A great enhancement!

Could we use this same styling / approach in the other parts of the UI where we display similar information?

For example the expanded row in the anomalies table for categorization jobs:

And the categorization job wizard:

walterra · 2024-01-26T12:59:42Z

x-pack/plugins/aiops/public/components/log_categorization/format_category.tsx

+  return isDarkTheme
+    ? {
+        tokenStyle: tokenStyleDark,
+        wildcardStyle: wildcardStyleDark,
+      }
+    : {
+        tokenStyle: tokenStyleLight,
+        wildcardStyle: wildcardStyleLight,
+      };


Nit: This could be wrapped in a useMemo so it returns the same instance on every call as long as isDarkTheme doesn't change.

Updated in c45c849

walterra · 2024-01-26T13:01:41Z

x-pack/plugins/aiops/public/components/log_categorization/format_category.tsx

+export const useCreateFormattedExample = () => {
+  const { tokenStyle, wildcardStyle } = useStyles();
+
+  function createFormattedExample(key: string, example: string): JSX.Element[] {


Nit: This function could be wrapped in useCallback() to avoid returning a new instance every time. Otherwise I think the useMemo in FormattedPatternExamples might not memoize as intended.

Updated in c45c849

walterra · 2024-01-26T13:04:18Z

x-pack/plugins/aiops/public/components/log_categorization/format_category.tsx

+};
+
+const WrapInText: FC = ({ children }) => (
+  <EuiText css={{ fontWeight: 'bold' }} size="s">


Nit: This css could be broken out to a definition outside the component scope. AFAIK the benefit would be that internally this would then be treated as a css class instead of inline css on every element.

As far as I can tell from testing, this still creates the class which is shared between all rows.
I believe this is why we were encouraged to use inline css= rather than style= for jsx.

walterra

Added some more minor suggestions but otherwise LGTM, also did a local test.

kibana-ci · 2024-01-26T16:59:52Z

💚 Build Succeeded

Buildkite Build
Commit: a60f24d

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`aiops`	427	429	+2

Async chunks

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

id	before	after	diff
`aiops`	394.4KB	397.8KB	+3.4KB

History

💔 Build #189589 failed c45c849
💚 Build #189563 succeeded b83a924
💛 Build #189136 was flaky 5ab5061
💔 Build #189086 failed 6ba4df2
💔 Build #189069 failed bbe71a2

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

cc @jgowdyelastic

…175320) Changes the syntax highlighting of the example text so that it is now based on the pattern's tokens, rather than being the default "log" format supplied by `EuiCode` Adds an expanded row to the able which lists the tokens, the regex and 4 examples so we can see how the non-token parts of each example vary. Highlighting colors have been copied from the `EuiCode` component. <img width="1027" alt="image" src="https://github.com/elastic/kibana/assets/22172091/e6068e93-cf52-4c6c-a8ea-2d3dccb08491">

jgowdyelastic added 8 commits January 23, 2024 15:20

[ML] Adding expanded rows to pattern analysis table

5840537

translation ids

056d997

code clean up

3b7736f

code clean up

c960620

translations and code clean up

619fd77

adding tests

a05d315

updating tests

8575e44

removing export

1fb2249

jgowdyelastic added 3 commits January 24, 2024 13:34

fixing dark mode

1a81673

Merge branch 'main' into adding-expanded-rows-to-pattern-analysis-table

0917953

fixing tests

bbe71a2

jgowdyelastic self-assigned this Jan 24, 2024

jgowdyelastic added release_note:enhancement :ml Feature:ML/AIOps ML AIOps features: Change Point Detection, Log Pattern Analysis, Log Rate Analysis v8.13.0 labels Jan 24, 2024

jgowdyelastic changed the title ~~[ML] Adding expanded rows to pattern analysis table~~ [ML] [AIOps] Adding expanded rows to pattern analysis table Jan 24, 2024

jgowdyelastic requested review from peteharverson and walterra January 24, 2024 17:25

jgowdyelastic marked this pull request as ready for review January 24, 2024 17:32

jgowdyelastic requested a review from a team as a code owner January 24, 2024 17:32

jgowdyelastic added 3 commits January 24, 2024 18:16

fixing types

9162fba

Merge branch 'main' into adding-expanded-rows-to-pattern-analysis-table

6ba4df2

Merge branch 'main' into adding-expanded-rows-to-pattern-analysis-table

5ab5061

peteharverson mentioned this pull request Jan 25, 2024

[ML] Increase Test Coverage 8.13.0 #173301

Closed

11 tasks

walterra reviewed Jan 26, 2024

View reviewed changes

changes based on review

5165088

peteharverson reviewed Jan 26, 2024

View reviewed changes

jgowdyelastic added 2 commits January 26, 2024 11:28

updating test

ff9785b

Merge branch 'adding-expanded-rows-to-pattern-analysis-table' of gith…

b83a924

…ub.com:jgowdyelastic/kibana into adding-expanded-rows-to-pattern-analysis-table

peteharverson approved these changes Jan 26, 2024

View reviewed changes

walterra reviewed Jan 26, 2024

View reviewed changes

walterra approved these changes Jan 26, 2024

View reviewed changes

jgowdyelastic added 2 commits January 26, 2024 13:24

hook improvements

c45c849

fixing test

a60f24d

jgowdyelastic merged commit b10f083 into elastic:main Jan 26, 2024
18 checks passed

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

peteharverson changed the title ~~[ML] [AIOps] Adding expanded rows to pattern analysis table~~ [ML] [AIOps] Adds expanded rows to pattern analysis table Feb 14, 2024

peteharverson changed the title ~~[ML] [AIOps] Adds expanded rows to pattern analysis table~~ [ML] AIOps: Adds expanded rows to pattern analysis table Feb 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] AIOps: Adds expanded rows to pattern analysis table #175320

[ML] AIOps: Adds expanded rows to pattern analysis table #175320

jgowdyelastic commented Jan 23, 2024 •

edited

Loading

jgowdyelastic commented Jan 24, 2024

jgowdyelastic commented Jan 24, 2024

elasticmachine commented Jan 24, 2024

walterra left a comment

walterra Jan 26, 2024

jgowdyelastic Jan 26, 2024

walterra Jan 26, 2024

jgowdyelastic Jan 26, 2024

walterra Jan 26, 2024

jgowdyelastic Jan 26, 2024

jgowdyelastic Jan 26, 2024

walterra Jan 26, 2024

jgowdyelastic Jan 26, 2024

peteharverson Jan 26, 2024

jgowdyelastic Jan 26, 2024

peteharverson Jan 26, 2024

peteharverson left a comment

walterra Jan 26, 2024

jgowdyelastic Jan 26, 2024

walterra Jan 26, 2024

jgowdyelastic Jan 26, 2024

walterra Jan 26, 2024 •

edited

Loading

jgowdyelastic Jan 26, 2024 •

edited

Loading

walterra left a comment

kibana-ci commented Jan 26, 2024

[ML] AIOps: Adds expanded rows to pattern analysis table #175320

[ML] AIOps: Adds expanded rows to pattern analysis table #175320

Conversation

jgowdyelastic commented Jan 23, 2024 • edited Loading

jgowdyelastic commented Jan 24, 2024

jgowdyelastic commented Jan 24, 2024

elasticmachine commented Jan 24, 2024

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

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

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

walterra Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

jgowdyelastic Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

kibana-ci commented Jan 26, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

jgowdyelastic commented Jan 23, 2024 •

edited

Loading

walterra Jan 26, 2024 •

edited

Loading

jgowdyelastic Jan 26, 2024 •

edited

Loading