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

Make data.search.aggs available on the server. #74472

Merged
merged 11 commits into from
Aug 13, 2020

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Aug 6, 2020

Closes #65793

Summary

This PR is the final step to expose the aggs service on the server. At a high level, it does the following:

  • Moves nearly all of the service code to common to be made available on client & server.
  • Adjusts all of the imports in the relocated files.
  • Creates a "common" aggs service definition with the main service contracts that are expected to be the same on both client and server.
  • Creates specific client & server service definitions, which wire up any environment-specific dependencies to the underlying common service.
    • For example, on the client we interact with core.uiSettings a bit differently, and need to write some glue code to pass the correct dependencies to the common service.
    • And on the server, we need a scoped saved object client before we can even access uiSettings, so we must wrap the start contract in an async function which received a scoped client.
  • Removes calculateAutoTimeExpression from setup method and only provides it during start
    • This wasn't actually used anywhere in Kibana during setup, and since it takes uiSettings as a dependency it wasn't really possible to expose it on the server during setup. So I removed it to keep the setup/start contracts the same across client and server.

It's not (quite) as scary as it looks.

This is a big PR, but not as intimidating as it looks: Since we are adding a new service on the server, about 46 markdown files were automatically generated. Additionally, the majority of files touched here were simply moved without changes, or moved while only updating import paths.

Reviewers

I tried to keep a clean commit history in hopes it would make reviews easier, but another way to approach this would be to start with reviewing the 3 different aggs_service.ts files (in common, public, and server), and then moving forward from there.

Testing

Initially the server API that is introduced here is not being used elsewhere in Kibana, so the main thing to test for would be any regressions in the browser around aggs -- these would be most likely to surface in core visualizations or lens, which are using this service behind the scenes.

Specific agg types to test which are slightly more complex in terms of dependencies and therefore more likely to break: percentile ranks, date histogram, histogram, range, date range.

Dev docs

The search.aggs service in the data plugin is now available on the server. Usage is the same as on the client, except that a scoped saved objects client must be provided on the server in order to retrieve the start contract:

const savedObjectsClient = savedObjects.getScopedClient(kibanaRequest);
// `aggs.asScopedToClient` will return the same contract as is available in the browser
const aggs = await data.search.aggs.asScopedToClient(savedObjectsClient);
const allAggTypes = aggs.types.getAll();

As part of this change, the calculateAutoTimeExpression method was removed from the setup contract, and now only exists on the data plugin's start contract. The method was unused in setup elsewhere in Kibana, so it was removed for simplicity.

In addition, the agg types registry has changed and now accepts a provider function which is used to inject dependencies which may be needed in the agg type definition, specifically a getConfig function which can be used to retrieve uiSettings:

const getMyAgg = ({ getConfig }) =>
  new MetricAggType({
    name: 'myAgg',
    expressionName: 'myAggFunction',
    getSerializedFormat: (agg) => ({ id: 'number' }),
    params: [
      {
        name: 'someParam',
        write: (agg, output, aggs) => ({
          const queryLanguage = getConfig('search:queryLanguage');
          ...etc
        })
      }
    ],
  });

// register the agg type provider
dataSetup.search.aggs.registerMetric('myAgg', getMyAgg);

We expect more changes to the agg type definition over the next few minor releases, and will add release notes indicating any future updates.

@lukeelmers lukeelmers self-assigned this Aug 6, 2020
@lukeelmers lukeelmers added Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. release_note:enhancement Team:AppArch v7.10.0 v8.0.0 labels Aug 6, 2020
@lukeelmers lukeelmers added this to In progress in kibana-app-arch via automation Aug 6, 2020
@lukeelmers lukeelmers moved this from In progress to Review in progress in kibana-app-arch Aug 6, 2020
@lukeelmers lukeelmers marked this pull request as ready for review August 6, 2020 04:36
@lukeelmers lukeelmers requested a review from a team August 6, 2020 04:36
@lukeelmers lukeelmers requested a review from a team as a code owner August 6, 2020 04:36
@elasticmachine
Copy link
Contributor

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

* default timezone, but `isDefault` is not currently offered on the
* server, so we need to manually check for the default value.
*/
isDefaultTimezone: () => getConfig('dateFormat:tz') === 'Browser',
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally we'd add isDefault to the uiSettings service on the server so that we don't need to manually check against the default value. I'm looking into this as an alternative.

Comment on lines 115 to 116
buckets: defaultAggTypes.buckets.concat(cachedAggTypes.buckets),
metrics: defaultAggTypes.metrics.concat(cachedAggTypes.metrics),
Copy link
Member Author

Choose a reason for hiding this comment

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

We need the default agg types which are in the registry to have ui settings which are scoped to the current user. However, technically 3rd party plugins can register their own agg types.

So for now we are making the assumption that 3rd party agg types are not using ui settings and therefore don't require a scoped saved objects client. This means we can store one shared copy of them in setup, and then merge everything together here.

I think this is fine for now since we don't even have known 3rd party plugins registering custom agg types right now, and they'd need to be doing it from the server anyway.

Longer term we should consider revisiting the design of the agg types and perhaps simplify things so that we could do something like give each agg type its own getConfig as a convenience, which could be used inside of the agg. But this would likely require a simpler "definition" style of registry item, similar to what we've done elsewhere (like expressions).


public start({ fieldFormats, uiSettings }: AggsStartDependencies): AggsStart {
return {
asScopedToClient: async (savedObjectsClient: SavedObjectsClientContract) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Named asScopedToClient since it is consistent with core.uiSettings.asScopedToClient

@lukeelmers
Copy link
Member Author

@elasticmachine merge upstream

@lukeelmers
Copy link
Member Author

@elasticmachine merge upstream

1 similar comment
@lukeelmers
Copy link
Member Author

@elasticmachine merge upstream

@lukeelmers lukeelmers force-pushed the feat/aggs-server branch 2 times, most recently from 299ee24 to 0a1bea9 Compare August 12, 2020 22:32
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Code LGTM - focused on the changed lens test. Tested locally in Chrome, searching for regressions in lens and the visualize editor, and everything looks good.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
data 540 +4 536

page load bundle size

id value diff baseline
data 1.4MB +29.1KB 1.4MB

History

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

@lukeelmers lukeelmers merged commit fe017f5 into elastic:master Aug 13, 2020
kibana-app-arch automation moved this from Review in progress to Done in current release Aug 13, 2020
@lukeelmers lukeelmers deleted the feat/aggs-server branch August 13, 2020 19:38
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 14, 2020
* master: (23 commits)
  Adding API test for custom link transaction example (elastic#74238)
  [Uptime] Singular alert (elastic#74659)
  Revert "attempt excluding a codeowners directory" (elastic#75023)
  [Metrics UI] Remove TSVB dependency from Metrics Explorer APIs (elastic#74804)
  Remove degraded state from ES status service (elastic#75007)
  [Reporting/Functional] unskip pagination test (elastic#74973)
  [Resolver] Stale query string values are removed when resolver's component instance ID changes. (elastic#74979)
  Add public url to Workplace Search plugin (elastic#74991)
  [Reporting] Update more Server Types for TaskManager (elastic#74915)
  [I18n] verify select icu-message options are in english (elastic#74963)
  Make data.search.aggs available on the server. (elastic#74472)
  [Security Solution][Resolver] Graph Control Tests and Update Simulator Selectors (elastic#74680)
  attempt excluding a codeowners directory
  [ML] DF Analytics: allow failed job to be stopped by force via the UI (elastic#74710)
  Add kibana-core-ui-designers team (elastic#74970)
  [Metrics UI] Fix inventory footer misalignment (elastic#74707)
  Remove legacy optimizer (elastic#73154)
  Update design-specific GH code-owners (elastic#74877)
  skip test Reporting paginates content elastic#74922
  [Metrics UI] Add Jest tests for alert previews (elastic#74890)
  ...
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, ...) release_note:enhancement release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.10.0 v8.0.0
Projects
kibana-app-arch
  
Done in current release
Development

Successfully merging this pull request may close these issues.

[data.search.aggs] Make aggs available on the server
5 participants