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

[APM] Update aggregations to support script sources #76429

Conversation

wylieconlon
Copy link
Contributor

field is not always required on aggregations, it's either field or script parameters. This distinction was represented for metric aggregations, but not for bucket aggregations.

@wylieconlon wylieconlon added Team:APM All issues that need APM UI Team support v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 1, 2020
@wylieconlon wylieconlon requested a review from a team as a code owner September 1, 2020 19:35
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

size?: number;
missing?: string;
order?: SortOptions;
execution_hint?: 'map' | 'global_ordinals';
};
} & MetricsAggregationOptions;
Copy link
Member

Choose a reason for hiding this comment

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

should we rename this then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

dgieselaar
dgieselaar previously approved these changes Sep 2, 2020
@dgieselaar
Copy link
Member

@wylieconlon hold up... I remember why i didn't approve.

@dgieselaar dgieselaar dismissed their stale review September 2, 2020 16:02

Would like to see one other change

.field,
},
cardinality:
'field' in projectionSource
Copy link
Member

@dgieselaar dgieselaar Sep 2, 2020

Choose a reason for hiding this comment

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

I don't think we (should) expect a script option here. I feel like we can solve this differently. Do you mind if I push a small change to your branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@wylieconlon wylieconlon merged commit 26d16dd into elastic:master Sep 3, 2020
@wylieconlon wylieconlon deleted the update-aggregation-types-to-support-scripts branch September 3, 2020 16:24
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 3, 2020
* master: (340 commits)
  [data.search.SearchSource] Remove legacy ES client APIs. (elastic#75943)
  [release notes] automatically retry on Github API 5xx errors (elastic#76447)
  [es_ui_shared] Fix eslint exhaustive deps rule (elastic#76392)
  [i18n] Integrate 7.9.1 Translations (elastic#76391)
  [APM] Update aggregations to support script sources (elastic#76429)
  [Security Solution] Refactor Network Top Countries to use Search Strategy (elastic#76244)
  Document security settings available on ESS (elastic#76513)
  [Ingest Manager] Add input revision to the config send to the agent (elastic#76327)
  [DOCS] Identifies cloud settings for Monitoring (elastic#76579)
  [DOCS] Identifies Cloud settings in Dev Tools (elastic#76583)
  [Ingest Manager] Better default value for fleet long polling timeout (elastic#76393)
  [data.indexPatterns] Fix broken rollup index pattern creation (elastic#76593)
  [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558)
  [Security Solution] add an excess validation instead of the exact match (elastic#76472)
  Introduce TS incremental builds & move src/test_utils to TS project  (elastic#76082)
  fix bad merge (elastic#76629)
  [Newsfeed] Ensure the version format when calling the API (elastic#76381)
  remove server_extensions mixin (elastic#76606)
  Remove legacy applications and legacy mode (elastic#75987)
  [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749)
  ...
wylieconlon pushed a commit that referenced this pull request Sep 3, 2020
* [APM] Update aggregations to support script sources

* Fix whitespace

* Fix checks

* Explicitly require field in projection

Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co>

Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants