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] Migrate internal urls to non-hash paths #76735

Merged
merged 81 commits into from
Sep 17, 2020

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Sep 4, 2020

Summary

This is WIP to migrate ML internal URLs to non-hash paths. Follow up of #75696 and part of meta issue to switch from HashRouter to BrowserRouter #75696.

  • Change the title of the pages (docTitle) whenever navigating to a new tab
    • Only persist the refreshInterval in the _g parameter
  • Fix some issue with router history not navigating back correctly for some pages (e.g. Filters/Calendar pages)
  • Fix broken links
    • Start trial link in data visualizer
    • Select a different index link when creating indices that are not time based
  • Replace the links for both Anomaly and DFA tables. Should work correctly for:
    • overview page
    • AD/DFA job list pages
    • Kibana management page
  • Links to jobs when creating or viewing from Kibana's sample data
  • Capabilities resolvers
    • adds redirectToMlAccessDeniedPage as a new field in PageDependencies
    • checkGetJobsCapabilitiesResolver -> adds redirectToMlAccessDeniedPage as required arg
    • checkCreateJobsCapabilitiesResolver -> adds redirectToMlAccessDeniedPage as required arg
    • checkFindFileStructurePrivilegeResolver -> adds redirectToJobsManagementPage as required arg
    • checkMlNodesAvailable -> adds redirectToJobsManagementPage as required arg
  • Links from Data Visualizer results to
    • Create new job based on index
    • View in data visualizer from file imports
    • Create new job from file imports
  • Calendars & Filters redirects (manage, create, cancel, new, save)
  • Link to calendars from the Job Details expanded panel
  • Link to SMV from the Job Details - Forecast panel
  • Link to SMV from the Job Details - Annotations table
  • Link to SMV from the Anomaly Explorer - Anomalies chart
  • Switching between the SMV and Anomaly Explorer

Potential Follow-up PRs

  • Persist the refreshInterval setting even after creating a new AD job (currently it's being reset-ed to { pause: true, value: 0 }

Checklist

Delete any items that are not applicable to this PR.

@qn895 qn895 added Feature:Anomaly Detection ML anomaly detection v8.0.0 Feature:File and Index Data Viz ML file and index data visualizer Feature:Data Frame Analytics ML data frame analytics features v7.10.0 labels Sep 4, 2020
@qn895 qn895 self-assigned this Sep 4, 2020
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.

Had a first look, overall the refactor and use of utils looks good. I added some smaller questions/suggestions.

One thing i noticed I'm not sure why this is necessary: I see the use of async/await with navigateToUrl or await mlUrlGenerator.createUrl but I'm not sure this is required?

Comment on lines +7 to +11
export const ANALYSIS_CONFIG_TYPE = {
OUTLIER_DETECTION: 'outlier_detection',
REGRESSION: 'regression',
CLASSIFICATION: 'classification',
} as const;
Copy link
Contributor

Choose a reason for hiding this comment

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

We have the same as an enum already in plugins/ml/public/application/data_frame_analytics/common/analytics.ts. This one using the const approach as well as the location here LGTM so suggest to try to get rid of the on mentioned in this comment.

Copy link
Member Author

@qn895 qn895 Sep 9, 2020

Choose a reason for hiding this comment

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

Went through and updated the usage of enum ANALYSIS_CONFIG_TYPE to const ANALYSIS_CONFIG_TYPE here dd3517b

@@ -79,3 +80,5 @@ export interface DataFrameAnalyticsConfig {
version: string;
allow_lazy_start?: boolean;
}

export type DataFrameAnalyticsType = 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 don't think we use the Type postfix in other places so suggest to just name this DataFrameAnalytics.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed here d3edff7

};

useEffect(() => {
document.title = `ML - ${TAB_DATA[selectedTabId].name} - Kibana`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if Kibana offers a way to do this more "officially" without directly manipulating document.title?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point! I do see coreStart.docTitle.change('My Title') that the Kibana team has exposed out ... Gonna take a look to see if we an implement this using that instead 👍

Copy link
Member Author

@qn895 qn895 Sep 15, 2020

Choose a reason for hiding this comment

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

Changed to using Kibana's chrome.docTitle.change() here c62a924

// globalState (e.g. selected jobs and time range) should be retained when changing pages.
// appState will not be considered.
const fullGlobalStateString = globalState !== undefined ? `?_g=${encode(globalState)}` : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the global state no longer necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still retaining the globalState here, just within redirectToTab instead of the encoded string.

…al-urls

# Conflicts:
#	x-pack/plugins/ml/public/application/settings/filter_lists/list/table.js
Comment on lines 289 to 291
analysisType: Object.keys(
item.metadata?.analytics_config.analysis
)[0] as DataFrameAnalyticsType,
Copy link
Contributor

Choose a reason for hiding this comment

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

We have getAnalysisType() available in plugins/ml/public/application/data_frame_analytics/common/analytics.ts.

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 cc8f5dd

@@ -290,7 +291,7 @@ export function getFormStateFromJobConfig(
analyticsJobConfig: Readonly<CloneDataFrameAnalyticsConfig>,
isClone: boolean = true
): Partial<State['form']> {
const jobType = Object.keys(analyticsJobConfig.analysis)[0] as ANALYSIS_CONFIG_TYPE;
const jobType = Object.keys(analyticsJobConfig.analysis)[0] as DataFrameAnalyticsType;
Copy link
Contributor

Choose a reason for hiding this comment

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

We have getAnalysisType() available in plugins/ml/public/application/data_frame_analytics/common/analytics.ts.

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 cc8f5dd

@@ -143,7 +140,7 @@ export const getAnalyticsFactory = (
checkpointing: {},
config,
id: config.id,
job_type: Object.keys(config.analysis)[0] as ANALYSIS_CONFIG_TYPE,
job_type: Object.keys(config.analysis)[0] as DataFrameAnalyticsType,
Copy link
Contributor

Choose a reason for hiding this comment

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

We have getAnalysisType() available in plugins/ml/public/application/data_frame_analytics/common/analytics.ts.

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 cc8f5dd

} = useMlKibana();

useEffect(() => {
let unamounted = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be unmounted?

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 cc8f5dd

/>
);
import { ML_PAGES } from '../../../../../common/constants/ml_url_generator';
import { Link } from 'react-router-dom';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This import could be at the top of the file.

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 cc8f5dd

@@ -71,7 +71,7 @@ export class JobsList extends Component {
return id;
}

return <ADJobIdLink id={id} />;
return <ADJobIdLink key={id} id={id} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Besides Ml as a prefix we hardly use any abbreviations in component names so I was wondering if we should make this the full written <AnomalyDetectionJobIdLink ... />.

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 cc8f5dd

@peteharverson
Copy link
Contributor

Gave latest edits a test, and all looks good. Only issue I have found is when coming into the Anomaly Detection jobs list from the Management app jobs list, and deleting the search query, the address bar URL ends up as e.g.

http://localhost:5601/zvl/app/ml/ml#/jobs?_g=(refreshInterval:(pause:!f,value:30000))

The page loads fine, but you get the extra ml# in the URL.

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Functional test fix LGTM

@qn895
Copy link
Member Author

qn895 commented Sep 16, 2020

Gave latest edits a test, and all looks good. Only issue I have found is when coming into the Anomaly Detection jobs list from the Management app jobs list, and deleting the search query, the address bar URL ends up as e.g.

http://localhost:5601/zvl/app/ml/ml#/jobs?_g=(refreshInterval:(pause:!f,value:30000))

The page loads fine, but you get the extra ml# in the URL.

Thanks for catching that. This issue should now be fixed in 92dad64.

I also realized I missed a few spots in the process of working on this issue and have made updates to those:

  • Link to calendars from the Job Details expanded panel
  • Link to SMV from the Job Details - Forecast panel
  • Link to SMV from the Job Details - Annotations table
  • Link to SMV from the Anomaly Explorer - Anomalies chart
  • Switching between the SMV and Anomaly Explorer

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.

Tested latest edits and LGTM

@qn895
Copy link
Member Author

qn895 commented Sep 17, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
ml 8.1MB +54.9KB 8.0MB

page load bundle size

id value diff baseline
ml 872.3KB +27.0KB 845.3KB

History

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

@qn895 qn895 merged commit d88b3a6 into elastic:master Sep 17, 2020
@qn895 qn895 deleted the ml-replace-internal-urls branch September 17, 2020 17:09
qn895 added a commit to qn895/kibana that referenced this pull request Sep 17, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
qn895 added a commit that referenced this pull request Sep 18, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
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 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.

7 participants