-
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 116 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,97 @@ | ||
# 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 except for actions, which | ||
may modify it to cause angular to re-render and watches to trigger. | ||
|
||
**Unit-Testability**: Creating unit tests for large parts of the UI code is | ||
made easy by expressing the as much of the logic as possible as | ||
side-effect-free functions. The only place where side-effects are allowed are | ||
actions. 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 | ||
| |* angular templates render state | ||
| v | ||
| |* angular calls actions in response to user action/system events | ||
| v | ||
| |* actions modify state | ||
| v | ||
+--+ | ||
|
||
``` | ||
|
||
**State**: The state is the single source of truth at | ||
`ContextAppController::state` and may only be modified by actions. | ||
|
||
**Action**: Actions are functions that are called inreponse user or system | ||
actions and may modified the state the are bound to via their closure. | ||
|
||
|
||
## 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
import expect from 'expect.js'; | ||
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[0]).to.have.property('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); | ||
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 don't think we should assert this here, since the previous test is covering it, and it doesn't reflect this test's description. 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 added this to avoid a hard-to-understand error for the following expectation if the spy was not called at all. I'll remove it. |
||
expect(esStub.search.firstCall.args[0].body).to.have.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().fail('expected the promise to be rejected'); | ||
}, | ||
(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,31 @@ | ||
import _ from 'lodash'; | ||
|
||
import { addComputedFields } from './utils/fields'; | ||
import { createAnchorQueryBody } from './utils/queries'; | ||
|
||
|
||
async function fetchAnchor(es, indexPattern, uid, sort) { | ||
const indices = await indexPattern.toIndexList(); | ||
const queryBody = addComputedFields(indexPattern, createAnchorQueryBody(uid, sort)); | ||
const response = await es.search({ | ||
index: indices, | ||
body: queryBody, | ||
}); | ||
|
||
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 document.'); | ||
} | ||
|
||
return Object.assign( | ||
{}, | ||
response.hits.hits[0], | ||
{ | ||
$$_isAnchor: true, | ||
}, | ||
); | ||
} | ||
|
||
|
||
export { | ||
fetchAnchor, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import _ from 'lodash'; | ||
|
||
import { addComputedFields } from './utils/fields'; | ||
import { getDocumentUid } from './utils/ids'; | ||
import { createSuccessorsQueryBody } from './utils/queries.js'; | ||
import { reverseQuerySort } from './utils/sorting'; | ||
|
||
|
||
async function fetchSuccessors(es, indexPattern, anchorDocument, sort, size) { | ||
const successorsQueryBody = prepareQueryBody(indexPattern, anchorDocument, sort, size); | ||
const results = await performQuery(es, indexPattern, successorsQueryBody); | ||
return results; | ||
} | ||
|
||
async function fetchPredecessors(es, indexPattern, anchorDocument, sort, size) { | ||
const successorsQueryBody = prepareQueryBody(indexPattern, anchorDocument, sort, size); | ||
const predecessorsQueryBody = reverseQuerySort(successorsQueryBody); | ||
const reversedResults = await performQuery(es, indexPattern, predecessorsQueryBody); | ||
const results = reversedResults.slice().reverse(); | ||
return results; | ||
} | ||
|
||
|
||
function prepareQueryBody(indexPattern, anchorDocument, sort, size) { | ||
const successorsQueryBody = addComputedFields( | ||
indexPattern, | ||
createSuccessorsQueryBody(anchorDocument.sort, sort, size) | ||
); | ||
return successorsQueryBody; | ||
} | ||
|
||
async function performQuery(es, indexPattern, queryBody) { | ||
const indices = await indexPattern.toIndexList(); | ||
|
||
const response = await es.search({ | ||
index: indices, | ||
body: queryBody, | ||
}); | ||
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'], []); | ||
} | ||
|
||
|
||
export { | ||
fetchPredecessors, | ||
fetchSuccessors, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import expect from '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', | ||
}, | ||
} | ||
}); | ||
|
||
const query = addComputedFields({ getComputedFields }, {}); | ||
expect(query).to.have.property('script_fields'); | ||
expect(query.script_fields).to.eql(getComputedFields().scriptFields); | ||
}); | ||
|
||
it('should add the `docvalue_fields` property defined in the given index pattern', function () { | ||
const getComputedFields = () => ({ | ||
docvalueFields: ['field1'], | ||
}); | ||
|
||
const query = addComputedFields({ getComputedFields }, {}); | ||
expect(query).to.have.property('docvalue_fields'); | ||
expect(query.docvalue_fields).to.eql(getComputedFields().docvalueFields); | ||
}); | ||
|
||
it('should add the `stored_fields` property defined in the given index pattern', function () { | ||
const getComputedFields = () => ({ | ||
storedFields: ['field1'], | ||
}); | ||
|
||
const query = addComputedFields({ getComputedFields }, {}); | ||
expect(query).to.have.property('stored_fields'); | ||
expect(query.stored_fields).to.eql(getComputedFields().storedFields); | ||
}); | ||
|
||
it('should preserve other properties of the query', function () { | ||
const getComputedFields = () => ({}); | ||
|
||
const query = addComputedFields({ getComputedFields }, { property1: 'value1' }); | ||
expect(query).to.have.property('property1', 'value1'); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
import expect from 'expect.js'; | ||
|
||
import { | ||
createAnchorQueryBody, | ||
createSuccessorsQueryBody, | ||
} from 'plugins/kibana/context/api/utils/queries'; | ||
|
||
|
||
describe('context app', function () { | ||
describe('function createAnchorQueryBody', function () { | ||
it('should return a search definition that searches the given uid', function () { | ||
const query = createAnchorQueryBody('UID', { '@timestamp': 'desc' }); | ||
expect(query.query.terms._uid[0]).to.eql('UID'); | ||
}); | ||
|
||
it('should return a search definition that sorts by the given criteria and uid', function () { | ||
const query = createAnchorQueryBody('UID', { '@timestamp': 'desc' }); | ||
expect(query.sort).to.eql([ | ||
{ '@timestamp': 'desc' }, | ||
{ _uid: 'asc' }, | ||
]); | ||
}); | ||
}); | ||
|
||
describe('function createSuccessorsQueryBody', function () { | ||
it('should return a search definition that includes the given size', function () { | ||
const query = createSuccessorsQueryBody([0, 'UID'], { '@timestamp' : 'desc' }, 10); | ||
expect(query).to.have.property('size', 10); | ||
}); | ||
|
||
it('should return a search definition that sorts by the given criteria and uid', function () { | ||
const query = createSuccessorsQueryBody([0, 'UID'], { '@timestamp' : 'desc' }, 10); | ||
expect(query).to.have.property('sort'); | ||
expect(query.sort).to.eql([ | ||
{ '@timestamp': 'desc' }, | ||
{ _uid: 'asc' }, | ||
]); | ||
}); | ||
|
||
it('should return a search definition that searches after the given uid', function () { | ||
const query = createSuccessorsQueryBody([0, 'UID'], { '@timestamp' : 'desc' }, 10); | ||
expect(query).to.have.property('search_after'); | ||
expect(query.search_after).to.eql([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.