[One Discover] Add summary column for logs contextual experience

[tonyghiani]

📓 Summary

Closes https://github.com/elastic/logs-dev/issues/165

This work introduces a Summary column as a replacement for the Document one for the Discover logs contextual experience.

We also decided to port this change as a replacement for the resource and content virtual columns in Logs Explorer to have a better alignment between the 2 apps.

🎥 Demo

summary_column_demo_fullhd.mov

💡 Reviewer hints

I left notes through the changes to help answer some questions.

The notable changes on this PR are:

• Rename Document column to Summary.
• Refactor resource and content virtual columns into a single Summary column, which replaces the default Summary content for the logs' contextual experience.
• Provide support for the plugin services to the context awareness profiles.
• Disable virtual columns (clean up will go in a follow-up work) in Logs Explorer and rely on the Summary column as the default selection.

[obltmachine]

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

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

[tonyghiani]

Hi @davismcphee, sorry for the late response, I've addressed most of your suggestions and spent time investigating the design issues you reported.

• Vertical alignment is wonky for Summary cells with no badges when row height is auto fit or custom > 1
• It also seems to be off with row height = 1, but only slightly (maybe slight difference in font size?)
• The cell content has an empty space at the start when row height = 1:
• The truncation ellipsis is not visible for custom row heights:
• When row height = 2, the text content below the badges is not rendered:
• The text content below the badges is cut off for normal or expanded densities:

These issues were mostly caused by the text size, conditional rendering and the badge excessive size, I've been discussing workarounds/solutions with Ryan and the latest EUI changes should allow us to fix most of them.

When row height > 1, it looks like cells with message values display the text content on the same line as the badges, but ones without message values display the formatted result on a separate line. Is this intentional? It looks a bit odd at least.

This doesn't depend on the message existence, it was about the content length.
If the message fits in a single line, it could go next to the badge.
It was initially intentional, but not necessarily a final version. I switched to always have the content on a second line when row height > 1👌

The Summary badge filtering doesn't work in ES|QL mode, and instead adds a DSL filter. We may be able to use the same filtering logic here that we currently use for filtering in non-Summary cells in ES|QL mode.

I fixed it with feat(discover): apply shared actions to summary column, although I'm not particularly happy because the implementation promotes prop drilling at many levels, which increases the maintainability effort for this code. I spent some time refactoring, but it was widely affecting the scope of this work. It might be worth discussing this in future and tackling some refactoring to reduce the props drilling of these data table actions.

Badge filtering is inaccessible to keyboard users since it's disabled in the popover, and you can't enter expandable cells by keyboard in the data grid.

As per this comment on Figma, this was a design choice, I see it comes with an accessibility issue.
Usually, the pattern is that the cell content is not interactive, and once the popover is opened, the focus goes there and the user can navigate the content with the keyboard.
@ryankeairns should we change it so that also the badges in the popover are not purely presentational, but have the same popup for filtering that is shown when clicking the cell badges? I don't see another way otherwise, as the cell content is not interactive by default (I tried interactive wrappers or forcing tab index on those badges, but the EuiDataGrid component is quite strict on this and changes everything back to not interactive.)

The badges also don't respect the density setting, although this may be less of a concern:

I think you mean the size of the badges doesn't scale with density changes. If so, EuiBadge has a unique size, so we cannot make it bigger/smaller based on the grid density.

Another thing is that some functional tests for these changes would be valuable here. I see a lot of the old Logs Explorer tests have been skipped, but there don't seem to be equivalent Summary column replacements for them. Maybe this was intended as part of the follow up to remove smart fields code? Personally I'd prefer to have some as part of the initial PR.

The skipped tests are purely focused to check if the virtual columns were rendered. For the summary column, I added a Jest test for the content, and with test(discover): add FT for summary column displaying I updated the FT test for the cell renderer extension point, I think is ok to start like this since we might change more on this column soon.

And lastly I need to confirm if we are good to merge this as part of the shared logs profile. Part of me feels like we discussed making it an O11y specific thing, but I honestly can't remember what the decision was...

Af far as I remember, this was intended to be a logs feature, as it purely relies on logs concepts, there is no specific Observability knowledge in the rendered data (apart from navigation to entities).

I'll keep polishing the implementation but I think is ready for another round of review 👍

[ryankeairns]

Badge filtering is inaccessible to keyboard users since it's disabled in the popover, and you can't enter expandable cells by keyboard in the data grid.

