[Visualizations] Pass 'aggs' parameter to custom request handlers #71423

fbaligand · 2020-07-13T11:18:27Z

This PR lets to pass 'aggConfigs' from build_pipeline.ts to visualization_function.ts, so that visualization_function.ts can pass it to visualization requestHandler.
It is useful for a custom requestHandler to have aggConfigs, to do a Elasticsearch query.
Especially since build_pipeline.ts pass 'aggConfigs' to esaggs.ts and that requestHandler lets to customize the Elasticsearch query (and/or response transformation) done by esaggs.ts:
https://github.com/elastic/kibana/blob/master/src/plugins/visualizations/public/legacy/build_pipeline.ts#L514

Closes #67916

kibanamachine · 2020-07-13T11:18:29Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2020-07-13T11:23:40Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

timroes · 2020-07-13T11:24:34Z

Jenkins, test this

lukeelmers

Thanks for doing this @fbaligand! Looks like this will close #67916

Add a few comments/questions... LMK what you think

lukeelmers · 2020-07-13T15:22:55Z

src/plugins/visualizations/public/expressions/visualization_function.ts

@@ -107,6 +116,7 @@ export const visualization = (): ExpressionFunctionVisualization => ({
        inspectorAdapters,
        queryFilter: getFilterManager(),
        forceFetch: true,
+        aggConfigs,


Could we call this aggs here instead of aggConfigs, as this is consistent with the request handler inside of esaggs.ts, and also with our previous implementation?

OK, I'll do that.

lukeelmers · 2020-07-13T15:28:43Z

src/plugins/visualizations/public/expressions/visualization_function.ts

@@ -94,6 +100,9 @@ export const visualization = (): ExpressionFunctionVisualization => ({
    const uiStateParams = args.uiState ? JSON.parse(args.uiState) : {};
    const uiState = new PersistedState(uiStateParams);

+    const aggConfigsState = args.aggConfigs ? JSON.parse(args.aggConfigs) : {};


I don't think you want {} as a fallback here... createAggConfigs expects an array of serialized agg configs as the aggConfigsState:

createAggConfigs( indexPattern: IndexPattern, configStates: AggConfigSerialized[] ) => IAggConfigs;

So you can probably either fall back to [], or to undefined (in which case createAggConfigs will default to [] anyway).

You're right, I'll put [] here.
Unless you prefer undefined?

Makes no difference IMHO. Inside createAggConfigs it will be [] either way

lukeelmers · 2020-07-13T15:44:23Z

src/plugins/visualizations/public/expressions/visualization_function.ts

@@ -94,6 +100,9 @@ export const visualization = (): ExpressionFunctionVisualization => ({
    const uiStateParams = args.uiState ? JSON.parse(args.uiState) : {};
    const uiState = new PersistedState(uiStateParams);

+    const aggConfigsState = args.aggConfigs ? JSON.parse(args.aggConfigs) : {};
+    const aggConfigs = indexPattern ? getSearch().aggs.createAggConfigs(indexPattern, aggConfigsState) : null;


Rather than setting to null here, perhaps we should simply exclude the aggs from the requestHandler if for some reason no index pattern is present? Then they are basically an optional parameter?

So I set undefined as default, instead of null?

Yeah, I guess that's what I'm proposing. Basically so that the argument to the request handler is aggs?: IAggConfigs instead of aggs: IAggConfigs | null

I don't feel strongly about this though. In the end it won't make a huge difference since the request handler will likely check if (aggs) {...} the same either way.

fbaligand · 2020-07-13T22:22:54Z

Hi @lukeelmers,

I just fixed the 3 remarks you told me.

lukeelmers · 2020-07-13T22:25:10Z

Jenkins, test this

fbaligand · 2020-07-14T04:22:10Z

@lukeelmers
I looked at the failed tests logs, and all of them with this error:
Cannot read property 'aggs' of undefined

My guess is that getSearch() returns undefined.
Do you agree?
If so, where and what to change to fix this issue?

I précise that I did manual tests on my machine, and it worked fine.
So, in dev mode, getSearch() is not undefined.

Or else, it is aggs variable that is undefined and line 121 fails?

What do you think about?

lukeelmers · 2020-07-14T04:36:12Z

@fbaligand My gut reaction is that it's likely one of 2 things:

getSearch() is undefined as you suggest. This would make sense, however that function should be throwing an error if it is not set, so it is surprising to not see that in the CI logs
vis.data or vis.data.aggs is somehow undefined in build_pipeline, either due to an issue with build pipeline, or an issue with the way the custom vis plugin functional test is set up

I will try to run the tests locally tomorrow and see if I can pinpoint the issue

fbaligand · 2020-07-14T05:57:29Z

@lukeelmers
You’re right, it’s probably in build_pipeline.ts.
Thanks a lot for trying to run the tests locally tomorrow!

… a vis.

lukeelmers · 2020-07-14T21:56:08Z

@fbaligand I looked at this -- the issue is indeed from build_pipeline. The problem was that the vis plugin used in the functional test doesn't use aggs (or index patterns) at all, so we actually need to check that vis.data.aggs exists before adding it to the pipeline string.

I've updated build_pipeline and pushed to this branch; hopefully CI comes back green this time 🤞

fbaligand · 2020-07-15T05:07:11Z

@lukeelmers
Great!
Thanks for the time spent on this and for the fix!
Hope that tests will succeed ;)

…astic#71423)

fbaligand · 2020-07-16T21:39:18Z

Great!
Thanks @lukeelmers for your help on this PR and for the merge!

* master: (214 commits) replacing hard coded links for ela.st (elastic#72240) skip flaky suite (elastic#60865) chore(NA): teardown dynamic dll plugin (elastic#72096) Register navLink actions for declared applications (elastic#72109) Fix value for process.hash.sha256 draggable (elastic#72142) Call setupIngest before fleet_install tests (elastic#72214) [Security Solution][Detections] Better toast errors (elastic#72205) skip flaky suite (elastic#64696) [Security Solution][Detections] Disable exceptions for Threshold and ML rules (elastic#72137) [Security Solution][Detections,Lists] Miscellaneous post-FF fixes (elastic#71990) [baseline/capture] use high-memory nodes with ramDisks (elastic#71894) skip flaky suite (elastic#77207) [Maps] Fix issue preventing TMS from rendering correctly (elastic#71946) using test_user with minimum privs (elastic#71988) Fixed Webhook connector doesn't retain added HTTP header settings (elastic#71924) [Ingest Manager] Do not show enrolling and unenrolling agents as online in agent counters (elastic#71921) [Maps] fix 'New Map' from getting added to recently accessed (elastic#72125) [Visualizations] Pass 'aggs' parameter to custom request handlers (elastic#71423) [Monitoring] Out of the box alert tweaks (elastic#71942) [ML] Fix datafeed start time is incorrect when the job has trailing empty buckets (elastic#71976) ...

* master: (55 commits) updates 'External alerts' tab text (elastic#72237) [Security Solution][Case] Fix connector's dropdown with conflicting requests (elastic#72037) replacing hard coded links for ela.st (elastic#72240) skip flaky suite (elastic#60865) chore(NA): teardown dynamic dll plugin (elastic#72096) Register navLink actions for declared applications (elastic#72109) Fix value for process.hash.sha256 draggable (elastic#72142) Call setupIngest before fleet_install tests (elastic#72214) [Security Solution][Detections] Better toast errors (elastic#72205) skip flaky suite (elastic#64696) [Security Solution][Detections] Disable exceptions for Threshold and ML rules (elastic#72137) [Security Solution][Detections,Lists] Miscellaneous post-FF fixes (elastic#71990) [baseline/capture] use high-memory nodes with ramDisks (elastic#71894) skip flaky suite (elastic#77207) [Maps] Fix issue preventing TMS from rendering correctly (elastic#71946) using test_user with minimum privs (elastic#71988) Fixed Webhook connector doesn't retain added HTTP header settings (elastic#71924) [Ingest Manager] Do not show enrolling and unenrolling agents as online in agent counters (elastic#71921) [Maps] fix 'New Map' from getting added to recently accessed (elastic#72125) [Visualizations] Pass 'aggs' parameter to custom request handlers (elastic#71423) ...

kibanamachine · 2020-07-17T20:18:45Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

fbaligand · 2020-07-17T21:54:23Z

@lukeelmers
Now that ci builds are OK, could you merge the 3 backport PRs?

lukeelmers · 2020-07-18T18:41:54Z

Thanks for the reminder @fbaligand

This was my bad -- I totally missed the notifications that the build has passed 🙄

I'm merging each of them with upstream as a precautionary measure to run one more build, and this time will keep an eye out & merge them when they come back green

…astic#71423)

…1423) (#72153) Co-authored-by: Fabien Baligand <fbaligand@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…1423) (#72155) Co-authored-by: Fabien Baligand <fbaligand@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…1423) (#72378) Co-authored-by: Fabien Baligand <fbaligand@gmail.com>

fbaligand · 2020-07-19T00:45:29Z

I can see now that all backport PRs are merged!
Thanks a lot @lukeelmers ! ;)

Just a last thing: if I’m not wrong, the fix will be out on kibana v7.8.1, not v7.8.2?

lukeelmers · 2020-07-19T03:28:06Z

if I’m not wrong, the fix will be out on kibana v7.8.1, not v7.8.2?

@fbaligand Yeah, currently getting this in 7.8.1 seems likely, but I'm still not 100% certain it will make it in. I will have a better idea next week and will make sure the PR labels are adjusted accordingly.

fbaligand · 2020-07-19T05:26:07Z

@lukeelmers
I can’t hide you that I would be very sad if it is not the case.
Actually, my plugin (enhanced-table) has a blocking error with Kibana 7.8.0 (fbaligand/kibana-enhanced-table#116) that would be fixed by this PR.

lukeelmers · 2020-07-20T16:21:10Z

@fbaligand I looked into it and, barring any unforeseen circumstances, this should still make 7.8.1. I'll let you know if anything changes.

fbaligand · 2020-07-20T16:54:55Z

Great!

lukeelmers · 2020-07-28T21:35:42Z

@fbaligand Just wanted to follow up now that 7.8.1 is released and confirm that the fix is in there 🚀 https://github.com/elastic/kibana/blob/v7.8.1/src/plugins/visualizations/public/legacy/build_pipeline.ts

fbaligand · 2020-07-28T22:36:50Z

Yes, I saw that!
That’s great!
Especially for my plugin :)

pass 'aggConfigs' parameter to request handler

17448ee

fbaligand requested a review from a team as a code owner July 13, 2020 11:18

kibanamachine added the 💝community label Jul 13, 2020

timroes added Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Team:AppArch labels Jul 13, 2020

timroes assigned lukeelmers and ppisljar Jul 13, 2020

lukeelmers requested changes Jul 13, 2020

View reviewed changes

lukeelmers linked an issue Jul 13, 2020 that may be closed by this pull request

[visualizations] Add aggs argument to custom requestHandlers #67916

Closed

lukeelmers added v7.9.0 v8.0.0 release_note:fix Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.8.2 labels Jul 13, 2020

fix @lukeelmers remarks

14e7d66

This comment has been minimized.

Sign in to view

elasticmachine and others added 2 commits July 14, 2020 14:32

Merge branch 'master' into visFuncAggConfigs

af3164e

Only provide aggs argument to visualization function if aggs exist on…

68c81a6

… a vis.

lukeelmers mentioned this pull request Jul 16, 2020

[7.9] [Visualizations] Pass 'aggs' parameter to custom request handlers (#71423) #72154

Closed

lukeelmers pushed a commit to lukeelmers/kibana that referenced this pull request Jul 16, 2020

[Visualizations] Pass 'aggs' parameter to custom request handlers (el…

39087d9

…astic#71423)

lukeelmers added the v7.10.0 label Jul 16, 2020

lukeelmers mentioned this pull request Jul 16, 2020

[7.8] [Visualizations] Pass 'aggs' parameter to custom request handlers (#71423) #72155

Merged

lukeelmers pushed a commit to lukeelmers/kibana that referenced this pull request Jul 16, 2020

[Visualizations] Pass 'aggs' parameter to custom request handlers (el…

e84b959

…astic#71423)

fbaligand mentioned this pull request Jul 17, 2020

Kibana 7.8.0: buckets/metrics are not loaded on Dashboard app fbaligand/kibana-enhanced-table#116

Closed

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 17, 2020

lukeelmers pushed a commit to lukeelmers/kibana that referenced this pull request Jul 18, 2020

[Visualizations] Pass 'aggs' parameter to custom request handlers (el…

42d84ac

…astic#71423)

lukeelmers mentioned this pull request Jul 18, 2020

[7.9] [Visualizations] Pass 'aggs' parameter to custom request handlers (#71423) #72378

Merged

lukeelmers added a commit that referenced this pull request Jul 18, 2020

[Visualizations] Pass 'aggs' parameter to custom request handlers (#7…

678ca80

…1423) (#72378) Co-authored-by: Fabien Baligand <fbaligand@gmail.com>

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 18, 2020

fbaligand deleted the visFuncAggConfigs branch July 19, 2020 05:45

lukeelmers added v7.8.1 and removed v7.8.2 labels Jul 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Visualizations] Pass 'aggs' parameter to custom request handlers #71423

[Visualizations] Pass 'aggs' parameter to custom request handlers #71423

fbaligand commented Jul 13, 2020 •

edited by lukeelmers

Loading

kibanamachine commented Jul 13, 2020

elasticmachine commented Jul 13, 2020

timroes commented Jul 13, 2020

lukeelmers left a comment

lukeelmers Jul 13, 2020

fbaligand Jul 13, 2020

lukeelmers Jul 13, 2020

fbaligand Jul 13, 2020

lukeelmers Jul 13, 2020

lukeelmers Jul 13, 2020

fbaligand Jul 13, 2020

lukeelmers Jul 13, 2020

fbaligand commented Jul 13, 2020

lukeelmers commented Jul 13, 2020

fbaligand commented Jul 14, 2020

lukeelmers commented Jul 14, 2020

fbaligand commented Jul 14, 2020

This comment has been minimized.

lukeelmers commented Jul 14, 2020

fbaligand commented Jul 15, 2020

fbaligand commented Jul 16, 2020

kibanamachine commented Jul 17, 2020

fbaligand commented Jul 17, 2020

lukeelmers commented Jul 18, 2020

fbaligand commented Jul 19, 2020

lukeelmers commented Jul 19, 2020

fbaligand commented Jul 19, 2020

lukeelmers commented Jul 20, 2020

fbaligand commented Jul 20, 2020

lukeelmers commented Jul 28, 2020

fbaligand commented Jul 28, 2020

[Visualizations] Pass 'aggs' parameter to custom request handlers #71423

[Visualizations] Pass 'aggs' parameter to custom request handlers #71423

Conversation

fbaligand commented Jul 13, 2020 • edited by lukeelmers Loading

kibanamachine commented Jul 13, 2020

elasticmachine commented Jul 13, 2020

timroes commented Jul 13, 2020

lukeelmers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbaligand commented Jul 13, 2020

lukeelmers commented Jul 13, 2020

fbaligand commented Jul 14, 2020

lukeelmers commented Jul 14, 2020

fbaligand commented Jul 14, 2020

This comment has been minimized.

lukeelmers commented Jul 14, 2020

fbaligand commented Jul 15, 2020

fbaligand commented Jul 16, 2020

kibanamachine commented Jul 17, 2020

fbaligand commented Jul 17, 2020

lukeelmers commented Jul 18, 2020

fbaligand commented Jul 19, 2020

lukeelmers commented Jul 19, 2020

fbaligand commented Jul 19, 2020

lukeelmers commented Jul 20, 2020

fbaligand commented Jul 20, 2020

lukeelmers commented Jul 28, 2020

fbaligand commented Jul 28, 2020

fbaligand commented Jul 13, 2020 •

edited by lukeelmers

Loading