-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Discover] Add support for log overview tab to Discover log profile #186680
[Discover] Add support for log overview tab to Discover log profile #186680
Conversation
Hi @davismcphee, could you help us understand the intended scope of this effort? Is the goal to support the tab exactly as it exists in the Logs Explorer or does it deviate somehow? |
@weltenwort The intent is to support it exactly as it exists in Logs Explorer, except for the O11y AI assistant, which should not yet be shown in Discover. |
component: (props) => ( | ||
<UnifiedDocViewerLogsOverview | ||
{...props} | ||
renderAIAssistant={plugins.logsShared.renderLogsAIAssistant} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
What if, instead of exporting a new logsShared.renderLogsAIAssistant
renderer and introducing the dependency on logsShared
, we directly consume this from the discoverShared
registered feature as it was doing directly in the UnifiedDocViewer? It should be fine to introduce the dependency on discover-shared and consume the registered feature, also avoiding to update the signature of the logsShared plugin at all, wdyt?
const logsAIAssistantFeature = discoverShared.features.registry.getById(
'observability-logs-ai-assistant'
);
registry.add({
id: 'doc_view_logs_overview',
title: i18n.translate('xpack.logsExplorer.docViews.logsOverview.title', {
defaultMessage: 'Overview',
}),
order: 0,
component: (props) => (
<UnifiedDocViewerLogsOverview
{...props}
renderAIAssistant={logsAIAssistantFeature.render}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, thanks! I like that better and I suppose that's why we introduced discoverShared
in the first place 🙂 Updated here: 97ba3c1.
import { LogsOverviewHeader } from './logs_overview_header'; | ||
import { LogsOverviewHighlights } from './logs_overview_highlights'; | ||
import { FieldActionsProvider } from '../../hooks/use_field_actions'; | ||
import { getUnifiedDocViewerServices } from '../../plugin'; | ||
import { LogsOverviewAIAssistant } from './logs_overview_ai_assistant'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
This file could be removed right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it can, removed here: 97ba3c1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @davismcphee, the taken approach LGTM, I just left a note on the introduced dependency and suggested a change to minimize changes to the plugin API, let me know your thoughts! The rest looks clean and doesn't need changes from the logs team 👌
104fd90
to
8884ab3
Compare
/ci |
3 similar comments
/ci |
/ci |
/ci |
… through discover_shared
e178e78
to
0251167
Compare
/ci |
/ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @tonyghiani! Updated with your suggestions.
component: (props) => ( | ||
<UnifiedDocViewerLogsOverview | ||
{...props} | ||
renderAIAssistant={plugins.logsShared.renderLogsAIAssistant} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, thanks! I like that better and I suppose that's why we introduced discoverShared
in the first place 🙂 Updated here: 97ba3c1.
import { LogsOverviewHeader } from './logs_overview_header'; | ||
import { LogsOverviewHighlights } from './logs_overview_highlights'; | ||
import { FieldActionsProvider } from '../../hooks/use_field_actions'; | ||
import { getUnifiedDocViewerServices } from '../../plugin'; | ||
import { LogsOverviewAIAssistant } from './logs_overview_ai_assistant'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it can, removed here: 97ba3c1.
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kibana-presentation changes LGTM
code review only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not test, but kibana JSONC changes LGTM!
⏳ Build in-progress
History
cc @davismcphee |
Summary
This PR adds the log overview tab from Logs Explorer to the Discover log document profile. The only difference between the tab in Logs Explorer and Discover is that the one in Logs Explorer includes the O11y AI assistant while the Discover one doesn't (for now at least):
Resolves #187096.
Checklist
For maintainers