Skip to content
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

Show event context #9198

Merged
merged 118 commits into from
Feb 22, 2017
Merged

Show event context #9198

merged 118 commits into from
Feb 22, 2017

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Nov 23, 2016

This aims to address some of the use cases described in #275.

Feedback is very welcome - please leave a comment below.

Progress

  • create basic table view
  • add controls to increase/decrease the number of surrounding entries
  • make number of surrounding entries editable directly
  • use the index-pattern's time field
  • only show context link for time-based indices
  • make number increment configurable
  • make default number of surrounding entries configurable
  • add loading indicators
  • make both sides of the context independently adjustable
  • add error handling
  • add end-of-data indicators
  • check cross-browser styling issues
  • add integration tests

Previews

The Event Context View

image

Link to the Event Context View from Discover

image

Warning shown when fewer events could be found than requested

image

Error shown when the anchor event could not be loaded

image

The `multi-transclude` attribute directive encapsulates the angular 1.4
workaround for multiple transclusion targets pioneered in `kbn-top-nav`
for separation of concerns and re-use.

There are some differences to the implementation in `kbn-top-nav`:
* The directive logic is completely contained inside the link function
  and therefore won't interfere with other controllers.
* The slots are declared as the attribute value.
* The transcluded items are appended to the transclusion target's
  children instead of replacing the whole target. This preserves the
  attributes present on the target element.
The new directive `<local-navigation>` creates a navigation that uses
the ui-framework block styles of the same name. It utilizes the
previously introduced `multi-transclude` directive to provide the
following transclusion targets:

* `primaryLeft`: The left side of the top row, often used for
  a title or breadcrumbs.
* `primaryRight`: The right side of the top row, often used for menus
  and time pickers.
* `secondary`: The bottom row for search bars or tab navigation.

While `kbn-top-nav` combined the concerns of rendering the surrounding
DOM as well as menu entries and the timepicker, this directive expects
those to be implemented separately and just placed in the navigation bar
via aforementioned transclusion slots.

A slot for dropdown panels has not yet been added.
The doc viewer panel now contains a (hideous) link which displays the
context of the entry using the same column configuration. The location
and styling is hopefully subject to future improvement
The breadcrumbs indicate that the context view is a sub-view of the
normal "Discover" view by including a link to it as the first item.
To focus on the use case of the app and to rule out numerous edge cases,
the context is always interpreted in relation the time field specified
in the index pattern.
The link is hidden for index patterns that do not have a time field
defined, because the entry context view is designed to allow viewing the
surrounding entries with respect to a time ordering.
@lukasolson
Copy link
Member

Here's some initial feedback I have:

Regarding the +/- surrounding entries: Almost everything I can see that shows entries in their context will simply have a "load older entries" button at the top and a "load newer entries" at the bottom. I like this better than simply a +/- because you don't lose your place, and you may only be interested in going one direction, rather than both at the same time.

Regarding "entries"... Shouldn't we use "documents" or "docs" rather than "entries"? I thought that was the verbiage we wanted, but I could be wrong.

I think as a first phase, simply dropping all filters and searches is fine, but I think in the future we may want to allow users to filter and search on this page. I think we should see if it's an often-requested feature before adding it.

I think there was some talk about allowing users to select a time range rather than +/- entries, and once again, this might be a neat feature but I think it is possibly outside 80% of use cases and we should wait to get feedback about whether or not it'd be useful.

Also, I remember hearing that we are ordering by time. Is this desirable, or should we simply be displaying the documents in the order that we receive them from ES? Is it guaranteed that the order we get them back from the query will be the same order they were inserted into ES? Also, perhaps it would be useful to allow sorting, but just doing an in-memory sort rather than setting up the query to sort by the given column. But again, I think this should probably be in a later phase.

Amazing work! This is going to be very, very useful!

@allenmchan
Copy link

This is going to be amazing. Thanks for working on this.

@weltenwort
Copy link
Member Author

@lukasolson Thanks for the thoughtful feedback. Let's dig into some of it:

+/- buttons: Yes, I thought about the buttons at the top and bottom as well, but my first attempt at integrating it with the doc-table (which I just reused from the main discover app) was so ugly that I dropped it in disgust. Placing the navigation in the top left corner has the advantage of consistency with the time-picker location. I could give it another try.

symmetric context size: That is a question of UI simplicity. Technically, there's no problem in splitting it into separate before/after sizes.

wording: That's absolutely up for discussion. As a couchdb user I'm pretty used to calling it "documents", but I'm not sure if that is too technical. The "entries" wording is more reminiscent of "log entries", which is the core use case for this app.

filters: I would be fine with leaving it out for now. I have been prototyping transferring over the discover filters in a disabled state so the user can incrementally re-enable them. But it definitely adds UI complexity. The question is how minimal of a feature set we can get away with for the first release.

sorting: That's an interesting topic. There is an "insertion order" sorting, but it is only consistent within one shard, I think. The sequence numbers that are currently being implemented in ES might help to make that consistent for full indices. But even with those numbers there is no guarantee that the insertion order of the documents actually has a semantic meaning. Buffering and bulk insertion by Logstash, Filebeat or the like could mix it up. On the other hand, the user has already defined a sorting key for time-based index patterns, that has "meaning" in the data, so I went with that for now.

@tbragin, your opinion would be much appreciated once more, especially on the matter of the feature set size.

The new setting `context:defaultSize` allows for customization of the
initial number of rows displayed when viewing an entry's context.
The new setting `context:step` allows for customization of the step size
the context size is incremented/decremented by when using the buttons in
the context view.
The local navigation directive does not require the multi-transclusion
workaround anymore. Instead, it comes with a set of child directives
(`localNavigationRow` and `localNavigationRowSection`) that can be used
to compose a complete local navigation declaratively.
@Bargs
Copy link
Contributor

Bargs commented Dec 7, 2016

Had a chance to play around with this some this evening. First of all, it's awesome! The core functionality already works really well, I'm excited. I jotted down some notes while I was playing around:

  • I'm not seeing the bold styling of the selected row, was that removed?
  • This might be controversial, but I kind of feel like the entries should be sorted with older at the top, newer at the bottom by default. I think the primary use case of this feature is to see the chronological sequence of events and it feels really unnatural to do that reading from bottom to top. Sorting oldest to newest would also better mirror how these events would appear in a log file. Discover is sorted in the opposite direction so one might argue that the switch would be confusing, but I think a context switch happens from "search mode" to "reading mode" when the user clicks that button. At least that's how my brain seemed to work :)
  • Clicking the "Discover" bread crumb seems to lose application state, my filters and selected columns are gone
  • As I play with it, I'm starting to agree with Lukas that it would be nice to be able to add older/newer entries separately. In particular, if I have a lot of entries on screen and I'm scrolled to the bottom of the page and I need to see more, I don't want to have to scroll all the way back up and all the way back down.
  • I think it's beyond the scope of this first version, but I do find myself wanting to modify the columns in the doc table. Maybe we should prioritize making the sidebar a nice reusable component sometime soon.
  • The entire "surrounding entries" container (between the + -) looks like it's clickable
  • If there are no more older/newer entries, it would be nice to display a message to the user indicating such. I found myself clicking the plus icon and watching the entries at the top remain static. I thought something was wrong until I realized I was already at the newest entry.

@weltenwort
Copy link
Member Author

@Bargs, thank you for the very useful feedback! Let me think out loud about your notes...

bold styling: should be there or it's broken - I'll try to restore it. I'm still looking for a better way to highlight the anchor without interfering with the doc table styling

sort order: Yes, I see what you mean. The intention was to stay as closely to the discover table as possible. For reading log data an ascending order would certainly be more intuitive, but whenever I think about it, it seems to cross over into the "log viewing as a specialized app" discussion.

discover state: I'm probably holding AppState wrong, will look into it.

separate older/newer entries: Good point about the scrolling, worth a try

column modification: That would certainly be great, but I did not want to block my progress with that. It also overlaps with the discussion of using newer Elasticsearch APIs to determine the available fields instead of doing it on the client side. That would make it a lot easier to reuse, I guess.

+/- navigation clickable: Lazy as I am, I reused the localNavMenu styles which only know the "disabled" state and the "clickable" state. When I have settled on a navigation style, I will fine-tune that.

indicating "the end": Good suggestion! I'm not sure how to make it look good in combination with the doc table, but I'll give it a try.

The internal context app state is now managed using a unidirectional
data flow. On the ui side, controlling the size of the context now
happens via buttons at the top and bottom of the doc table or via text
inputs in the local navigation bar. Both now allow for independent
manipulation of the number of predecessors and successors. The two
halves of the table are also loaded independently and have independent
loading status indicators.
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion. What do you think?

<span class="kuiStatusText__icon kuiIcon fa-bolt"></span>
<span ng-bind-template="Only {{ contextApp.state.rows.predecessors.length }} entries newer than the anchor could be found."></span>
</span>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of putting the button first, and changing its text so it's clear that it's an incremental action?

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 it definitely looks less "bumpy"

ng-href="{{ getContextAppHref() }}"
ng-if="indexPattern.hasTimeField()"
>
View surrounding entries
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also change all references to "entries" to instead refer to "documents"? I think this is more consistent with the language Kibana uses elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True... I guess I was too caught up in the "log entry" terminology

To display the loading step size on the two loading buttons, the default
step size has been moved into the state.
From chrome 55 on the background color of the anchor row was not
rendered correctly. Applying the color to the cells instead does not
exhibit that problem.
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI and functionality looks good to me. Great work @weltenwort !

@epixa
Copy link
Contributor

epixa commented Feb 16, 2017

Can we pull the WIP prefix off the PR now? :)

@weltenwort weltenwort changed the title WIP: Show event context Show event context Feb 16, 2017
Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple tiny comments

});
});

it('should ignore additional new values', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description isn't very clear to me. How does "should ignore invalid properties" sound to you?


describe('context app', function () {
describe('action setQueryParameters', function () {
it('should update the queryParameters with certain values from the given object', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to below, "certain values" doesn't really mean anything to me. Would something like "should update the queryParameters with valid properties from the given object" sound ok to you?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, that was sloppy wording 😇 "valid properties" sounds a lot better

} from './constants';


export function QueryParameterActionsProvider() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really need to be a "Provider" anymore does it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, but I left it as a provider to maintain consistency with the query actions. The pattern was "actions are created via a provider (to inject required angular dependencies)". It could become "actions are created via a provider if they have angular dependencies" instead. I'm not partial to either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I don't feel too strongly either way, it's probably fine to leave as is.

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@weltenwort weltenwort merged commit 85facdd into elastic:master Feb 22, 2017
elastic-jasper added a commit that referenced this pull request Feb 22, 2017
Backports PR #9198

**Commit 1:**
Add app to display discover entry context

* Original sha: f80a169
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-18T12:50:29Z

**Commit 2:**
Extract multi-transclude workaround into directive

The `multi-transclude` attribute directive encapsulates the angular 1.4
workaround for multiple transclusion targets pioneered in `kbn-top-nav`
for separation of concerns and re-use.

There are some differences to the implementation in `kbn-top-nav`:
* The directive logic is completely contained inside the link function
  and therefore won't interfere with other controllers.
* The slots are declared as the attribute value.
* The transcluded items are appended to the transclusion target's
  children instead of replacing the whole target. This preserves the
  attributes present on the target element.

* Original sha: 05aedd8
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-21T14:56:20Z

**Commit 3:**
Add a generic local-navigation directive

The new directive `<local-navigation>` creates a navigation that uses
the ui-framework block styles of the same name. It utilizes the
previously introduced `multi-transclude` directive to provide the
following transclusion targets:

* `primaryLeft`: The left side of the top row, often used for
  a title or breadcrumbs.
* `primaryRight`: The right side of the top row, often used for menus
  and time pickers.
* `secondary`: The bottom row for search bars or tab navigation.

While `kbn-top-nav` combined the concerns of rendering the surrounding
DOM as well as menu entries and the timepicker, this directive expects
those to be implemented separately and just placed in the navigation bar
via aforementioned transclusion slots.

A slot for dropdown panels has not yet been added.

* Original sha: 2e422f7
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-21T15:04:50Z

**Commit 4:**
Use `<local-navigation>` for context size controls

* Original sha: 89b31e9
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-21T15:07:48Z

**Commit 5:**
Wire up the context app state to the url

* Original sha: 3227016
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-21T15:09:34Z

**Commit 6:**
Add basic link from discover to the context app

The doc viewer panel now contains a (hideous) link which displays the
context of the entry using the same column configuration. The location
and styling is hopefully subject to future improvement

* Original sha: ef78a3c
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-21T15:12:38Z

**Commit 7:**
Change context view breadcrumbs

The breadcrumbs indicate that the context view is a sub-view of the
normal "Discover" view by including a link to it as the first item.

* Original sha: be355d8
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-22T19:14:30Z

**Commit 8:**
Return promises from the reload and init actions

* Original sha: b2db703
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-22T19:16:53Z

**Commit 9:**
Make the context size display editable

* Original sha: c3d7459
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-22T19:19:11Z

**Commit 10:**
Always sort on the index-pattern's time field

To focus on the use case of the app and to rule out numerous edge cases,
the context is always interpreted in relation the time field specified
in the index pattern.

* Original sha: 9e5986c
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-23T13:44:34Z

**Commit 11:**
Improve doc and context link styling in docTable

* Original sha: d64f938
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-23T14:54:36Z

**Commit 12:**
Fix font-awesome class name typo

* Original sha: 2acc123
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-23T14:58:08Z

**Commit 13:**
Hide context link for non-time-based index patterns

The link is hidden for index patterns that do not have a time field
defined, because the entry context view is designed to allow viewing the
surrounding entries with respect to a time ordering.

* Original sha: 4991bd8
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-11-23T15:11:22Z

**Commit 14:**
Merge branch 'master' into discover-context-app

* Original sha: 1bb189d
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-05T12:39:18Z

**Commit 15:**
Add setting to configure the default context size

The new setting `context:defaultSize` allows for customization of the
initial number of rows displayed when viewing an entry's context.

* Original sha: e10f458
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-05T14:57:49Z

**Commit 16:**
Add setting to configure the context size step

The new setting `context:step` allows for customization of the step size
the context size is incremented/decremented by when using the buttons in
the context view.

* Original sha: 123d6b0
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-05T15:01:06Z

**Commit 17:**
Enforce a minimal context size of 0

* Original sha: dbaed4f
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-05T15:04:48Z

**Commit 18:**
Reimplement the local nav without multi-transclusion

The local navigation directive does not require the multi-transclusion
workaround anymore. Instead, it comes with a set of child directives
(`localNavigationRow` and `localNavigationRowSection`) that can be used
to compose a complete local navigation declaratively.

* Original sha: 840278f
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-06T15:21:51Z

**Commit 19:**
Add visual indicator for first initialization

* Original sha: 4730c77
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-06T16:35:37Z

**Commit 20:**
Merge commit '10f7880' into discover-context-app

* Original sha: 2054287
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-13T12:04:44Z

**Commit 21:**
Improve loading ui and behaviour, refactor state

The internal context app state is now managed using a unidirectional
data flow. On the ui side, controlling the size of the context now
happens via buttons at the top and bottom of the doc table or via text
inputs in the local navigation bar. Both now allow for independent
manipulation of the number of predecessors and successors. The two
halves of the table are also loaded independently and have independent
loading status indicators.

* Original sha: a9c96aa
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-13T18:55:58Z

**Commit 22:**
Merge branch 'master' into discover-context-app

* Original sha: a2b1456
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-13T18:59:57Z

**Commit 23:**
Remove plus icons from "load more" buttons

* Original sha: bb1b056
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-13T19:16:34Z

**Commit 24:**
Merge branch 'master' into discover-context-app

* Original sha: 36c9b17
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-13T21:47:16Z

**Commit 25:**
Merge branch 'master' into discover-context-app

* Original sha: 72e7faf
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-14T11:49:07Z

**Commit 26:**
Fix AppState synchronization

* Original sha: 366962a
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-15T14:54:59Z

**Commit 27:**
Merge branch 'master' into discover-context-app

* Original sha: a98d967
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-15T14:57:32Z

**Commit 28:**
Fix linting errors

* Original sha: 778d3ce
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-15T15:00:33Z

**Commit 29:**
Fix more linting errors

* Original sha: 46d8472
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-15T15:10:11Z

**Commit 30:**
Make anchor highlighting more prominent

* Original sha: 3ae8a5d
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2016-12-16T12:41:47Z
@epixa epixa added the v5.4.0 label Feb 22, 2017
weltenwort pushed a commit that referenced this pull request Feb 22, 2017
@tbragin
Copy link
Contributor

tbragin commented May 1, 2017

@jimgoodwin Blurb and screenshots for 5.4 blog:

Event Context most commonly comes up in log troubleshooting workflows. The idea is that once you’ve filtered down on a number of events (e.g. errors), you want to see some number of events around each of those (e.g. to see what happened before the error). The initial feature in 5.4 allows user to see global context, and we are working on additional ability to further filter this context down (e.g. to logs coming from a specific server) in a future PR: #11466

lec1

lec2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet