[Logs Explorer] Add customisation for virtual columns and add the 1st virtual column

[achyutjhunjhunwala]

Summary

• Closes [Logs Explorer] Add support for virtual columns in the data table #171844
• Closes [Logs Explorer] Add a new "content" virtual column in the data table #171726
• Closes [Logs Explorer] Add support for injecting custom cell actions in Virtual Columns #173825

This PR does 4 things

• Customisation point to inject customCellRenderers in the Data Table.
• Adding the 1st virtual column - Content and removal of Message column
• Add cell actions for virtual columns so that the custom fields can be filtered for
• Renders similar content on the Flyout instead of Message

To make this PR easy to review

Why Kibana Operations

• This PR initially increased the bundle size causing the values to go beyond the boundaries set in limits.yml. This has been fixed now after suggestion from Kibana Ops team

For Kibana Data Discovery Team - Changes done on Unified Data Table

• Moving around types to make them more generic and reusable
• Split the 2 components SourceDocument and SourceDocumentPopover out of the 1 single large file ger_render_cell_value.tsx they were in. This allows me to export them and lazy load the components in my plugin.
• Passed additional parameters to externalCustomRenderers function

For Kibana Data Discovery Team - Changes done on Discover Plugin

• Currently the discover plugin was not passing the registered customisation for externalCustomRenderers. Simply added the hook to retrieve the registered customisation and pass it as props.
• Added new customisation type for CellRendererCustomisation

For Logs UX Team - Changes done on Log Explorer

• Registered a new customisation for custom Virtual columns.
• Rendering logic for Log Level and Message with fallbacks
• When Message and its fallbacks are not present, we must render the whole document. Used extracted logic to display complete document as it appears in current Discover.
• When a JSON column is expanded (expandCellAction), it should render a JSON viewer as it currently does in Discover
• Items like Log Level which are rendered inside a Chip should allow contextual actions like Filter In, Filter Out and Copy

Want to run this PR locally

// Checkout this PR
gh pr checkout 173732

// Start Kibana locally
yarn start 

// Start ES locally
yarn es snapshot

// Generate dummy data
node scripts/synthtrace simple_logs.ts --clean

Live Demo can be seen here

Demo

What's pending - Basically why is the PR still in Draft Stage

• Fix colors for logs levels after confirmation from Isa and hide level when not present
• Fix JSON rendering on the content cell when message field is not present so that it looks like exactly how it looks in Discover
• Add E2E tests
• Update DEMO with a gif rather than a screenshot

[apmmachine]

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

• /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
• /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[achyutjhunjhunwala]

/ci

[achyutjhunjhunwala]

/ci

[achyutjhunjhunwala]

/ci

[achyutjhunjhunwala]

/ci

[elasticmachine]

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

[achyutjhunjhunwala]

The data view warning is gone now, well done. We're almost there, I think.

One small thing I noticed is, that we don't have the ability to filter-in/filter-out the message or its fallsbacks via the cell pop-over.

Do you know if that would be possible to add back? It works in the fly-out content section:

@weltenwort @isaclfreire

I am not sure how the filter action on the Table would look like for the message field.
It brings certain complexity which are not present in the Flyout.

1. In the Flyout, we only render the Content block when message or any fallbacks are present. Where as in the table we render the Raw Documents when none is present. In this case we don't want user to add that as a filter. Conditional rendering means not all cells in the Table would have let say the same actions. Not sure if we want that.

2. Also the Log Level Chip provides a Arrow on the right hand side giving visual hints that there is a possible action if clicked on. For Message field, we don't have any such visual hint. I am not sure how we want to handle this. Open for suggestions

[weltenwort]

Visually these just would be the data grid hover and popover actions. For what happens when the user clicks two options come to mind:

• Just filter for the message/error.message/event.original. This would mean there's no action for the "full document" fall-back and this wouldn't work for the resource column, because there's no obvious "main" field.
• Filter for a combination of the log level and message. This still wouldn't make sense for the "full document", but at least it would be applicable for the resource column (i.e. "filter for this exact combination").

We can split this discussion and change into a follow-up PR to get this one merged soon if you like.

[weltenwort]

Is the @timestamp or another field supposed to be bold when the whole JSON document is rendered in the content column?

@gbamparop That was intentional. The idea was to distinguish the keys and values similar to what Discover does when no column is added. What alternative would you suggest?

[isaclfreire]

Since Content is a smart field created from the grouping of many fields, the initial thought was to allow users to be able to filter by log.level, and rely on the 'Insights' area to filter from message patterns (5 most common / least common), as shown below.

I am aware though this doesn't exactly address the scenarios listed above, so if we want to at least keep the original behaviour (described by Felix in point 1), of being able to filter by the message/error.message/event.orignal, I will need to dedicate some time to understand how this can be done visually. This might imply a complete change in the cell filtering design.

I was also made aware today that the Discover team is slightly changing the data grid cell actions, as described in elastic/eui#7343. Worth it giving a look.

[achyutjhunjhunwala]

@weltenwort @isaclfreire Given the size, complexity of the PR and next week being On-Week, i am in favour of splitting the task of how we handle Filtering behavior for Message Field in the Table separately outside the scope of this PR.

On the contrary, i also like Isa's approach that we can leave the message field like that in the table as this is a special column in the table. This way irrespective of the message, fallback or whole document, we keep consistency. But yes, let's move this to separate discussion.

@isaclfreire Thanks for sharing the Datagrid EUI changes. Looking at their changes on a high level, it seems we will auto inherit those features.

[weltenwort]

Ok, let's tackle the cell actions in a follow-up. Even if we don't want to support the filter actions, the "copy" action would probably be quite useful for users.

[davismcphee]

Great work! I left a few minor points of feedback, but assuming those get addressed, the Discover changes LGTM.

I didn't do extensive testing on the Logs Explorer side, so my review is primarily on the Discover changes, but I poked around a bit and noticed a couple things worth mentioning. I'll approve once the Discover feedback has been addressed though, and leave it to your team to decide what to do with this feedback.

It looks like removing a column from the sidebar causes Logs Explorer to crash with a React hooks related error:

remove.mp4

After opening a log level badge popover, clicking on the badge again to close it causes the popover to get stuck open. This is most often caused by using onClick={() => setIsPopoverOpen(true)} instead of onClick={() => setIsPopoverOpen(!isPopoverOpen)}:

popover.mp4

These next ones aren't necessarily bugs, just some UX feedback.

Unfortunately it looks like the EUI upgrade broke the vertical alignment on the log level badge:

I agree with @kertal about the column based approach to the content cell. For example, it causes the cell layout to look odd IMO when setting the row height to auto:

It also feels like by default there's too little space available for the content cell compared to the service.name and host.name cells. This can be improved by collapsing the sidebars, but it might be worth reconsidering the defaults here IMO:

[achyutjhunjhunwala]

@davismcphee Thank you for the detailed review. Looks like i introduced some regressions while fixing other code reviews. I have fixed most of them, answering them here,

It looks like removing a column from the sidebar causes Logs Explorer to crash with a React hooks related error:

Good catch, fixed it now.

After opening a log level badge popover, clicking on the badge again to close it causes the popover to get stuck open

Fixed

Unfortunately it looks like the EUI upgrade broke the vertical alignment on the log level badge:

I didn't understand this one. I see this as expected. The text and the arrow is center aligned to the badge. Which means they will not be vertically aligned with each other.

I agree with @kertal about the column based approach to the content cell. For example, it causes the cell layout to look odd IMO when setting the row height to auto:

I am not sure if we are setting row height as Auto in Log Explorer for the data table. Or is it some user level setting ?
I will loop in @weltenwort and @isaclfreire to comment on here.

It also feels like by default there's too little space available for the content cell compared to the service.name and host.name cells. This can be improved by collapsing the sidebars, but it might be worth reconsidering the defaults here IMO:

