[One Discover] Add summary column for logs contextual experience #192567
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 8 commits
September 5, 2024 16:16
tonyghiani
added
release_note:feature
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
…ghiani/kibana into tonyghiani-feature/one-discover-summary-column-copy
…to feature/one-discover-summary-column
…ghiani/kibana into tonyghiani-feature/one-discover-summary-column
…ature/one-discover-summary-column
tonyghiani
added
the
backport:prev-minor
logeekal
approved these changes
Sep 25, 2024
davismcphee
reviewed
Sep 26, 2024
There was a problem hiding this comment.
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!
src/plugins/discover/public/components/data_types/logs/summary_column/content.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/components/data_types/logs/summary_column/content.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/components/data_types/logs/summary_column/content.tsx
Show resolved
Hide resolved
src/plugins/discover/public/components/data_types/logs/summary_column/utils.tsx
Show resolved
Hide resolved
| data-test-subj={dataTestSubj} | ||
| css={badgeCss} | ||
| /> | ||
| <EuiFlexGroup alignItems="center" css={{ height: '100%' }}> |
There was a problem hiding this comment.
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.
src/plugins/discover/public/application/context/context_app_content.tsx
Outdated
Show resolved
Hide resolved
…ight Fix default grid density and row height passed to `getCellRenderers`
davismcphee
reviewed
Sep 26, 2024
| data-test-subj={dataTestSubj} | ||
| css={badgeCss} | ||
| /> | ||
| <EuiFlexGroup alignItems="center" css={{ height: '100%' }}> |
There was a problem hiding this comment.
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?
davismcphee
approved these changes
Sep 27, 2024
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @tonyghiani |
|
Starting backport for target branches: 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Sep 30, 2024
…stic#192567) ## 📓 Summary Closes elastic/logs-dev#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 https://github.com/user-attachments/assets/3dfca483-768e-471c-8735-d5fc8bbd5d00 ## 💡 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. --------- Co-authored-by: Marco Antonio Ghiani <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Davis McPhee <[email protected]> (cherry picked from commit 36a73ce)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Sep 30, 2024
#192567) (#194389) # Backport This will backport the following commits from `main` to `8.x`: - [[One Discover] Add summary column for logs contextual experience (#192567)](#192567) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Marco Antonio Ghiani","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-30T09:08:03Z","message":"[One Discover] Add summary column for logs contextual experience (#192567)\n\n## 📓 Summary\r\n\r\nCloses https://github.com/elastic/logs-dev/issues/165\r\n\r\nThis work introduces a Summary column as a replacement for the Document\r\none for the Discover logs contextual experience.\r\n> We also decided to port this change as a replacement for the\r\n**resource** and **content** virtual columns in Logs Explorer to have a\r\nbetter alignment between the 2 apps.\r\n\r\n## 🎥 Demo\r\n\r\n\r\nhttps://github.com/user-attachments/assets/3dfca483-768e-471c-8735-d5fc8bbd5d00\r\n\r\n## 💡 Reviewer hints\r\n\r\nI left notes through the changes to help answer some questions.\r\n\r\nThe notable changes on this PR are:\r\n- Rename `Document` column to `Summary`.\r\n- Refactor `resource` and `content` virtual columns into a single\r\n`Summary` column, which replaces the default Summary content for the\r\nlogs' contextual experience.\r\n- Provide support for the plugin services to the context awareness\r\nprofiles.\r\n- Disable virtual columns (clean up will go in a follow-up work) in Logs\r\nExplorer and rely on the Summary column as the default selection.\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Antonio Ghiani <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"36a73ce52e2ca14c2d46c3a6504ebaa53fed9f98","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["v9.0.0","release_note:feature","Team:DataDiscovery","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-logs","Project:OneDiscover"],"title":"[One Discover] Add summary column for logs contextual experience","number":192567,"url":"https://github.com/elastic/kibana/pull/192567","mergeCommit":{"message":"[One Discover] Add summary column for logs contextual experience (#192567)\n\n## 📓 Summary\r\n\r\nCloses https://github.com/elastic/logs-dev/issues/165\r\n\r\nThis work introduces a Summary column as a replacement for the Document\r\none for the Discover logs contextual experience.\r\n> We also decided to port this change as a replacement for the\r\n**resource** and **content** virtual columns in Logs Explorer to have a\r\nbetter alignment between the 2 apps.\r\n\r\n## 🎥 Demo\r\n\r\n\r\nhttps://github.com/user-attachments/assets/3dfca483-768e-471c-8735-d5fc8bbd5d00\r\n\r\n## 💡 Reviewer hints\r\n\r\nI left notes through the changes to help answer some questions.\r\n\r\nThe notable changes on this PR are:\r\n- Rename `Document` column to `Summary`.\r\n- Refactor `resource` and `content` virtual columns into a single\r\n`Summary` column, which replaces the default Summary content for the\r\nlogs' contextual experience.\r\n- Provide support for the plugin services to the context awareness\r\nprofiles.\r\n- Disable virtual columns (clean up will go in a follow-up work) in Logs\r\nExplorer and rely on the Summary column as the default selection.\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Antonio Ghiani <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"36a73ce52e2ca14c2d46c3a6504ebaa53fed9f98"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192567","number":192567,"mergeCommit":{"message":"[One Discover] Add summary column for logs contextual experience (#192567)\n\n## 📓 Summary\r\n\r\nCloses https://github.com/elastic/logs-dev/issues/165\r\n\r\nThis work introduces a Summary column as a replacement for the Document\r\none for the Discover logs contextual experience.\r\n> We also decided to port this change as a replacement for the\r\n**resource** and **content** virtual columns in Logs Explorer to have a\r\nbetter alignment between the 2 apps.\r\n\r\n## 🎥 Demo\r\n\r\n\r\nhttps://github.com/user-attachments/assets/3dfca483-768e-471c-8735-d5fc8bbd5d00\r\n\r\n## 💡 Reviewer hints\r\n\r\nI left notes through the changes to help answer some questions.\r\n\r\nThe notable changes on this PR are:\r\n- Rename `Document` column to `Summary`.\r\n- Refactor `resource` and `content` virtual columns into a single\r\n`Summary` column, which replaces the default Summary content for the\r\nlogs' contextual experience.\r\n- Provide support for the plugin services to the context awareness\r\nprofiles.\r\n- Disable virtual columns (clean up will go in a follow-up work) in Logs\r\nExplorer and rely on the Summary column as the default selection.\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Antonio Ghiani <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"36a73ce52e2ca14c2d46c3a6504ebaa53fed9f98"}}]}] BACKPORT--> Co-authored-by: Marco Antonio Ghiani <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📓 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.
🎥 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:
Documentcolumn toSummary.resourceandcontentvirtual columns into a singleSummarycolumn, which replaces the default Summary content for the logs' contextual experience.