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

[ML] Kibana API endpoint for histogram chart data #70976

Merged
merged 21 commits into from
Jul 14, 2020

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Jul 7, 2020

Summary

Part of #69235.

  • Introduces a dedicated Kibana API endpoint as part of ML plugin's suite of data visualizer API endpoints and moves the logic to query and transform the required data from client to server.
  • Adds support for sampling to retrieve the data for the field histograms. For now this is not configurable by the end user and is hard coded to 5000. This is to have a first iteration of this functionality in for 7.9 and protect users when querying large clusters. The button to enable the histogram charts now includes a tooltip that mentions the sampler.

Todos

  • API endpoint
  • API integration tests
  • Migrate DFA results pages
  • Migrate DFA wizard
  • Migrate Transform wizard

Checklist

For maintainers

@walterra walterra self-assigned this Jul 7, 2020
@walterra walterra added :ml Feature:Data Frame Analytics ML data frame analytics features Feature:Transforms ML transforms release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels Jul 7, 2020
@walterra walterra marked this pull request as ready for review July 9, 2020 10:50
@walterra walterra requested a review from a team as a code owner July 9, 2020 10:50
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

export const dataVisualizerFieldHistogramsSchema = schema.object({
/** Query to match documents in the index. */
query: schema.any(),
fields: schema.arrayOf(schema.any()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - can you add some sort of comment for fields too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in b794e97.

samplerShardSize,
});

return httpService.http<any>({
Copy link
Contributor

@peteharverson peteharverson Jul 9, 2020

Choose a reason for hiding this comment

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

I am repeatedly seeing this error the first time I try to view the histograms in the Transforms wizard.

The workflow I use is:

  • Open the ML plugin. Switch to the Data Frame Analytics page.
  • Starting creating a DFA job, and view the histograms for the source data.
  • Then switch to the Transform page in Stack Management.
  • Create a transform, and click the Histogram charts button

image

Once in the Transforms wizard, if I then do a browser refresh, the charts then start working as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested latest edit with 84203df and this fixes the issue with http for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peteharverson It turned out having the endpoint only as part of the ML plugin doesn't guarantee that it's available when the transform plugin loads. To avoid a runtime dependency between the transforms and ML plugin (which would mean adding ML to transforms' kibana.json) I duplicated the API endpoint so each plugin has their own endpoint to get the histogram data. Both plugins still share the code to created the server side aggregations. The update is in
a117e43, please have another look.

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest edits against a variety of indexes and LGTM.

@walterra
Copy link
Contributor Author

walterra commented Jul 9, 2020

@elasticmachine merge upstream

color="text"
onClick={toggleChartVisibility}
>
{i18n.translate('xpack.ml.dataGrid.histogramButtonText', {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we use FormattedMessage in templates?

@@ -107,7 +108,7 @@ export const Page: FC = () => {
autoRefreshSelector: true,
});

const dataLoader = new DataLoader(currentIndexPattern, kibanaConfig);
const dataLoader = new DataLoader(currentIndexPattern, getToastNotifications());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid usting cached getToastNotifications and retriveve toastNotifications from the kibana context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rest of the file uses the same approach and I didn't plan to refactor it in this PR.

Comment on lines 65 to 69
// TODO Temporary fix to make the Data Grid Histograms in the Transforms Wizard work
setDependencyCache({
http: appDependencies.http,
});

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can avoid this temp fix without significant refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 3306ff8.

@walterra
Copy link
Contributor Author

@darnautov addressed your comments, tests passed, please have another look.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
ml 387 +1 386

History

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

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM

@walterra walterra merged commit 262e075 into elastic:master Jul 14, 2020
@walterra walterra deleted the ml-api-histograms branch July 14, 2020 11:37
walterra added a commit to walterra/kibana that referenced this pull request Jul 14, 2020
- Introduces dedicated Kibana API endpoints as part of ML and transform plugin API endpoints and moves the logic to query and transform the required data from client to server.
- Adds support for sampling to retrieve the data for the field histograms. For now this is not configurable by the end user and is hard coded to 5000. This is to have a first iteration of this functionality in for 7.9 and protect users when querying large clusters. The button to enable the histogram charts now includes a tooltip that mentions the sampler.
jgowdyelastic pushed a commit that referenced this pull request Jul 14, 2020
- Introduces dedicated Kibana API endpoints as part of ML and transform plugin API endpoints and moves the logic to query and transform the required data from client to server.
- Adds support for sampling to retrieve the data for the field histograms. For now this is not configurable by the end user and is hard coded to 5000. This is to have a first iteration of this functionality in for 7.9 and protect users when querying large clusters. The button to enable the histogram charts now includes a tooltip that mentions the sampler.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 14, 2020
* master: (21 commits)
  [Maps] 7.9 design improvements (elastic#71563)
  [ML] Changing all calls to ML endpoints to use internal user (elastic#70487)
  [eventLog] prevent log writing when initialization fails (elastic#71339)
  [Observability] landing page always being displayed (elastic#71494)
  [IM] Address data stream copy feedback (elastic#71615)
  [Logs UI] Anomalies page dataset filtering (elastic#71110)
  [data.search.aggs] Remove `use_field_mapping` from top hits agg (elastic#71168)
  [ML] Anomaly swim lane embeddable navigation and filter actions (elastic#71082)
  Fixes typo in siem_cloudtrail job description (elastic#71569)
  Require granted API Keys to have a name (elastic#71623)
  Update  getUsageForCollection (elastic#71609)
  Only fetch saved elements once (elastic#71310)
  [SecuritySolution][Resolver] Adding siem index and guarding process ancestry (elastic#71570)
  [APM] Additional data telemetry changes (elastic#71112)
  [Visualize] Fix export table for table export links (elastic#71249)
  [Search] Server side search API (elastic#70446)
  use inclusive language (elastic#71607)
  [Security Solution] Hide timeline footer when Resolver is open (elastic#71516)
  [Index template wizard] Remove shadow and use border for components panels (elastic#71606)
  [ML] Kibana API endpoint for histogram chart data (elastic#70976)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Frame Analytics ML data frame analytics features Feature:Transforms ML transforms :ml release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants