[Logs UI] Show highlighted log entries in the minimap #40745
Merged
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
…nto 22916-log-highlighting
…nto 22916-log-highlighting
* Show a highlight is active * Focus input when menu is opened * Icon for highlights menu * Debounce onChange handler * Loading state for highlights menu * Neaten up Provider
* Ensure width of popover stays consistent * Add a button to clear highlights
While a `phrase_prefix` query feels more natural, it doesn't work on `keyword` type fields. Using just a `phrase` query keeps the highlighting consistent between text and keyword type fields.
💚 Build Succeeded |
jasonrhodes
requested review from
jasonrhodes
and removed request for
jasonrhodes
jasonrhodes
reviewed
Aug 1, 2019
| throw new DependencyError('Failed to load source: No apollo client available.'); | ||
| } | ||
| if (!start || !end || !highlightTerms.length) { | ||
| throw new Error(); |
There was a problem hiding this comment.
Should this error have a message? What does this throw do?
There was a problem hiding this comment.
It should have a message, even though it never bubbles up beyond the hook. It's used to short-circuit the loading mechanism. Maybe a return Promise.reject() would be more apppriate. 🤔
jasonrhodes
reviewed
Aug 1, 2019
There was a problem hiding this comment.
Overall this looks good but I noticed a couple of quirks worth mentioning:
- When I first loaded the PR, I searched for a highlight and it all worked except the next/prev buttons were consistently disabled. I added a console.log in the component and the problem went away and stayed away even after I removed the console.log and ever since it's been fine. Probably just some kind of cache quirk or something, but mentioning it in case it comes up again.
- I have a video of me clicking through the mini-map and eventually I get to a supposed section with highlights where there aren't any highlights. Sometimes the highlights don't pop up until after a delay. I'll try to attach the video here shortly...
|
The highlights showing up with a delay is not unexpected. They are loaded asynchronously so the delay depends on the network connection and the cluster load. The alternative would be to block displaying anything until the highlights have been loaded, which is probably an even worse experience. We are showing a spinner inside of the highlight popover to indicate the highlights are still loading. Maybe we should move it into the button so it's visible even when the popover is closed? |
That's a great idea. I don't think it's a blocker for this ticket, but I'd prefer to see that at some point. Up to you all if you want to do it here or separately. As for the other problem I had, if you all can't reproduce it by clicking through highlights in the minimap, I'm ok to move on and keep an eye out. |
jasonrhodes
approved these changes
Aug 6, 2019
There was a problem hiding this comment.
Left a last comment here: #40745 (comment)
But LGTM for merging.
|
I was finally able to reproduce the missing highlights you describe. I'll investigate. |
|
So it turned out that the query returned hits that didn't have any highlights because the filter clauses took into account additional fields beyond those that were used for highlighting. Now the fields should be the same, which means these unhighlighted entries should no longer occur. @jasonrhodes can you confirm that this prevents the situation you ran into? |
💚 Build Succeeded |
weltenwort
changed the title
jasonrhodes
approved these changes
Aug 9, 2019
💚 Build Succeeded |
|
Awesome work @weltenwort 🎉 |
Kerry350
added a commit
to Kerry350/kibana
that referenced
this pull request
Aug 9, 2019
* Commit to allow opening the PR * Add start of the highlights menu * Add terms input * Add a container for highlights state * Start to hook up highlights state effects, and Bridge with Redux * Move highlighted logs to a separate graphql field * Add highlights argument to unit tests * Add initial draft of a log highlights hook * Finish hooking up hook state ready for GraphQL query * Take startKey and endKey from overall collection of log entries * Add sourceVersion * Fetch the data * Use columns ids instead of column indices * merge highlights into stream WIP * Add highlighting for the message column type * Use the serialized filter query * UX / Performance improvements * Show a highlight is active * Focus input when menu is opened * Icon for highlights menu * Debounce onChange handler * Loading state for highlights menu * Neaten up Provider * Tweaks * Breathing room for icon * Better debounce time for typing * Menu changes * Ensure width of popover stays consistent * Add a button to clear highlights * Add highlighting for field columns * Use phrase query for highlighting While a `phrase_prefix` query feels more natural, it doesn't work on `keyword` type fields. Using just a `phrase` query keeps the highlighting consistent between text and keyword type fields. * Factor out commonly used visibility state * Fix debounce to support changing onChange prop * Move clear highlight button inline * Translate labels * Rely on popover ownfocus prop to focus the input * Rename function to avoid i18n checker false positive * Rough and ready WIP next / previous * Add `countBefore` and `countAfter` highlight args * Fix value used * Add a check for entry * Rearrange icons * 0 falsey blurgh * Organise into separate sub-hooks for clarity * Add comment * Use filter and highlight queries also beyond interval * Refine behaviour of next / previous * Stops wrapping * Ensures that when scrolling next / previous takes off where it was left * Ensures the first highlight is scrolled to when changing highlight term * Add log highlights api tests * Fix a few runtime and compiler errors and linter warnings * Refactor prev/next jumping to avoid index errors * Use nearest index for initial jump * Include element type in translation id * Disable clear button when nothing to clear * Initial summary highlights state * Amend component usage * Comment out load call until query exists * Add graphql api for summary highlights * Add the client-side log summary highlight query * Directly use log view configuration without a bridge * Rename log entry highlight hook to maintain symmetry * Re-enable highlight rendering in the minimap * Keep the time cursor from intercepting clicks * tweak minimap highlighting colors and interaction * Clean up after merge * Remove commented-out code * Change cursor to hint at minimap highlighting interaction * Restrict highlight query filter to relevant fields * Add explanatory error message
|
@Kerry350 well done yourself 😉 thanks for the great collaboration |
Kerry350
added a commit
that referenced
this pull request
Aug 9, 2019
* Commit to allow opening the PR * Add start of the highlights menu * Add terms input * Add a container for highlights state * Start to hook up highlights state effects, and Bridge with Redux * Move highlighted logs to a separate graphql field * Add highlights argument to unit tests * Add initial draft of a log highlights hook * Finish hooking up hook state ready for GraphQL query * Take startKey and endKey from overall collection of log entries * Add sourceVersion * Fetch the data * Use columns ids instead of column indices * merge highlights into stream WIP * Add highlighting for the message column type * Use the serialized filter query * UX / Performance improvements * Show a highlight is active * Focus input when menu is opened * Icon for highlights menu * Debounce onChange handler * Loading state for highlights menu * Neaten up Provider * Tweaks * Breathing room for icon * Better debounce time for typing * Menu changes * Ensure width of popover stays consistent * Add a button to clear highlights * Add highlighting for field columns * Use phrase query for highlighting While a `phrase_prefix` query feels more natural, it doesn't work on `keyword` type fields. Using just a `phrase` query keeps the highlighting consistent between text and keyword type fields. * Factor out commonly used visibility state * Fix debounce to support changing onChange prop * Move clear highlight button inline * Translate labels * Rely on popover ownfocus prop to focus the input * Rename function to avoid i18n checker false positive * Rough and ready WIP next / previous * Add `countBefore` and `countAfter` highlight args * Fix value used * Add a check for entry * Rearrange icons * 0 falsey blurgh * Organise into separate sub-hooks for clarity * Add comment * Use filter and highlight queries also beyond interval * Refine behaviour of next / previous * Stops wrapping * Ensures that when scrolling next / previous takes off where it was left * Ensures the first highlight is scrolled to when changing highlight term * Add log highlights api tests * Fix a few runtime and compiler errors and linter warnings * Refactor prev/next jumping to avoid index errors * Use nearest index for initial jump * Include element type in translation id * Disable clear button when nothing to clear * Initial summary highlights state * Amend component usage * Comment out load call until query exists * Add graphql api for summary highlights * Add the client-side log summary highlight query * Directly use log view configuration without a bridge * Rename log entry highlight hook to maintain symmetry * Re-enable highlight rendering in the minimap * Keep the time cursor from intercepting clicks * tweak minimap highlighting colors and interaction * Clean up after merge * Remove commented-out code * Change cursor to hint at minimap highlighting interaction * Restrict highlight query filter to relevant fields * Add explanatory error message
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
The minimap to the right now shows the distribution of highlighted entries within the configured interval. In addition, an on-hover tooltip gives more details and a click jumps to one of the highlights within the corresponding time bucket.
Preview
Task breakdown
logHighlightshookChecklist
Documentation was added for features that require explanation or tutorials