[Logs Overview] Add a flyout to show category document examples #194867
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
…mbeddable-iteration-1
…mbeddable-iteration-1
|
@weltenwort Couple of comments above, but otherwise I've responded to the feedback 👍 |
weltenwort
reviewed
Oct 22, 2024
Comment on lines
+166
to
+167
| core?: CoreStart; | ||
| share?: SharePluginStart; |
There was a problem hiding this comment.
Maybe my confusion about binding the dependencies early below is driven by the fact that I can't seem to figure out why we need those dependencies to be added here and all upstream usage sites. I'm surely missing something obvious, am I not?
There was a problem hiding this comment.
We need share and core for the ServiceNameBadgeWithActions component. Previously this was accessed via context directly (as it lived in Discover). I changed this to accept the dependencies as props as everything was moved to the new package and we can no longer rely on that context.
The SummaryCellPopover ultimately uses this for the component map creation from this line now.
There was a problem hiding this comment.
Ah, thanks for helping me understand that. ❤️
…y350/kibana into 193450-show-category-docs-in-flyout
|
@weltenwort I've removed the prebinding of the component dependencies, these are now passed in at the usage site. And I've amended the translation imports to named imports 👍 |
weltenwort
approved these changes
Oct 23, 2024
x-pack/plugins/observability_solution/logs_shared/public/types.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/logs_shared/public/plugin.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/logs_shared/tsconfig.json
Outdated
Show resolved
Hide resolved
Comment on lines
+166
to
+167
| core?: CoreStart; | ||
| share?: SharePluginStart; |
There was a problem hiding this comment.
Ah, thanks for helping me understand that. ❤️
...y/logs_overview/src/components/log_category_details/log_category_document_examples_table.tsx
Show resolved
Hide resolved
⏳ Build in-progress
History
cc @Kerry350 |
kibanamachine
added
v9.0.0
backport missing
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
@Kerry350 We intended for this to land in 8.x too, right? I'm tempted to add the backport labels myself now but wanted to confirm with you first 🙂 |
Kerry350
added
the
backport:prev-minor
|
Starting backport for target branches: 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Oct 28, 2024
…tic#194867) ## Summary Implements elastic#193450. ## Discover changes⚠️ As part of this we need to render a basic table with the log level and summary columns, which is technically context aware but only in the sense we know we want it to be a logs context up front. The "correct" solution here (or at least from recent conversations) is to use the saved search embeddable. There is upcoming work planned to move log stream component usages over to the saved search embeddable. However, currently this isn't in a place to just be dropped in without some pretty extensive work. I didn't feel comfortable doing a big push on that work as a side effort to this work, especially with a loose (if possible) 8.16 aim for this. What I've done (and which isn't ideal I appreciate) is used the start contract of the Discover plugin to export the columns / cells pre-wrapped with the Discover services. It's not ideal in the sense of dependencies, but technically Discover doesn't use logs shared. I considered Discover shared but that's for registering functionality for Discover, rather than the other way around. Eventually we'll be able to remove this and convert over to the new solution. I'm all ears to a better solution, but there's a big mismatch between the needs here and dropping in something that exists currently. Thankfully the changeset for Discover is small if we're happy to keep this temporarily. Edit: I've made some notes here: elastic/logs-dev#111 (comment) Edit: New package added here: elastic@c290819 ## Overview From a high level: - Adds a new state machine for handling "details" to show in the flyout (document examples now, plus details and a timeline later). - Hooks this up to a flyout expanded from the categories table. - Provides linking to Discover to view documents from the category in the flyout. I've also left some comments inline. ## UI / UX    --------- Co-authored-by: Felix Stürmer <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Felix Stürmer <[email protected]> Co-authored-by: Julia Rechkunova <[email protected]> (cherry picked from commit 6b63f7f)
💚 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
Oct 28, 2024
…#194867) (#197966) # Backport This will backport the following commits from `main` to `8.x`: - [[Logs Overview] Add a flyout to show category document examples (#194867)](#194867) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Kerry Gallagher","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-24T14:49:27Z","message":"[Logs Overview] Add a flyout to show category document examples (#194867)\n\n## Summary\r\n\r\nImplements https://github.com/elastic/kibana/issues/193450.\r\n\r\n## Discover changes⚠️ \r\n\r\nAs part of this we need to render a basic table with the log level and\r\nsummary columns, which is technically context aware but only in the\r\nsense we know we want it to be a logs context up front.\r\n\r\nThe \"correct\" solution here (or at least from recent conversations) is\r\nto use the saved search embeddable. There is upcoming work planned to\r\nmove log stream component usages over to the saved search embeddable.\r\nHowever, currently this isn't in a place to just be dropped in without\r\nsome pretty extensive work. I didn't feel comfortable doing a big push\r\non that work as a side effort to this work, especially with a loose (if\r\npossible) 8.16 aim for this.\r\n\r\nWhat I've done (and which isn't ideal I appreciate) is used the start\r\ncontract of the Discover plugin to export the columns / cells\r\npre-wrapped with the Discover services. It's not ideal in the sense of\r\ndependencies, but technically Discover doesn't use logs shared. I\r\nconsidered Discover shared but that's for registering functionality for\r\nDiscover, rather than the other way around.\r\n\r\nEventually we'll be able to remove this and convert over to the new\r\nsolution. I'm all ears to a better solution, but there's a big mismatch\r\nbetween the needs here and dropping in something that exists currently.\r\nThankfully the changeset for Discover is small if we're happy to keep\r\nthis temporarily.\r\n\r\nEdit: I've made some notes here:\r\nhttps://github.com/elastic/logs-dev/issues/111#issuecomment-2411096251\r\n\r\nEdit: New package added here:\r\nhttps://github.com/elastic/kibana/commit/c290819c1c1e1cb5a67d437cca7783c0e2302c8f\r\n\r\n## Overview\r\n\r\nFrom a high level:\r\n\r\n- Adds a new state machine for handling \"details\" to show in the flyout\r\n(document examples now, plus details and a timeline later).\r\n\r\n- Hooks this up to a flyout expanded from the categories table.\r\n\r\n- Provides linking to Discover to view documents from the category in\r\nthe flyout.\r\n\r\nI've also left some comments inline.\r\n\r\n## UI / UX \r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n---------\r\n\r\nCo-authored-by: Felix Stürmer <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Elastic Machine <[email protected]>\r\nCo-authored-by: Felix Stürmer <[email protected]>\r\nCo-authored-by: Julia Rechkunova <[email protected]>","sha":"6b63f7f6314e9c05525df32629be7ba769c6ab4c","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport missing","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-logs","backport:version"],"title":"[Logs Overview] Add a flyout to show category document examples ","number":194867,"url":"https://github.com/elastic/kibana/pull/194867","mergeCommit":{"message":"[Logs Overview] Add a flyout to show category document examples (#194867)\n\n## Summary\r\n\r\nImplements https://github.com/elastic/kibana/issues/193450.\r\n\r\n## Discover changes⚠️ \r\n\r\nAs part of this we need to render a basic table with the log level and\r\nsummary columns, which is technically context aware but only in the\r\nsense we know we want it to be a logs context up front.\r\n\r\nThe \"correct\" solution here (or at least from recent conversations) is\r\nto use the saved search embeddable. There is upcoming work planned to\r\nmove log stream component usages over to the saved search embeddable.\r\nHowever, currently this isn't in a place to just be dropped in without\r\nsome pretty extensive work. I didn't feel comfortable doing a big push\r\non that work as a side effort to this work, especially with a loose (if\r\npossible) 8.16 aim for this.\r\n\r\nWhat I've done (and which isn't ideal I appreciate) is used the start\r\ncontract of the Discover plugin to export the columns / cells\r\npre-wrapped with the Discover services. It's not ideal in the sense of\r\ndependencies, but technically Discover doesn't use logs shared. I\r\nconsidered Discover shared but that's for registering functionality for\r\nDiscover, rather than the other way around.\r\n\r\nEventually we'll be able to remove this and convert over to the new\r\nsolution. I'm all ears to a better solution, but there's a big mismatch\r\nbetween the needs here and dropping in something that exists currently.\r\nThankfully the changeset for Discover is small if we're happy to keep\r\nthis temporarily.\r\n\r\nEdit: I've made some notes here:\r\nhttps://github.com/elastic/logs-dev/issues/111#issuecomment-2411096251\r\n\r\nEdit: New package added here:\r\nhttps://github.com/elastic/kibana/commit/c290819c1c1e1cb5a67d437cca7783c0e2302c8f\r\n\r\n## Overview\r\n\r\nFrom a high level:\r\n\r\n- Adds a new state machine for handling \"details\" to show in the flyout\r\n(document examples now, plus details and a timeline later).\r\n\r\n- Hooks this up to a flyout expanded from the categories table.\r\n\r\n- Provides linking to Discover to view documents from the category in\r\nthe flyout.\r\n\r\nI've also left some comments inline.\r\n\r\n## UI / UX \r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n---------\r\n\r\nCo-authored-by: Felix Stürmer <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Elastic Machine <[email protected]>\r\nCo-authored-by: Felix Stürmer <[email protected]>\r\nCo-authored-by: Julia Rechkunova <[email protected]>","sha":"6b63f7f6314e9c05525df32629be7ba769c6ab4c"}},"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/194867","number":194867,"mergeCommit":{"message":"[Logs Overview] Add a flyout to show category document examples (#194867)\n\n## Summary\r\n\r\nImplements https://github.com/elastic/kibana/issues/193450.\r\n\r\n## Discover changes⚠️ \r\n\r\nAs part of this we need to render a basic table with the log level and\r\nsummary columns, which is technically context aware but only in the\r\nsense we know we want it to be a logs context up front.\r\n\r\nThe \"correct\" solution here (or at least from recent conversations) is\r\nto use the saved search embeddable. There is upcoming work planned to\r\nmove log stream component usages over to the saved search embeddable.\r\nHowever, currently this isn't in a place to just be dropped in without\r\nsome pretty extensive work. I didn't feel comfortable doing a big push\r\non that work as a side effort to this work, especially with a loose (if\r\npossible) 8.16 aim for this.\r\n\r\nWhat I've done (and which isn't ideal I appreciate) is used the start\r\ncontract of the Discover plugin to export the columns / cells\r\npre-wrapped with the Discover services. It's not ideal in the sense of\r\ndependencies, but technically Discover doesn't use logs shared. I\r\nconsidered Discover shared but that's for registering functionality for\r\nDiscover, rather than the other way around.\r\n\r\nEventually we'll be able to remove this and convert over to the new\r\nsolution. I'm all ears to a better solution, but there's a big mismatch\r\nbetween the needs here and dropping in something that exists currently.\r\nThankfully the changeset for Discover is small if we're happy to keep\r\nthis temporarily.\r\n\r\nEdit: I've made some notes here:\r\nhttps://github.com/elastic/logs-dev/issues/111#issuecomment-2411096251\r\n\r\nEdit: New package added here:\r\nhttps://github.com/elastic/kibana/commit/c290819c1c1e1cb5a67d437cca7783c0e2302c8f\r\n\r\n## Overview\r\n\r\nFrom a high level:\r\n\r\n- Adds a new state machine for handling \"details\" to show in the flyout\r\n(document examples now, plus details and a timeline later).\r\n\r\n- Hooks this up to a flyout expanded from the categories table.\r\n\r\n- Provides linking to Discover to view documents from the category in\r\nthe flyout.\r\n\r\nI've also left some comments inline.\r\n\r\n## UI / UX \r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n---------\r\n\r\nCo-authored-by: Felix Stürmer <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Elastic Machine <[email protected]>\r\nCo-authored-by: Felix Stürmer <[email protected]>\r\nCo-authored-by: Julia Rechkunova <[email protected]>","sha":"6b63f7f6314e9c05525df32629be7ba769c6ab4c"}}]}] BACKPORT--> Co-authored-by: Kerry Gallagher <[email protected]>
kibanamachine
added
v8.17.0
and removed
backport missing
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
Implements #193450.
Discover changes⚠️
As part of this we need to render a basic table with the log level and summary columns, which is technically context aware but only in the sense we know we want it to be a logs context up front.
The "correct" solution here (or at least from recent conversations) is to use the saved search embeddable. There is upcoming work planned to move log stream component usages over to the saved search embeddable. However, currently this isn't in a place to just be dropped in without some pretty extensive work. I didn't feel comfortable doing a big push on that work as a side effort to this work, especially with a loose (if possible) 8.16 aim for this.
What I've done (and which isn't ideal I appreciate) is used the start contract of the Discover plugin to export the columns / cells pre-wrapped with the Discover services. It's not ideal in the sense of dependencies, but technically Discover doesn't use logs shared. I considered Discover shared but that's for registering functionality for Discover, rather than the other way around.
Eventually we'll be able to remove this and convert over to the new solution. I'm all ears to a better solution, but there's a big mismatch between the needs here and dropping in something that exists currently. Thankfully the changeset for Discover is small if we're happy to keep this temporarily.
Edit: I've made some notes here: https://github.com/elastic/logs-dev/issues/111#issuecomment-2411096251
Edit: New package added here: c290819
Overview
From a high level:
Adds a new state machine for handling "details" to show in the flyout (document examples now, plus details and a timeline later).
Hooks this up to a flyout expanded from the categories table.
Provides linking to Discover to view documents from the category in the flyout.
I've also left some comments inline.
UI / UX