-
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
[APM][Infrastructure UI] Enables log flyouts on APM logs tables #132617
[APM][Infrastructure UI] Enables log flyouts on APM logs tables #132617
Conversation
Pinging @elastic/apm-ui (Team:apm) |
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
@@ -223,6 +261,7 @@ Read more at https://github.com/elastic/kibana/blob/main/src/plugins/kibana_reac | |||
jumpToTarget={noop} | |||
reportVisibleInterval={handlePagination} | |||
reloadItems={fetchEntries} | |||
onOpenLogEntryFlyout={flyoutEnabled ? handleOpenLogFlyoutClick : undefined} |
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.
I'm wondering if we should write tests for this. I haven't found any tests for this component, but it could be nice to write some.
}); | ||
|
||
flyoutRef.current = openFlyout( | ||
<KibanaReactContextProvider> |
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.
this is needed because LogEntryFlyout
needs to call some services that exist in this context. openFlyout
isolates the component into a separate mount which doesn't include it
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
This seems to work quite well - good job 👍
I noticed that clicking the actions in the "Investigate" menu doesn't close the fly-out. When opened from the shared component. The reason probably is that the globally scoped overlays service keeps the fly-out alive across the navigation. Do we want to make sure the actions also close the fly-out?
Aside from that I just left a few ideas about the code structure and the component properties below.
if (flyoutRef.current) { | ||
flyoutRef.current.close(); | ||
} |
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.
How about writing this as
if (flyoutRef.current) { | |
flyoutRef.current.close(); | |
} | |
flyoutRef.current?.close(); |
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 catch!
}) => { | ||
const flyoutRef = useRef<ReturnType<KibanaReactOverlays['openFlyout']>>(); |
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.
What do you think about using OverlayRef
from @kbn/core/public
directly here?
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.
cool! much better
@@ -208,6 +217,35 @@ Read more at https://github.com/elastic/kibana/blob/main/src/plugins/kibana_reac | |||
[isLoadingMore, fetchPreviousEntries, fetchNextEntries] | |||
); | |||
|
|||
const handleCloseFlyout = () => { |
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.
This component is getting uncomfortably long. What do you think about bundling the fly-out-related functionality into a useLogEntryFlyout
hook, which could be consumed like
const { closeLogEntryFlyout, openLogEntryFlyout } = useLogEntryFlyout({ sourceId });
?
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.
I totally agree. It could be a little confusing because of the useLogFlyout
hook, but it's worth it
@@ -72,6 +77,7 @@ interface LogStreamContentProps { | |||
center?: LogEntryCursor; | |||
highlight?: string; | |||
columns?: LogColumnDefinition[]; | |||
flyoutEnabled?: boolean; |
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.
💭 I can see requests in the future to also support the other log entry actions in the shared component. We could try to anticipate that by designing this as a log entry row feature configuration property like
logEntryActions?: LogEntryAction[];
where LogEntryAction
could be
interface LogEntryAction {
actionType: 'show-details';
}
This would open up some possibilities without breaking the API:
- We could offer more actions that the consumer can opt into:
type LogEntryAction =
| {
actionType: 'show-details';
}
| {
actionType: 'show-in-context';
}
- We could offer action-specific configuration:
type LogEntryAction =
| {
actionType: 'show-details';
timeout: number;
}
| {
actionType: 'custom';
label: string;
handleAction: () => void;
}
What do you think?
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.
I like it! But I'm not sure about anticipating it now. It makes more sense to me once we have the actual requirement. Besides, it expands a bit the purpose of this PR. Having the requirement for those changes also help me to understand better what it needs to provide.
From what I know, we will still have more work to do on the flyout in the next cycle. We could do these things in the future. wdyt?
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.
Sure, the compiler should allow us to find and fix usages in the future when the API changes 👍 In that case, do we want to name the property so it matches the EUI style more closely? Something like enableFlyoutAction
or showFlyoutAction
?
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.
showFlyoutAction
sounds good. I'll change that.
Thanks for reviewing it @weltenwort!
This is true. I had noticed that too, but I thought that's how it was supposed to be. I personally think it's better to close the flyout. I'll look into it |
…ing to another page
1a33519
to
8b7343c
Compare
8b7343c
to
1a33519
Compare
@elasticmachine merge upstream |
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.
APM changes lgtm.
Awesome improvement!
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
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 polishing it so much 👍
Summary
This PR closes #111325 by enabling LogStream component to show flyouts on APM
Trace
sample logs andLogs
tabScreenshots
Trace Samples
Logs Tab
Notes
Investigate
, which caused the popover to not close when the button is clicked 2x or more. The button will now toggle the visibility when clicked.