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

AggConfig refactoring #44497

Closed
5 tasks
Tracked by #60126
timroes opened this issue Aug 30, 2019 · 3 comments
Closed
5 tasks
Tracked by #60126

AggConfig refactoring #44497

timroes opened this issue Aug 30, 2019 · 3 comments
Labels
Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:x-large Extra Large Level of Effort Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@timroes
Copy link
Contributor

timroes commented Aug 30, 2019

Peter and I discussed recently some changes and refactoring we want to do around AggConfig, which I'd like to outline in this issue.

  • Move the advanced JSON away from an AggParam and instead make it a property with getter/setter on AggConfig itself (setDslParamsOverwrite(params: Record<string, unknown>)). Then move the editor for it directly into the vis editor code, which will handle setting this property. AggConfig should take care of using this property after generating the DSL from all parameters for overwrite. Remove the JSON AggParamType after that.
  • Do the same for the customLabel, move it to AggConfig with getter/setter and give it an editor.
  • Make AggType have one top-level toDsl method instead of every parameter having it's own write method. Currently every parameter writes itself out, or doesn't or very often just willy-nilly also handles other parameters. With one top-level method we will have a way cleaner approach to that.
    • AggParamType is still allowed to have a write method.
    • Call a toDsl(aggConfig: AggConfig, params: Record<string, unknown>) on the AggConfig when needing to generate the DSL. The params we'll pass in, will be already converted by the write method of the AggParamType if there was one (so e.g. the field parameter will already be the correct DSL, potentially a script depending on the field, and not just the plain field name).
    • The method itself returns us { aggName: string; params: Record<string, unkown>; parentAggs }, while parentAggs being the parent aggs as they exist already today.
    • That way we have one central place that generates the DSL, and parameters are not by default written into the DSL (will allow us to get rid of hundreds of write: _.noop. It also allows us to create different type of aggregations (e.g. date_histogram vs auto_date_histogram) depending on the specific AggConfig or parameters to it.
  • Try to get rid of subAggs in the toDsl calls, by actually requiring the aggregation who wants sub aggregations to directly call toDsl on the AggConfigs of it sub aggregations and assign them to its aggs parameter in the DSL.

cc @ppisljar

Parent issue #60126

@timroes timroes added Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Team:Visualizations Visualization editors, elastic-charts and infrastructure Team:AppArch labels Aug 30, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Jun 21, 2021
@exalate-issue-sync exalate-issue-sync bot added loe:x-large Extra Large Level of Effort and removed loe:small Small Level of Effort labels May 16, 2022
@vadimkibana
Copy link
Contributor

Thank you for contributing to this issue, however, we are closing this issue due to inactivity as part of a backlog grooming effort. If you believe this feature/bug should still be considered, please reopen with a comment.

@vadimkibana vadimkibana closed this as not planned Won't fix, can't repro, duplicate, stale Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:x-large Extra Large Level of Effort Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

3 participants