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.

Sign up for GitHub

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

Jump to bottom

Add rollupSearchStrategy #20505

Merged
merged 4 commits into from
Jul 23, 2018
Merged

Add rollupSearchStrategy #20505

merged 4 commits into from
Jul 23, 2018

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jul 6, 2018 •
edited
Loading

Partially addresses #20004:

  • Create API endpoint for executing a rollups search
  • Add rollupSearchStrategy for hitting this endpoint
  • Create an adapter that wraps a rollup search but presents the same interface as an esPromise (the promise-like object returned by the elasticsearch-js lib).
  • Create an adapter that resolves this promise with a result object that has the same shape as that returned by a resolving esPromise.
  • Reformat body argument into correct payload shape
  • Validate and/or set correct intervals and timezone
  • Blocked by Delegate surfacing of Courier errors to the consumer #21056
  • Surface error callout in the sidebar
  • Batch together requests to hit a bulk rollup search endpoint
  • Implement failedSearchRequests in rollupSearchStrategy

image

Errors

All Visualize errors (including rollup errors) are now displayed as toasts. Rollup errors will also be surfaced in the sidebar, where they'll persist until the next request.

image

image

image

@cjcenizal cjcenizal added WIP Work in progress :Discovery labels Jul 6, 2018
@cjcenizal cjcenizal mentioned this pull request Jul 6, 2018
Closed
4 tasks
@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal cjcenizal force-pushed the rollup-search-strategy branch from 385cd50 to 3d979e6 Compare July 6, 2018 20:44
@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal cjcenizal mentioned this pull request Jul 6, 2018
Merged
@cjcenizal cjcenizal mentioned this pull request Jul 13, 2018
Closed
52 tasks
@cjcenizal cjcenizal force-pushed the rollup-search-strategy branch from 3d979e6 to ba91cdf Compare July 19, 2018 03:32
@cjcenizal
Add rollupSearchStrategy.
de444d5 
- Add logic to rollupSearchStrategy for extracting criteria for the rollup search.
@cjcenizal cjcenizal force-pushed the rollup-search-strategy branch from ba91cdf to 0587c66 Compare July 19, 2018 03:35
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal cjcenizal force-pushed the rollup-search-strategy branch 2 times, most recently from 74d5360 to f17a051 Compare July 20, 2018 23:45
@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal cjcenizal force-pushed the rollup-search-strategy branch from f17a051 to 7ae39d9 Compare July 23, 2018 21:47
@cjcenizal
Add SearchError for surfacing courier search errors.
43f59dd 
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors..
@cjcenizal cjcenizal force-pushed the rollup-search-strategy branch from 7ae39d9 to 43f59dd Compare July 23, 2018 22:26
@cjcenizal cjcenizal removed the WIP Work in progress label Jul 23, 2018
@cjcenizal cjcenizal changed the title [WIP] Add rollupSearchStrategy Add rollupSearchStrategy Jul 23, 2018
jen-huang
jen-huang approved these changes Jul 23, 2018
View reviewed changes
Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Code LGTM! So excited for this. Great work!

src/ui/public/courier/search_strategy/default_search_strategy.js
@@ -71,14 +72,30 @@ export const defaultSearchStrategy = {
const searching = es.msearch({ body: serializedFetchParams });

return {
// Unwrap the responses object returned by the es client.
searching: searching.then(({ responses }) => responses),
// Munge data into shape expected by consumer.
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL 'munge'

src/ui/public/vis/editors/default/sidebar.js
$scope.$watch('vis.requestError', (requestError) => {
if (requestError && requestError.messsage) {
const { message } = requestError;
const isRollupError = message.includes('rollup');
Copy link
Contributor

Choose a reason for hiding this comment

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

when we fix this up on the next pass, let's remove rollup reference too! 🙂

x-pack/plugins/rollup/public/search/rollup_search_strategy.js
}
}

// TODO: Temporarily automatically assign same timezone and interval as what's defined by
Copy link
Contributor

Choose a reason for hiding this comment

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

this will be handled by EditorConfig #20519
(just linking for prosperity!)

@cjcenizal cjcenizal merged this pull request into elastic:feature/rollups Jul 23, 2018
@cjcenizal cjcenizal deleted the rollup-search-strategy branch July 23, 2018 23:01
@elasticmachine
Copy link
Contributor

💔 Build Failed

jen-huang pushed a commit that referenced this pull request Jul 24, 2018
@cjcenizal @jen-huang
Add rollupSearchStrategy and SearchError (#20505)
189d35c 
* Add /api/rollup/search endpoint and rollupSearchStrategy.
* Add SearchError for surfacing courier search errors.
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors.
jen-huang pushed a commit that referenced this pull request Jul 31, 2018
@cjcenizal @jen-huang
Add rollupSearchStrategy and SearchError (#20505)
cf2b43e 
* Add /api/rollup/search endpoint and rollupSearchStrategy.
* Add SearchError for surfacing courier search errors.
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors.
jen-huang pushed a commit that referenced this pull request Aug 10, 2018
@cjcenizal @jen-huang
Add rollupSearchStrategy and SearchError (#20505)
a81c26b 
* Add /api/rollup/search endpoint and rollupSearchStrategy.
* Add SearchError for surfacing courier search errors.
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors.
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Aug 24, 2018
@cjcenizal
Add rollupSearchStrategy and SearchError (elastic#20505)
3fb74a5 
* Add /api/rollup/search endpoint and rollupSearchStrategy.
* Add SearchError for surfacing courier search errors.
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors.
cjcenizal added a commit that referenced this pull request Sep 4, 2018
@cjcenizal
Add rollupSearchStrategy and SearchError (#20505)
3090d57 
* Add /api/rollup/search endpoint and rollupSearchStrategy.
* Add SearchError for surfacing courier search errors.
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors.
jen-huang pushed a commit that referenced this pull request Sep 7, 2018
@cjcenizal @jen-huang
Add rollupSearchStrategy and SearchError (#20505)
df42fe7 
* Add /api/rollup/search endpoint and rollupSearchStrategy.
* Add SearchError for surfacing courier search errors.
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors.
cjcenizal added a commit that referenced this pull request Sep 19, 2018
@cjcenizal
Add rollupSearchStrategy and SearchError (#20505)
534d3f6 
* Add /api/rollup/search endpoint and rollupSearchStrategy.
* Add SearchError for surfacing courier search errors.
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors.
cjcenizal added a commit that referenced this pull request Sep 28, 2018
@cjcenizal
Add rollupSearchStrategy and SearchError (#20505)
897c212 
* Add /api/rollup/search endpoint and rollupSearchStrategy.
* Add SearchError for surfacing courier search errors.
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors.
cjcenizal added a commit that referenced this pull request Oct 2, 2018
@cjcenizal
Add rollupSearchStrategy and SearchError (#20505)
016a2e1 
* Add /api/rollup/search endpoint and rollupSearchStrategy.
* Add SearchError for surfacing courier search errors.
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors.
@cjcenizal cjcenizal mentioned this pull request Oct 3, 2018
Merged
cjcenizal added a commit that referenced this pull request Oct 4, 2018
@cjcenizal
Add rollupSearchStrategy and SearchError (#20505)
427f7e1 
* Add /api/rollup/search endpoint and rollupSearchStrategy.
* Add SearchError for surfacing courier search errors.
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors.
jen-huang pushed a commit that referenced this pull request Oct 17, 2018
@cjcenizal @jen-huang
Add rollupSearchStrategy and SearchError (#20505)
5cb426e 
* Add /api/rollup/search endpoint and rollupSearchStrategy.
* Add SearchError for surfacing courier search errors.
- Use toastNotifications to surface errors in visualization editor.
- Add call-out react directive and use it to surface rollup errors in the visualization editor sidebar.
- Temporarily assign timezone and interval values from rollup job to the search to avoid errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jen-huang jen-huang jen-huang approved these changes

@bmcconaghy bmcconaghy Awaiting requested review from bmcconaghy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cjcenizal @elasticmachine @jen-huang