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] Anomaly Detection: Adds a page to list supplied job configurations #191564

Merged

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Aug 28, 2024

Summary

This PR adds a page in the UI for 'Supplied configurations'
Dependent on this fix to the endpoint schema going in first: #191633

NOTE: This item will be added to the side-nav of oblt serverless once this update is in: #190458 - replaced by #192050

Adds dedicated UI page for preconfigured job packages - subitem of the Anomaly Detection navigation:
image

When they can't be run in the ML UI:
image

When selected - flyout opens to reveal package assets:
image

Clicking the Run data recognizer button shows matching data views (if any) with link to job creation:
image

Empty table when no matching dataviews are found:
image

Jobs tab of flyout:
image

kibana tab of flyout:
image

Checklist

Delete any items that are not applicable to this PR.

@alvarezmelissa87
Copy link
Contributor Author

@szabosteve - pinging you for feedback on the copy used. 🙏

@alvarezmelissa87 alvarezmelissa87 changed the title [ML] Anomaly Detection preconfigured jobs: adds page in ML UI for preconfigured job packages/ ML Modules [ML] Anomaly Detection supplied configurations: adds page in ML UI for Supplied configurations (ML Modules) Aug 28, 2024
<p>
<FormattedMessage
id="xpack.ml.anomalyDetection.suppliedConfigurationsFlyout.unableToUseModuleHelpMessage"
defaultMessage="These supplied configurations can be used in {appName}."
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like These jobs can be created in the {appName} app.?

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline, note that the Logs jobs can't be created in the Serverless oblt project, so we'll need to remove the text about using the jobs in the Logs app on serverless.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add in the check for serverless, and remove the text for 'These supplied configurations can be used in Logs' if on serverless?

Screenshot 2024-09-05 at 16 20 51

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Sep 5, 2024

Choose a reason for hiding this comment

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

@peteharverson - for serverless security, the tags have been updated to only show compatible modules so there's no need to do anything on the browserside to check.

As for oblt - do those need to be shown if they can't be used at all? Should we just remove the 'observability' tag which will ensure they aren't shown in this view at all? cc @jgowdyelastic

For example, those won't show up as they are filtered out by tag in the modules endpoint.
image

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Sep 5, 2024

Choose a reason for hiding this comment

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

Removed copy in oblt in b21695c

image

@jgowdyelastic
Copy link
Member

jgowdyelastic commented Sep 4, 2024

A viewer user gets this error when attempting to view the page. I think we should allow them to access the page, but any links to create jobs should be disabled.

image

@alvarezmelissa87
Copy link
Contributor Author

@peteharverson, @szabosteve - Thank you so much for taking a look! Would you be up for resolving the comments once you confirm the suggested update has been added? Would help reduce the noise. Thank you! 🙏

@alvarezmelissa87
Copy link
Contributor Author

A viewer user gets this error when attempting to view the page. I think we should allow them to access the page, but any links to create jobs should be disabled.

image

Good call! Updated in cdb1656

@peteharverson
Copy link
Contributor

Gave this another test against cdb1656 (stateful, and serverless oblt). Overall looks good and resolved most of my previous comments.

Only items left from my testing are:

@alvarezmelissa87
Copy link
Contributor Author

@jgowdyelastic - all updates added in c81e7f6

@alvarezmelissa87
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

UI text 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.

Test latest changes locally and on cloud, and LGTM

async getDashboardEditUrl(dashboardId: string) {
async fetchDashboardsById(ids: string[]) {
const findDashboardsService = await dashboardService.findDashboardsService();
const responses = await findDashboardsService.findByIds(ids);
Copy link
Member

@jgowdyelastic jgowdyelastic Sep 11, 2024

Choose a reason for hiding this comment

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

I'm seeing 404 errors from this call in the console if the dashboard doesn't exist.
image

I think we should catch these and just return an empty array if they can't be found.

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Sep 12, 2024

Choose a reason for hiding this comment

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

These errors are actually not thrown from here - findByIds has it's own error handling internally in src/plugins/dashboard/public/services/dashboard_content_management/lib/find_dashboards.ts and returns the caught error in an object with a 'status' and 'error' property. The response is type FindDashboardsByIdResponse - which is defined in that same file I shared. That's why here - we simply filter out the results with error status.

I think if we want to do something to remove the logged 404s, it will need to be in a separate PR and would need to chat with whoever maintains that service.

@jgowdyelastic
Copy link
Member

Styling nit pick, the kibana objects are listed in bold but the jobs are not. It would be good to be consistent. It's quite noticeable when switching tabs.

image image

@alvarezmelissa87
Copy link
Contributor Author

Styling nit pick, the kibana objects are listed in bold but the jobs are not. It would be good to be consistent. It's quite noticeable when switching tabs.

image image

Good point - updated to make the style consistent in 7d8cbbd

@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 12, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 2026 2036 +10

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
aiAssistantManagementSelection 90.6KB 90.8KB +126.0B
lists 143.1KB 143.3KB +126.0B
ml 4.6MB 4.6MB +20.2KB
securitySolution 19.7MB 19.7MB +583.0B
total +21.0KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
ml 101 102 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 418.6KB 418.8KB +161.0B
ml 78.7KB 79.1KB +388.0B
securitySolutionEss 16.3KB 16.4KB +55.0B
securitySolutionServerless 21.4KB 21.5KB +55.0B
total +659.0B
Unknown metric groups

async chunk count

id before after diff
ml 110 111 +1

ESLint disabled line counts

id before after diff
ml 563 564 +1

Total ESLint disabled count

id before after diff
ml 566 567 +1

History

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

cc @alvarezmelissa87

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

@alvarezmelissa87 alvarezmelissa87 merged commit e5600b1 into elastic:main Sep 12, 2024
42 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 12, 2024
@alvarezmelissa87 alvarezmelissa87 deleted the ml-preconfigured-jobs-page branch September 12, 2024 16:30
@peteharverson peteharverson changed the title [ML] Anomaly Detection supplied configurations: adds page in ML UI for Supplied configurations (ML Modules) [ML] Anomaly Detection: Adds a page to list supplied job configurations Sep 13, 2024
@alvarezmelissa87
Copy link
Contributor Author

Link to this view will be added to observable/stateful nav in this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:build-serverless-image ci:cloud-deploy Create or update a Cloud deployment Feature:Anomaly Detection ML anomaly detection :ml release_note:feature Makes this part of the condensed release notes v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants