-
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
Show event context #9198
Show event context #9198
Changes from 65 commits
f80a169
05aedd8
2e422f7
89b31e9
3227016
ef78a3c
be355d8
b2db703
c3d7459
9e5986c
d64f938
2acc123
4991bd8
1bb189d
e10f458
123d6b0
dbaed4f
840278f
4730c77
2054287
a9c96aa
a2b1456
bb1b056
36c9b17
72e7faf
366962a
a98d967
778d3ce
46d8472
3ae8a5d
ddda2ff
84d0a3d
491e212
89f3af2
170b6e6
cfd0573
3ed6941
d15b35c
c2d2240
ed354e0
62f7342
5472730
b7a789d
34abd9c
e9cdb7a
00d1bbe
d3a37cb
d4daca4
6ac96e2
2cbbf62
ce2d015
a7d32ac
fbb61c1
82f88c4
2f31979
5cc91aa
b80be8b
02440e8
cb4d95f
2e5b192
5c50c1d
43472f5
5272a76
2756db1
de6983f
cbe3602
29e1fa8
19df62f
9e4dcea
e0046e2
1b7fb54
cdae37c
9d411ea
826910e
0566459
4b275b9
61d7683
c0021f2
a916f49
f07926b
05cc080
ef3573e
183991e
8997be7
61f0423
6af8043
d8a7a4f
a0040cc
d23f2e4
e8862c7
0102c18
9ef0b69
9af7cb6
f8dcd35
41ea1bf
0a4fa98
a3572d0
ce4c07f
938e841
06245b9
44c1e8c
519a3a4
3856851
61be5c1
912df52
21c9098
c6ac75b
9706068
9b8bf37
8998279
ad1c856
d8f99e7
8a5b60e
50205b8
cb2bae6
12ce61b
2784b0c
62bf542
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
# Discover Context App Implementation Notes | ||
|
||
The implementation of this app is intended to exhibit certain desirable | ||
properties by adhering to a set of *principles*. This document aims to explain | ||
those and the *concepts* employed to achieve that. | ||
|
||
|
||
## Principles | ||
|
||
**Single Source of Truth**: A good user experience depends on the UI displaying | ||
consistent information across the whole page. To achieve this, there should | ||
always be a single source of truth for the application's state. In this | ||
application this is the `ContextAppController::state` object. | ||
|
||
**Unidirectional Data Flow**: While a single state promotes rendering | ||
consistency, it does little to make the state changes easier to reason about. | ||
To avoid having state mutations scattered all over the code, this app | ||
implements a unidirectional data flow architecture. That means that the state | ||
is treated as immutable throughout the application and a new state may only be | ||
derived via the root reducer (see below). | ||
|
||
**Unit-Testability**: Creating unit tests for large parts of the UI code is | ||
made easy by expressing the state management logic mostly as side-effect-free | ||
functions. The only place where side-effects are allowed are action creators | ||
and the reducer middlewares. Due to the nature of AngularJS a certain amount of | ||
impure code must be employed in some cases, e.g. when dealing with the isolate | ||
scope bindings in `ContextAppController`. | ||
|
||
**Loose Coupling**: An attempt was made to couple the parts that make up this | ||
app as loosely as possible. This means using pure functions whenever possible | ||
and isolating the angular directives diligently. To that end, the app has been | ||
implemented as the independent `ContextApp` directive in [app.js](./app.js). It | ||
does not access the Kibana `AppState` directly but communicates only via its | ||
directive properties. The binding of these attributes to the state and thereby | ||
to the route is performed by the `CreateAppRouteController`in | ||
[index.js](./index.js). Similarly, the `SizePicker` directive only communicates | ||
with its parent via the passed properties. | ||
|
||
|
||
## Concepts | ||
|
||
To adhere to the principles mentioned above, this app borrows some concepts | ||
from the redux architecture that forms a ciruclar unidirectional data flow: | ||
|
||
``` | ||
|
||
|* create initial state | ||
v | ||
+->+ | ||
| v | ||
| |* state | ||
| v | ||
| |* selectors calculate derived (memoized) values | ||
| v | ||
| |* angular templates render values | ||
| v | ||
| |* angular dispatches actions in response to user action/system events | ||
| v | ||
| |* middleware processes the action | ||
| v | ||
| |* reducer derives new state from action | ||
| v | ||
+--+ | ||
|
||
``` | ||
|
||
**State**: The state is the single source of truth at | ||
`ContextAppController::state` and may only be replaced by a new state create | ||
via the root reducer. | ||
|
||
**Reducer**: The reducer at `ContextAppController.reducer` can derive a new | ||
state from the previous state and an action. It must be a pure function and can | ||
be composed from sub-reducers using various utilities like | ||
`createReducerPipeline`, that passes the action and the previous sub-reducer's | ||
result to each sub-reducer in order. The reducer is only called by the dispatch | ||
function located at `ContextAppController::dispatch`. | ||
|
||
**Action**: Actions are objets that describe user or system actions in a | ||
declarative manner. Each action is supposed to be passed to | ||
`ContextAppController::dispatch`, that passes them to each of its middlewares | ||
in turn before calling the root reducer with the final action. Usually, actions | ||
are created using helper functions called action creators to ensure they adhere | ||
to the [Flux Standard Action] schema. | ||
|
||
[Flux Standard Action]: https://github.com/acdlite/flux-standard-action | ||
|
||
**Selector**: To decouple the view from the specific state structure, selectors | ||
encapsulate the knowledge about how to retrieve a particular set of values from | ||
the state in a single location. Additionally, selectors can encapsulate the | ||
logic for calculating derived properties from the state, which should be kept | ||
as flat as possible and free of duplicate information. The performance penalty | ||
for performing these calculations at query time is commonly circumvented by | ||
memoizing the selector results as is the case with `createSelector` from | ||
[redux_lite/selector_helpers.js](./redux_lite/selector_helpers.js). | ||
|
||
|
||
## Directory Structure | ||
|
||
**index.js**: Defines the route and renders the `<context-app>` directive, | ||
binding it to the `AppState`. | ||
|
||
**app.js**: Defines the `<context-app>` directive, that is at the root of the | ||
application. Creates the store, reducer and bound actions/selectors. | ||
|
||
**query.js**: Exports the actions, reducers and selectors related to the | ||
query status and results. | ||
|
||
**query_parameters.js**: Exports the actions, reducers and selectors related to | ||
the parameters used to construct the query. | ||
|
||
**components/loading_button**: Defines the `<context-loading-button>` | ||
directive including its respective styles. | ||
|
||
**components/size_picker**: Defines the `<context-size-picker>` | ||
directive including its respective styles. | ||
|
||
**api/anchor.js**: Exports `fetchAnchor()` that creates and executes the | ||
query for the anchor document. | ||
|
||
**api/context.js**: Exports `fetchPredecessors()` and `fetchSuccessors()` that | ||
create and execute the queries for the preceeding and succeeding documents. | ||
|
||
**api/utils**: Exports various functions used to create and transform | ||
queries. | ||
|
||
**redux_lite**: Exports various functions to create the dispatcher, action | ||
creators, reducers and selectors. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
import { expect } from 'chai'; | ||
import sinon from 'sinon'; | ||
|
||
import { fetchAnchor } from 'plugins/kibana/context/api/anchor'; | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI the AirBnB style guide advises against multiple consecutive blank lines (I can't find the rule but they note it in an issue comment: airbnb/javascript#1048 (comment)). I don't mind if you leave the whitespace the way it is now because the linter can fix it on it's own, but I just wanted to let you know. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been using the following rules:
This results in:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation. |
||
describe('context app', function () { | ||
describe('function fetchAnchor', function () { | ||
it('should use the `search` api to query the given index', function () { | ||
const indexPatternStub = createIndexPatternStub('index1'); | ||
const esStub = createEsStub(['hit1']); | ||
|
||
return fetchAnchor(esStub, indexPatternStub, 'UID', { '@timestamp': 'desc' }) | ||
.then(() => { | ||
expect(esStub.search.calledOnce).to.be.true; | ||
expect(esStub.search.firstCall.args) | ||
.to.have.lengthOf(1) | ||
.with.deep.property('[0].index', 'index1'); | ||
}); | ||
}); | ||
|
||
it('should include computed fields in the query', function () { | ||
const indexPatternStub = createIndexPatternStub('index1'); | ||
const esStub = createEsStub(['hit1']); | ||
|
||
return fetchAnchor(esStub, indexPatternStub, 'UID', { '@timestamp': 'desc' }) | ||
.then(() => { | ||
expect(esStub.search.calledOnce).to.be.true; | ||
expect(esStub.search.firstCall.args) | ||
.to.have.lengthOf(1) | ||
.with.deep.property('[0].body') | ||
.that.contains.all.keys(['script_fields', 'docvalue_fields', 'stored_fields']); | ||
}); | ||
}); | ||
|
||
it('should reject with an error when no hits were found', function () { | ||
const indexPatternStub = createIndexPatternStub('index1'); | ||
const esStub = createEsStub([]); | ||
|
||
return fetchAnchor(esStub, indexPatternStub, 'UID', { '@timestamp': 'desc' }) | ||
.then( | ||
() => { | ||
expect(true, 'expected the promise to be rejected').to.be.false; | ||
}, | ||
(error) => { | ||
expect(error).to.be.an('error'); | ||
} | ||
); | ||
}); | ||
|
||
it('should return the first hit after adding an anchor marker', function () { | ||
const indexPatternStub = createIndexPatternStub('index1'); | ||
const esStub = createEsStub([{ property1: 'value1' }, {}]); | ||
|
||
return fetchAnchor(esStub, indexPatternStub, 'UID', { '@timestamp': 'desc' }) | ||
.then((anchorDocument) => { | ||
expect(anchorDocument).to.have.property('property1', 'value1'); | ||
expect(anchorDocument).to.have.property('$$_isAnchor', true); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
|
||
function createIndexPatternStub(indices) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move these helpers to the top of the file? I find that structure to be more readable, and AirBnB's rules will cause the linter to complain because of no-use-before-define. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm used to applying the pattern suggested in Robert C. Martin's "Clean Code" in regards to ordering: It should be easy to read by starting with the highlights and getting more detailed the further the reader progresses through the file. |
||
return { | ||
getComputedFields: sinon.stub() | ||
.returns({}), | ||
toIndexList: sinon.stub() | ||
.returns(indices), | ||
}; | ||
} | ||
|
||
function createEsStub(hits) { | ||
return { | ||
search: sinon.stub() | ||
.returns({ | ||
hits: { | ||
hits, | ||
total: hits.length, | ||
}, | ||
}), | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import _ from 'lodash'; | ||
|
||
import { addComputedFields } from './utils/fields'; | ||
import { createAnchorQuery } from './utils/queries'; | ||
|
||
|
||
async function fetchAnchor(es, indexPattern, uid, sort) { | ||
const indices = await indexPattern.toIndexList(); | ||
const response = await es.search({ | ||
index: indices, | ||
body: addComputedFields(indexPattern, createAnchorQuery(uid, sort)), | ||
}); | ||
|
||
if (_.get(response, ['hits', 'total'], 0) < 1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the ES client returns an error? We should handle that scenario gracefully. I usually display a user friendly error message with the notifier and log the actual error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but I will handle that on a level closer to the UI. This function is not concerned with user interaction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would at least generally translate the ES errors into something more generic at the service level, so that clients of the service don't end up coupling themselves to the ES client. |
||
throw new Error('Failed to load anchor row.'); | ||
} | ||
|
||
return { | ||
...response.hits.hits[0], | ||
$$_isAnchor: true, | ||
}; | ||
} | ||
|
||
|
||
export { | ||
fetchAnchor, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
import _ from 'lodash'; | ||
|
||
import { addComputedFields } from './utils/fields'; | ||
import { getDocumentUid } from './utils/ids'; | ||
import { createSuccessorsQuery } from './utils/queries.js'; | ||
import { reverseQuerySort } from './utils/sorting'; | ||
|
||
|
||
async function fetchSuccessors(es, indexPattern, anchorDocument, sort, size) { | ||
const successorsQuery = prepareQuery(indexPattern, anchorDocument, sort, size); | ||
const results = await performQuery(es, indexPattern, successorsQuery); | ||
return results; | ||
} | ||
|
||
async function fetchPredecessors(es, indexPattern, anchorDocument, sort, size) { | ||
const successorsQuery = prepareQuery(indexPattern, anchorDocument, sort, size); | ||
const predecessorsQuery = reverseQuerySort(successorsQuery); | ||
const reversedResults = await performQuery(es, indexPattern, predecessorsQuery); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point there is no knowledge about how it is sorted, just that it is reversed from what it was before. I'll try to think of a name that makes that more obvious. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The sequence of operations is actually quite simple:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, sounds good! I think this makes more sense to me now and we can ignore my original comment. Thanks for the explanation. |
||
const results = reverseResults(reversedResults); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's either just call |
||
return results; | ||
} | ||
|
||
|
||
function prepareQuery(indexPattern, anchorDocument, sort, size) { | ||
const anchorUid = getDocumentUid(anchorDocument._type, anchorDocument._id); | ||
const successorsQuery = addComputedFields( | ||
indexPattern, | ||
createSuccessorsQuery(anchorUid, anchorDocument.sort, sort, size) | ||
); | ||
return successorsQuery; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left a comment about renaming const successorsQueryBody = createQueryBody(
indexPattern,
createSuccessorsQuery(anchorDocument.sort, sort, size)
); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think we can make this code more readable by naming the function to reflect the returned value more clearly, and by breaking out arguments into named variables: function createSuccessorsQueryBody(indexPattern, anchorDocument, sort, size) {
const rawQueryBody = createSuccessorsQuery(anchorDocument.sort, sort, size);
const successorsQueryBody = createQueryBody(indexPattern, rawQueryBody);
return successorsQueryBody;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After some more thought, I think that most of my comments about renaming and organization don't need to block this PR and can be addressed in a later refactoring PR, after I've had some time to put together a more comprehensive proposal. 😄 |
||
} | ||
|
||
async function performQuery(es, indexPattern, query) { | ||
const indices = await indexPattern.toIndexList(); | ||
|
||
const response = await es.search({ | ||
index: indices, | ||
body: query, | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as in |
||
|
||
return _.get(response, ['hits', 'hits'], []); | ||
} | ||
|
||
function reverseResults(results) { | ||
results.reverse(); | ||
return results; | ||
} | ||
|
||
|
||
export { | ||
fetchPredecessors, | ||
fetchSuccessors, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
import { expect } from 'chai'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you mind switching from Chai to expect.js? The vast majority of our tests are written with expect.js. It looks like Chai was pulled in with Timelion which was originally an external plugin. We have an open issue about moving to Chai but until then it would be better to stick to expect.js. |
||
|
||
import { addComputedFields } from 'plugins/kibana/context/api/utils/fields'; | ||
|
||
|
||
describe('context app', function () { | ||
describe('function addComputedFields', function () { | ||
it('should add the `script_fields` property defined in the given index pattern', function () { | ||
const getComputedFields = () => ({ | ||
scriptFields: { | ||
sourcefield1: { | ||
script: '_source.field1', | ||
}, | ||
} | ||
}); | ||
|
||
expect(addComputedFields({ getComputedFields }, {})).to.have.property('script_fields') | ||
.that.is.deep.equal(getComputedFields().scriptFields); | ||
}); | ||
|
||
it('should add the `docvalue_fields` property defined in the given index pattern', function () { | ||
const getComputedFields = () => ({ | ||
docvalueFields: ['field1'], | ||
}); | ||
|
||
expect(addComputedFields({ getComputedFields }, {})).to.have.property('docvalue_fields') | ||
.that.is.deep.equal(getComputedFields().docvalueFields); | ||
}); | ||
|
||
it('should add the `stored_fields` property defined in the given index pattern', function () { | ||
const getComputedFields = () => ({ | ||
storedFields: ['field1'], | ||
}); | ||
|
||
expect(addComputedFields({ getComputedFields }, {})).to.have.property('stored_fields') | ||
.that.is.deep.equal(getComputedFields().storedFields); | ||
}); | ||
|
||
it('should preserve other properties of the query', function () { | ||
const getComputedFields = () => ({}); | ||
|
||
expect(addComputedFields({ getComputedFields }, { property1: 'value1' })) | ||
.to.have.property('property1', 'value1'); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { expect } from 'chai'; | ||
|
||
import { | ||
createAnchorQuery, | ||
createSuccessorsQuery, | ||
} from 'plugins/kibana/context/api/utils/queries'; | ||
|
||
|
||
describe('context app', function () { | ||
describe('function createAnchorQuery', function () { | ||
it('should return a search definition that searches the given uid', function () { | ||
expect(createAnchorQuery('UID', { '@timestamp': 'desc' })) | ||
.to.have.deep.property('query.terms._uid[0]', 'UID'); | ||
}); | ||
|
||
it('should return a search definition that sorts by the given criteria', function () { | ||
expect(createAnchorQuery('UID', { '@timestamp': 'desc' })) | ||
.to.have.deep.property('sort[0]') | ||
.that.is.deep.equal({ '@timestamp': 'desc' }); | ||
}); | ||
}); | ||
|
||
describe('function createSuccessorsQuery', function () { | ||
it('should return a search definition that includes the given size', function () { | ||
expect(createSuccessorsQuery('UID', [0], { '@timestamp' : 'desc' }, 10)) | ||
.to.have.property('size', 10); | ||
}); | ||
|
||
it('should return a search definition that sorts by the given criteria and uid', function () { | ||
expect(createSuccessorsQuery('UID', [0], { '@timestamp' : 'desc' }, 10)) | ||
.to.have.property('sort') | ||
.that.is.deep.equal([ | ||
{ '@timestamp': 'desc' }, | ||
{ _uid: 'asc' }, | ||
]); | ||
}); | ||
|
||
it('should return a search definition that search after the given uid', function () { | ||
expect(createSuccessorsQuery('UID', [0], { '@timestamp' : 'desc' }, 10)) | ||
.to.have.property('search_after') | ||
.that.is.deep.equal([0, 'UID']); | ||
}); | ||
}); | ||
}); |
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 file needs to be updated to reflect the current principles and directory structure.
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 I will remove it, since most of what it describes is no longer true.