-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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][Fleet] Link to ML assets from Integration > Assets tab #189767
[ML][Fleet] Link to ML assets from Integration > Assets tab #189767
Conversation
box: { | ||
incremental: true, | ||
}, | ||
filters: transformFilters, | ||
query: query?.queryText, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in dd8e5b7
@@ -33,11 +33,9 @@ const getKibanaLinkForESAsset = (type: ElasticsearchAssetType, id: string): stri | |||
case 'data_stream_ilm_policy': | |||
return `/app/management/data/index_lifecycle_management/policies/edit/${id}`; | |||
case 'transform': | |||
// TODO: Confirm link for transforms | |||
return ''; | |||
return `/app/management/data/transform?_a=(transform:(queryText:${id}))`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently for ES assets that isn't possible but I can create a follow up issue to allow it for ES assets.
case 'ml_model': | ||
// TODO: Confirm link for ml models | ||
return ''; | ||
return `/app/ml/trained_models?_a=(trained_models:(queryText:'model_id:(${id})'))`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for models - would be nice to add the description, if it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ is it possible to utilize our ML locator to retrieve these URLs? It'd make it easier to maintain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was directed to keep it consistent in that file, for now. If maintainability becomes an issue then we can bring it up.
Pinging @elastic/ml-ui (:ml) |
Pinging @elastic/fleet (Team:Fleet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fleet changes LGTM. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested url state and the links from integration packages!
@elasticmachine merge upstream |
case 'ml_model': | ||
// TODO: Confirm link for ml models | ||
return ''; | ||
return `/app/ml/trained_models?_a=(trained_models:(queryText:'model_id:(${id})'))`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ is it possible to utilize our ML locator to retrieve these URLs? It'd make it easier to maintain
.../plugins/transform/public/app/sections/transform_management/transform_management_section.tsx
Outdated
Show resolved
Hide resolved
.../plugins/transform/public/app/sections/transform_management/transform_management_section.tsx
Outdated
Show resolved
Hide resolved
.../plugins/transform/public/app/sections/transform_management/transform_management_section.tsx
Outdated
Show resolved
Hide resolved
const onTableChange: EuiBasicTableProps<TypeOfItem>['onChange'] = useCallback( | ||
({ page, sort }: CriteriaWithPagination<TypeOfItem>) => { | ||
let resultSortField = sort?.field; | ||
if (typeof resultSortField !== 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what other types can it be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I think this is holdover defensive code from the example code I was using from https://github.com/elastic/kibana/pull/140613/files in use_table_settings.ts
I think it depends on the dataset - it's possible it could not be a string type always.
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Public APIs missing comments
Async chunks
History
To update your PR or re-run it, just comment with: |
…189767) ## Summary Related issue: elastic#182199 This PR adds links for ML assets installed via Integrations. For `transform` and `ml_model` (as they are ES asset types) the manual mappings in [get_bulk_assets.ts](https://github.com/elastic/kibana/blob/ac6f643904c6bb7e8342b68e470e933e33a7c206/x-pack/plugins/fleet/server/services/epm/packages/get_bulk_assets.ts#L21) has been modified to include a link to the Transform management page and the Trained models page, respectively. The pages will be filtered to the asset id. This PR also adds the ability to save state in the url for the Transform list to allow the url to link to a filtered list of transforms. ### To test: - From the side navigation - click the `Add integrations` button at the bottom to get to the Integrations page. <img width="318" alt="image" src="https://github.com/user-attachments/assets/d4632221-7f83-4678-ac0f-cb1e20853a6d"> - To test ml models link, install the `Living off the Land Attack Detection` integration - To test transform link, install the `Lateral Movement Detection` integration - Once they are installed you can navigate to the `Installed Integrations` tab on the Integrations page <img width="660" alt="image" src="https://github.com/user-attachments/assets/8dc1db76-4b93-4057-b502-a90980a2a484"> - Select the installed package you want to view and then go to the `Assets` tab <img width="1256" alt="image" src="https://github.com/user-attachments/assets/c3382f9d-b1ed-4043-ac3f-73180effefe8"> - You can then expand the desired asset section (Ml models or transforms) and click the link to ensure it takes you to the correct place ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: Elastic Machine <[email protected]>
Summary
Related issue: #182199
This PR adds links for ML assets installed via Integrations.
For
transform
andml_model
(as they are ES asset types) the manual mappings in get_bulk_assets.ts has been modified to include a link to the Transform management page and the Trained models page, respectively. The pages will be filtered to the asset id.This PR also adds the ability to save state in the url for the Transform list to allow the url to link to a filtered list of transforms.
To test:
Add integrations
button at the bottom to get to the Integrations page.To test ml models link, install the
Living off the Land Attack Detection
integrationTo test transform link, install the
Lateral Movement Detection
integrationOnce they are installed you can navigate to the
Installed Integrations
tab on the Integrations pageAssets
tabChecklist
Delete any items that are not applicable to this PR.