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

[data.search.aggs] Throw an error when trying to create an agg type that doesn't exist. #81509

Merged
merged 4 commits into from
Nov 10, 2020
Merged

[data.search.aggs] Throw an error when trying to create an agg type that doesn't exist. #81509

merged 4 commits into from
Nov 10, 2020

Conversation

lukeelmers
Copy link
Member

In #77423 we introduced a shard_delay agg type that exists for testing purposes, and is conditionally enabled via yml config.

This represents the first time we may have an agg stored in a visualization saved object which won't actually exist at runtime. In this case we should throw a clear error rather than waiting for AggConfig to break when it attempts to create an agg of type undefined.

This PR adds a check that throws an error if no matching agg type is found in the registry when someone calls createAggConfig

@lukeelmers lukeelmers added Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Oct 22, 2020
@lukeelmers lukeelmers requested a review from a team as a code owner October 22, 2020 16:40
@lukeelmers lukeelmers self-assigned this Oct 22, 2020
@elasticmachine
Copy link
Contributor

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

@lukeelmers
Copy link
Member Author

— "page load bundle size" for "data" is 34.0B over the configured limit of 1.1MB

Haha. I'm pushing us over the edge with 34.0B. I think there's another PR open which adjusts our limit, will rerun once that's merged.

lukasolson
lukasolson approved these changes Oct 26, 2020
View reviewed changes
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM, only nit I would add is that it would be nice if this were an "error" type notification instead of a "warning":

image

lukasolson
lukasolson reviewed Oct 26, 2020
View reviewed changes
src/plugins/data/common/search/aggs/agg_configs.ts Outdated
const aggType = typeof type === 'string' ? this.typesRegistry.get(type) : type;

if (!aggType) {
throw new Error(`Unable to find a registered agg type for "${type}"`);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to i18n this since it's shown in an error pop-up?

@kibanamachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

History

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

@lukeelmers lukeelmers force-pushed the fix/aggs-check-type branch 2 times, most recently from 14a4878 to 99002de Compare November 9, 2020 21:16
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 962.6KB 962.9KB +293.0B

History

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

@lukeelmers lukeelmers merged commit f193d9c into elastic:master Nov 10, 2020
@lukeelmers lukeelmers deleted the fix/aggs-check-type branch November 10, 2020 00:33
@lukeelmers lukeelmers mentioned this pull request Nov 10, 2020
Merged
lukeelmers added a commit that referenced this pull request Nov 10, 2020
@lukeelmers
[data.search.aggs] Throw an error when trying to create an agg type t…
0fcc454 
…hat doesn't exist. (#81509) (#83024)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 10, 2020
@gmmorris
Merge branch 'master' into alerts/stack-alerts-public
aadbe35 
* master: (39 commits)
  Fix ilm navigation (elastic#81664)
  [Lens] Distinct icons for XY and pie chart value labels toolbar (elastic#82927)
  [data.search.aggs] Throw an error when trying to create an agg type that doesn't exist. (elastic#81509)
  Index patterns api - load field list on server (elastic#82629)
  New events resolver (elastic#82170)
  [App Search] Misc naming tech debt (elastic#82770)
  load empty_kibana in test to have clean starting point (elastic#82772)
  Remove data <--> expressions circular dependencies. (elastic#82685)
  Update 8.0 breaking change template to gather information on how to programmatically detect it. (elastic#82905)
  Add alerting as codeowners to related documentation folder (elastic#82777)
  Add captions to user and space grid pages (elastic#82713)
  add alternate path for x-pack/Cloud test for Lens (elastic#82634)
  Uses asCurrentUser in getClusterUuid (elastic#82908)
  [Alerting][Connectors] Add new executor subaction to get 3rd party case fields (elastic#82519)
  Fix test import objects (elastic#82767)
  [ML] Add option for anomaly charts for metric detector should plot min, mean or max as appropriate (elastic#81662)
  Update alert type selection layout to rows instead of grid (elastic#73665)
  Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. (elastic#82817)
  Update grunt and related packages (elastic#79327)
  Allow the repository to search across all namespaces (elastic#82863)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasolson lukasolson lukasolson approved these changes

Assignees

@lukeelmers lukeelmers

Labels
Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lukeelmers @elasticmachine @kibanamachine @lukasolson