We have more user stories coming in where the regular columns like service.name and host.name will be replaced by a single column called resource column. This should create more space for the content column.
@isaclfreire fyi

UPDATE

@davismcphee We had an internal discussion b/w myself, @isaclfreire and @weltenwort.
We would want to keep the Log Level as part of the Content column for the moment. The table would already limit the number of columns when it loads initially to provide enough space for the Content column. Also, If the user adds more columns, they should also be able to drag and change the column width as per their need. After releasing this feature, we will collect some feedbacks (internal+external) and revisit this topic later.

[weltenwort]

I am not sure if we are setting row height as Auto in Log Explorer for the data table. Or is it some user level setting?

For the Logs Explorer the row height defaults to "single".

[davismcphee]

Thanks for addressing the PR feedback, the Data Discovery changes LGTM 👍 And I confirmed the sidebar and popover issues are working as expected now too.

I didn't understand this one. I see this as expected. The text and the arrow is center aligned to the badge. Which means they will not be vertically aligned with each other.

Sorry, I should have been a bit more specific with what I meant here. It looks like originally the badge was vertically aligned to the center of the row, but after the last EUI upgrade it's aligned to the bottom of the row instead.

Before upgrade:

After upgrade:

I am not sure if we are setting row height as Auto in Log Explorer for the data table. Or is it some user level setting ?

By default it seems to be set to single, but this can be changed by users through the display options button above the grid:

We have more user stories coming in where the regular columns like service.name and host.name will be replaced by a single column called resource column. This should create more space for the content column.

This makes sense, and I agree it would likely improve the current situation with limited space.

We would want to keep the Log Level as part of the Content column for the moment. The table would already limit the number of columns when it loads initially to provide enough space for the Content column.

Oops, my feedback here was also a bit vague. I didn't mean that I think a separate data table column would be better for the log level, just that the current use of flex columns results in a lot of whitespace when the content wraps. Placing the badge inline with the content so the text can wrap around it might make better use of the available space.

That said, none of this feedback is blocking or anything, so no need for me to hold up this PR 🙂

[achyutjhunjhunwala]

@davismcphee Thank you for the approval

1. Regarding the EUI change, i discussed this with the EUI team on the Slack channel. Post that we decided to leave it like that for the moment.
2. Same with the multiline change via the control. At the moment we default it to single, but i agree users can change it. i discussed this with our team internally and we will reiterate this over time to improve it.

[weltenwort]

Great job! 👏 Thanks for all the attention to detail!

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 757d81a

Failed CI Steps

• FTR Configs #29

Test Failures

• [job] [logs] FTR Configs #29 / discover/group1 discover test query #2, which has an empty time range should show matches when time range is expanded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
cloudSecurityPosture	402	405	+3
discover	690	694	+4
logExplorer	638	648	+10
total			+17

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
@kbn/unified-data-table	49	51	+2

Async chunks

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

id	before	after	diff
cloudSecurityPosture	413.3KB	414.1KB	+747.0B
discover	557.7KB	558.6KB	+921.0B
logExplorer	981.7KB	985.1KB	+3.4KB
total			+5.0KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
discover	21	22	+1

Page load bundle

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

id	before	after	diff
logExplorer	21.9KB	36.4KB	+14.5KB

Unknown metric groups

API count

id	before	after	diff
@kbn/unified-data-table	109	111	+2

async chunk count

id	before	after	diff
logExplorer	6	9	+3

ESLint disabled line counts

id	before	after	diff
@kbn/unified-data-table	4	7	+3

Total ESLint disabled count

id	before	after	diff
@kbn/unified-data-table	4	7	+3

History

• 💛 Build #186401 was flaky 31d894d
• 💚 Build #186371 succeeded e63ecfa
• 💔 Build #186331 failed d7fc432
• 💛 Build #186166 was flaky 51592c3
• 💔 Build #186150 failed 4965a7c
• 💔 Build #186114 failed b98a299

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

cc @achyutjhunjhunwala