-
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
[Logs UI] Use index name for queries for log entry details fly-out #67004
[Logs UI] Use index name for queries for log entry details fly-out #67004
Conversation
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
@driskell would you mind checking if I represented your contributions correctly? |
@elasticmachine merge upstream |
💚 Build SucceededHistory
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.
Code looks nice and works correctly 👍
For a separate PR: I don't know what's our approach to error handling. For example if I try to pass an ID or an index that doesn't exist the API endpoint crashes instead of returning a 404 (I did the API, so this one is on me 😅)..
Since we are the only consumers of this API maybe we don't care, but I wanna comment on it nonetheless
flyoutVisibility?: string | null; | ||
surroundingLogsId?: string | null; | ||
} | ||
|
||
export const useLogFlyout = () => { | ||
const { sourceId } = useLogSourceContext(); | ||
const [flyoutVisible, setFlyoutVisibility] = useState<boolean>(false); | ||
const [flyoutId, setFlyoutId] = useState<string | null>(null); | ||
const [flyoutRef, setFlyoutRef] = useState<{ id: string; index: string } | null>(null); |
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 changing the *Ref
suffix? That way we can reserve it to React refs.
Maybe something more specific, like flyoutLogItem
, or flyoutLogLine
Sorry for letting this sit for so long. The async search migration has changed a lot of things underneath which will allow me to solve this in a much simpler way. |
ℹ️ This cleanly applies the relevant changes of #57761 to the current state of
master
.Summary
This utilize the index name in the log entry detail fly-out to prevent scanning of all available indices on
getLogItem
, which can bring down small clusters.closes #38240