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

[Logs UI] Add ML module setup hook #43050

Closed

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Aug 9, 2019

⚠️ This is just a draft for inclusion in a larger PR. It includes commits from the prior unmerged PR #42931.

Summary

This sketches how the ML module setup API can be called for the log analysis module (defined in #42872) using the new overrides enabled by #42946.

@weltenwort weltenwort added Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Aug 9, 2019
@weltenwort weltenwort self-assigned this Aug 9, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Kerry350 Kerry350 marked this pull request as ready for review August 13, 2019 20:15
@Kerry350 Kerry350 requested a review from a team as a code owner August 13, 2019 20:15
@Kerry350 Kerry350 changed the title [Logs UI] Draft: Add ML module setup hook [Logs UI] Add ML module setup hook Aug 13, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

I'm pulling this in now to review now that the module PR is merged and I can test it. Also this comment review should make the infra-logs-ui team review request go away...

@Zacqary
Copy link
Contributor

Zacqary commented Aug 15, 2019

How is this hook expected to be used? Is it something like this?

const [setupMlModule] = useLogAnalysisJobs();

return <button onClick={setupMlModule}>Create ML Job</button>

@Kerry350
Copy link
Contributor

Kerry350 commented Aug 15, 2019

@Zacqary Pretty much, yep, useLogAnalysisJobs() is already being used in the page component for things like checking the job status. This should be extended to now make use of setupMlModule and setupMlModuleRequest too. Those can be passed directly as props to the setup screen component, <AnalysisSetupContent />, from the page component. No need to call the hook again.

Behind the scenes this uses useTrackedPromise which is used in quite a few places now for data fetching with hooks.setupMlModule will execute the request itself, and setupMlModuleRequest will provide you with state to see if the request is saving / loading.

What I'd recommend is not exporting the setupMlModuleRequest variable directly from the hook (it is at the moment as I needed to satisfy the build and not have variables that were defined but not used). But instead setup another memoized variable, like with the job status in the same hook, and export that variable. E.g.:

const isSettingUpMLModule = useMemo(() => setupMlModuleRequest.state === 'pending', [
    setupMlModuleRequest.state,
  ]);

Just like the job status check. This will provide you with the ability to disable the save button and so on.

@jasonrhodes
Copy link
Member

We're going to close this and move forward with the PR that @Zacqary opened off of this branch (#43413), since that PR includes some fixes and tweaks of this work.

@weltenwort weltenwort added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants