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

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

Merged
merged 7 commits into from
Jul 16, 2020

Conversation

fbaligand
Copy link
Contributor

@fbaligand fbaligand commented Jul 13, 2020

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

@fbaligand fbaligand requested a review from a team as a code owner July 13, 2020 11:18
@kibanamachine
Copy link
Contributor

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?

@timroes timroes added Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Team:AppArch labels Jul 13, 2020
@elasticmachine
Copy link
Contributor

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

@timroes
Copy link
Contributor

timroes commented Jul 13, 2020

Jenkins, test this

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll do that.

@@ -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) : {};
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I set undefined as default, instead of null?

Copy link
Member

Choose a reason for hiding this comment

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

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.

@lukeelmers lukeelmers linked an issue Jul 13, 2020 that may be closed by this pull request
@lukeelmers 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
@fbaligand
Copy link
Contributor Author

Hi @lukeelmers,

I just fixed the 3 remarks you told me.

@lukeelmers
Copy link
Member

Jenkins, test this

@fbaligand
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

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

@lukeelmers

This comment has been minimized.

@lukeelmers
Copy link
Member

@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
Copy link
Contributor Author

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

@fbaligand
Copy link
Contributor Author

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

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 17, 2020
* 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)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 17, 2020
* 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 kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 17, 2020
@kibanamachine
Copy link
Contributor

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
Copy link
Contributor Author

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

@lukeelmers
Copy link
Member

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

lukeelmers added a commit that referenced this pull request Jul 18, 2020
…1423) (#72153)

Co-authored-by: Fabien Baligand <fbaligand@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
lukeelmers added a commit that referenced this pull request Jul 18, 2020
…1423) (#72155)

Co-authored-by: Fabien Baligand <fbaligand@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
lukeelmers added a commit that referenced this pull request Jul 18, 2020
…1423) (#72378)

Co-authored-by: Fabien Baligand <fbaligand@gmail.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 18, 2020
@fbaligand
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

@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.

@fbaligand fbaligand deleted the visFuncAggConfigs branch July 19, 2020 05:45
@lukeelmers lukeelmers added v7.8.1 and removed v7.8.2 labels Jul 20, 2020
@lukeelmers
Copy link
Member

@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
Copy link
Contributor Author

Great!

@lukeelmers
Copy link
Member

@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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💝community Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix v7.8.1 v7.9.0 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[visualizations] Add aggs argument to custom requestHandlers
6 participants