As per this comment on Figma, this was a design choice, I see it comes with an accessibility issue.
Usually, the pattern is that the cell content is not interactive, and once the popover is opened, the focus goes there and the user can navigate the content with the keyboard.
@ryankeairns should we change it so that also the badges in the popover are not purely presentational, but have the same popup for filtering that is shown when clicking the cell badges? I don't see another way otherwise, as the cell content is not interactive by default (I tried interactive wrappers or forcing tab index on those badges, but the EuiDataGrid component is quite strict on this and changes everything back to not interactive.)

@tonyghiani I recall that thread and tried this on the oblt instance where this capability still exists. Using it makes me now feel it is better to leave it as it was (keyboard accessible; popovers on top of a modal) rather than remove the functionality. I don't have bandwidth to take this on - and I don't know that @l-suarez does either - so, in the meantime, I believe it would be preferable to keep it keyboard accessible until a more-complete alternative arises.

[logeekal]

Looks good from the investigations side. Thanks.

[davismcphee]

Great work on the updates! Basically all of the issues I reported have been resolved now, and it's looking nice and polished 👌 There are just a couple of final things left to address and this will be good to go on my end. We'll also clarify tomorrow in the sync that we're good to merge this into the generic logs profile just to be certain.

I think you mean the size of the badges doesn't scale with density changes.

Yeah I was referring to the text size of the badges, but if the scale is hardcoded, we're definitely good to leave it.

I updated the FT test for the cell renderer extension point, I think is ok to start like this since we might change more on this column soon.

Agreed, these tests should be good for now, thanks!

[davismcphee]

I definitely agree there are challenges around vertically aligning the badges in cells, but in this case I think the magic pixel number results in a better UX than vertically centering for multiple lines:

I also think the upcoming EUI changes will fix the issue with badges being cut off for custom row height = 1, which gets rid of part of the issue. In this case I'd recommend we revert these changes to keep the negative margin and potentially address this separately as a followup if there's a better approach.

[davismcphee]

All of the remaining comments have been resolved, and we got confirmation we're good to merge this into the generic logs profile. There's just one tiny thing left to do (revert some changes), then I promise I'll get out of the way and let this get merged 🙏

[davismcphee]

Unfortunately it looks like these changes never got reverted and are still present in the PR branch:

Could we please revert these as the final task on this PR?

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: cd0d7df
• Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-192567-cd0d7df39993

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
canvas	1292	1291	-1
cloudSecurityPosture	652	651	-1
discover	998	992	-6
esqlDataGrid	364	363	-1
eventAnnotationListing	579	578	-1
lens	1462	1461	-1
logsExplorer	563	562	-1
observability	1061	1060	-1
searchPlayground	242	241	-1
securitySolution	5832	5831	-1
slo	845	844	-1
unifiedDocViewer	220	219	-1
unifiedHistogram	186	185	-1
total			-18

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	90	110	+20

Async chunks

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

id	before	after	diff
cloudSecurityPosture	506.5KB	506.8KB	+317.0B
discover	815.4KB	819.2KB	+3.8KB
esqlDataGrid	154.0KB	154.3KB	+318.0B
logsExplorer	222.7KB	222.7KB	+1.0B
securitySolution	20.5MB	20.5MB	+1.6KB
slo	855.1KB	855.4KB	+317.0B
unifiedDocViewer	124.3KB	124.3KB	-75.0B
total			+6.2KB

Page load bundle

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

id	before	after	diff
discover	49.0KB	49.0KB	+14.0B
logsExplorer	27.5KB	27.5KB	-3.0B
total			+11.0B

Unknown metric groups

API count

id	before	after	diff
@kbn/unified-data-table	165	185	+20

async chunk count

id	before	after	diff
discover	27	28	+1

ESLint disabled line counts

id	before	after	diff
discover	24	27	+3

Total ESLint disabled count

id	before	after	diff
discover	26	29	+3

History

• 💚 Build #237249 succeeded 9199821
• 💔 Build #237229 failed 4afb2a0
• 💛 Build #237053 was flaky 505dbc1
• 💛 Build #236932 was flaky a3ac8fe
• 💛 Build #236893 was flaky a2cd794

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

[davismcphee]

Looks like we're all good to go now, so let's merge this thing 🤘 Awesome work on this, thanks! It's a huge step forward for One Discover and I'm excited to get it out to users soon.