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] Extend MlUrlGenerator to support other ML pages #75696

Merged
merged 35 commits into from
Sep 2, 2020

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Aug 21, 2020

Summary

This PR is part of #69265 to extend MlUrlGenerator for other apps within Kibana to access and to convert ML urls to non-hash paths.

Example usage:

  const mlUrlGenerator = useMlUrlGenerator();
  const {
    services: {
      application: { navigateToUrl },
    },
  } = useMlKibana();

  const redirectToAnalyticsManagementPage = async () => {
    const url = await mlUrlGenerator.createUrl({ page: ML_PAGES.DATA_FRAME_ANALYTICS });
    await navigateToUrl(url);
  };

Or preserve search within ml plugin

  const urlGenerator = useMlUrlGenerator();
  const navigateToPath = useNavigateToPath();

  const redirectToAnalyticsManagementPage = async () => {
    const path = await urlGenerator.createUrl({
      page: ML_TABS.DATA_FRAME_ANALYTICS',
      jobId,
      analysisType,
    });
    await navigateToPath(path, preserveSearch); // set preserve search to true
  };

Addition parameters that belong to globalApp _g and appState _a or custom to the page itself are also supported

        const url = await urlGenerator.createUrl({
          page: ML_PAGES.TIME_SERIES_EXPLORER,
          jobIds: ['logs_categorization_1'],
          detectorIndex: 0,
          entities: { mlcategory: '2' },
          refreshInterval: {
            display: 'Off',
            pause: false,
            value: 0,
          },
          timeRange: {
            from: '2020-07-12T00:39:02.912Z',
            to: '2020-07-22T15:52:18.613Z',
            mode: 'absolute',
          },
          zoom: {
            from: '2020-07-20T23:58:29.367Z',
            to: '2020-07-21T11:00:13.173Z',
          },
          query: {
            analyze_wildcard: true,
            query: '*',
          },
        });

Checklist

@qn895 qn895 added the :ml label Aug 21, 2020
@qn895 qn895 self-assigned this Aug 21, 2020
@qn895 qn895 added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 Feature:Anomaly Detection ML anomaly detection Feature:Data Frame Analytics ML data frame analytics features Feature:File and Index Data Viz ML file and index data visualizer refactoring labels Aug 21, 2020
@qn895 qn895 marked this pull request as ready for review August 21, 2020 19:15
@qn895 qn895 requested review from a team as code owners August 21, 2020 19:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@walterra
Copy link
Contributor

walterra commented Aug 24, 2020

Could we expose ML_TABS as part of the url generator itself so we don't have to do a separate import for that? For example the code could the look something like this:

const mlUrlGenerator = useMlUrlGenerator();
  const {
    services: {
      application: { navigateToUrl },
    },
  } = useMlKibana();

  const redirectToAnalyticsManagementPage = async () => {
    const url = await mlUrlGenerator.createUrl({ page: mlUrlGenerator.ML_TABS.DATA_FRAME_ANALYTICS });
    await navigateToUrl(url);
  };

Naming nit: ML_TABS is what we make show up as tabs in the UI, but I wonder if it makes more sense to rename it to ML_PAGES as part of the url generator.

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

app arch updates LGTM

@qn895
Copy link
Member Author

qn895 commented Aug 24, 2020

Naming nit: ML_TABS is what we make show up as tabs in the UI, but I wonder if it makes more sense to rename it to ML_PAGES as part of the url generator.

@walterra I agree - it's much more clear! Renamed it to ML_PAGES here: 7c9318e

@qn895
Copy link
Member Author

qn895 commented Aug 26, 2020

I noticed the links we add for the Kibana Sample data sets still use the old hash style URLs. Can these be edited to the non-hash version - defined in ml/server/lib/sample_data_sets/sample_data_sets.ts

Removed the hash here 64b936b
although I didn't use the url generator for this one. If we want to create a new `/modules/ slash page I can definitely do that.

@qn895 please take a look at #74555 (files), we should probably replace useLinkProps with ML Url Generator

Thanks for bringing it up. I will make the changes for these in a follow up PR as it involves reviews from another team.

pause: boolean;
value: number;
}

export interface TimeRange {
from: string;
to: string;
mode?: 'absolute' | 'relative';
mode?: 'absolute' | 'relative' | 'quick';
Copy link
Contributor

@lizozom lizozom Aug 27, 2020

Choose a reason for hiding this comment

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

@qn895 I'm slightly confused on how both these fields get used.

I don't even see any place in the code using them (nor here or in the data plugin), to the point I would consider removing mode 🤨

I'd love to sync on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lizozom Thanks for the feedback. Our team decided it would be good to take those parameters out as they are outdated. I have reverted the additions to the data plugin here: 5eefb05

mlUrlGeneratorState as DataFrameAnalyticsExplorationUrlState
);
case ML_PAGES.DATA_VISUALIZER:
return this.createDataVisualizerUrl(mlUrlGeneratorState as DataVisualizerUrlState);
Copy link
Member

Choose a reason for hiding this comment

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

these 3 are the same and so cold fall through to one case

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated here 4d6a5a0

/**
* Creates URL to the Anomaly Explorer page
*/
private createExplorerUrl({
refreshInterval,
Copy link
Member

Choose a reason for hiding this comment

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

this file is now very large. would it be possible/convenient to break it into multiple files? one per page perhaps.
It might make it easier to manage in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Refactor to separate files here 4d6a5a0

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Latest changes LGTM! 👍 But please pay attention to the MLPageState suggestion

CLASSIFICATION: 'classification',
} as const;

type DATAFRAME_ANALYTICS_TYPE = typeof ANALYSIS_CONFIG_TYPE[keyof typeof ANALYSIS_CONFIG_TYPE];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we only use this notation for constants. For types, it's a camel case starting with capital.

Suggested change
type DATAFRAME_ANALYTICS_TYPE = typeof ANALYSIS_CONFIG_TYPE[keyof typeof ANALYSIS_CONFIG_TYPE];
type DataFrameAnalyticsType = typeof ANALYSIS_CONFIG_TYPE[keyof typeof ANALYSIS_CONFIG_TYPE];

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated here 8248d7f

x-pack/plugins/ml/common/types/ml_url_generator.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM.

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.

A few extra minor function / file renames for the data frame analytics pages are needed.

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.

Latest edits LGTM!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
ml 1385 +7 1378

async chunks size

id value diff baseline
ml 8.2MB +930.0B 8.2MB

page load bundle size

id value diff baseline
ml 582.7KB +6.1KB 576.6KB

distributable file count

id value diff baseline
total 45539 +2 45537

History

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

@qn895 qn895 merged commit aac8424 into elastic:master Sep 2, 2020
@qn895 qn895 deleted the new-ml-url-generator branch September 2, 2020 19:52
qn895 added a commit to qn895/kibana that referenced this pull request Sep 2, 2020
qn895 added a commit that referenced this pull request Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Anomaly Detection ML anomaly detection Feature:Data Frame Analytics ML data frame analytics features Feature:File and Index Data Viz ML file and index data visualizer :ml refactoring release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants