[Dataset quality] Add Flyout Integration Actions by mohamedhamed-ahmed · Pull Request #179401 · elastic/kibana

mohamedhamed-ahmed · 2024-03-26T05:27:50Z

📝 Summary

This PR adds actions to the integration section in the dataset quality flyout.
These actions navigate to different integration-related pages.
The Dashboards action is only visible if the integration does have dashboard assets installed, otherwise its hidden.

🎥 Demo

IntegrationActions.mov

ghost · 2024-03-26T05:28:04Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2024-03-27T02:02:55Z

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

mohamedhamed-ahmed · 2024-03-27T03:10:49Z

/ci

…fix'

…ohamedhamed-ahmed/kibana into 178843-add-integration-actions

yngrdyn · 2024-03-27T10:09:01Z

/oblt-deploy-serverless

x-pack/plugins/observability_solution/dataset_quality/common/constants.ts

...ervability_solution/dataset_quality/server/routes/data_streams/get_integration_dashboards.ts

x-pack/plugins/observability_solution/dataset_quality/common/constants.ts

...observability_solution/dataset_quality/public/components/flyout/integration_actions_menu.tsx

...ugins/observability_solution/dataset_quality/public/hooks/use_flyout_integration_actions.tsx

x-pack/plugins/observability_solution/dataset_quality/public/components/flyout/types.ts

...k/plugins/observability_solution/dataset_quality/public/hooks/use_dataset_quality_flyout.tsx

...k/plugins/observability_solution/dataset_quality/public/services/data_streams_stats/types.ts

...lution/dataset_quality/public/state_machines/dataset_quality_controller/src/state_machine.ts

x-pack/plugins/observability_solution/dataset_quality/server/routes/data_streams/routes.ts

mohamedhamed-ahmed · 2024-03-28T12:50:44Z

By my understanding it should be avoided to use the saved object client on saved objects that are not defined by the current plugin (instead, the owner plugin should expose logic to do so via the contract).

@flash1293 I see the point there and I agree on the concept in general. Let me just try to explain what I have in mind.

So in our case we want to query 2 different SOs to get the data we need:

epm_pckages => this is owned by fleet and we can use the service you referred to, the only thing that need to be modified there is to make that function accept filtration options so that we can send filters to the SO. Maybe @kpollich can advice here
dashboard => I don't think this is owned by fleet and that is why they are doing the query and filtration on their side after they query the epm_packages SO, and we will still need to do the same on our side, unless this service is exposed from somewhere else that we can make use of.

Ideally how I see that interaction with SO is either types are exposed from a central place and we use the abstraction already provided by the SO, or each SO type owner exposes a service that we can use as the case with fleet here.

flash1293 · 2024-03-28T12:59:50Z

For dashboards there is the find dashboards service on the client that works on ids:

kibana/src/plugins/dashboard/public/services/dashboard_content_management/types.ts

Line 116 in 6827db4

findByIds: (ids: string[]) => Promise<FindDashboardsByIdResponse[]>;

So we could use the fleet API on the server side to get the dashboard ids we care about (with the extension on the fleet side as mentioned, but that makes sense to me) and the dashboard find service on the client:

kibana/src/plugins/dashboard/public/services/dashboard_content_management/types.ts

Line 116 in 6827db4

findByIds: (ids: string[]) => Promise<FindDashboardsByIdResponse[]>;

It looks like we need only the name, not the full saved object, maybe that's worth an extension as well. @ThomThomson do you have any guidance here? It seems like the dashboard find service sits on top of the content management service and it shouldn't be hard for it to extend to to handle this use case as well./

…fix'

kpollich · 2024-03-28T13:42:46Z

I don't think it would be a reason to block this PR, but I'm interested what @kpollich thinks about this - Kyle, in your opinion what's the "right" way to access integration details like in this PR? I can imagine this will also become relevant in the context of onboarding as it will have a similar integration of showing/linking to assets of an integration. The packageService exposed via the fleet plugin looks related in this context:

I think directly querying for saved objects managed by other plugins is probably a bit of an antipattern in Kibana, and generally the recommendation from the Kibana core folks is for plugins to wrap any saved object operations needed by other plugins in either a server side API exposed in the plugin's start contract or a REST API. Fleet actually does both in this case. Maybe it's worth pinging @elastic/kibana-core just to make sure my understanding of Kibana's recommendations here is not off-base.

The right way to access this data from another Kibana plugin is (in my opinion) probably by consuming the packageService exposed by Fleet's server-side plugin contract, then wrapping that data in an API owned by your plugin if it needs to be exposed client side.

Here's some examples of other plugins that need integration data that take that approach:

kibana/x-pack/plugins/cloud_defend/server/routes/status/status.ts

Lines 147 to 148 in 01cf912

    
           packageService.asInternalUser.getInstallation(INTEGRATION_PACKAGE_NAME), 
        
           packageService.asInternalUser.fetchFindLatestPackage(INTEGRATION_PACKAGE_NAME),

kibana/x-pack/plugins/cloud_security_posture/server/routes/status/status.ts

Lines 222 to 223 in 01cf912

    
           packageService.asInternalUser.getInstallation(CLOUD_SECURITY_POSTURE_PACKAGE_NAME), 
        
           packageService.asInternalUser.fetchFindLatestPackage(CLOUD_SECURITY_POSTURE_PACKAGE_NAME),

kibana/x-pack/plugins/observability_solution/apm/server/routes/fleet/get_latest_apm_package.ts

Lines 12 to 39 in 01cf912

    
           export async function getLatestApmPackage({ 
        
             fleetPluginStart, 
        
             request, 
        
           }: { 
        
             fleetPluginStart: NonNullable<APMPluginStartDependencies['fleet']>; 
        
             request: KibanaRequest; 
        
           }) { 
        
             const packageClient = fleetPluginStart.packageService.asScoped(request); 
        
             const latestPackage = await packageClient.fetchFindLatestPackage( 
        
               APM_PACKAGE_NAME 
        
             ); 
        
             const packageInfo = 
        
               'getBuffer' in latestPackage 
        
                 ? (await packageClient.readBundledPackage(latestPackage)).packageInfo 
        
                 : latestPackage; 
        
             const { 
        
               name, 
        
               version, 
        
               title, 
        
               policy_templates: policyTemplates, 
        
             } = packageInfo; 
        
             const firstTemplate = policyTemplates?.[0]; 
        
             const policyTemplateInputVars = 
        
               firstTemplate && 'inputs' in firstTemplate 
        
                 ? firstTemplate.inputs?.[0].vars || [] 
        
                 : []; 
        
             return { package: { name, version, title }, policyTemplateInputVars }; 
        
           }

epm_pckages => this is owned by fleet and we can use the service you referred to, the only thing that need to be modified there is to make that function accept filtration options so that we can send filters to the SO. Maybe @kpollich can advice here

More than happy to accept a PR that extends the PackageService methods with new parameters that do the filtering you're expecting here, but the filtering can also happen server side in your plugin before sending back an API response if you'd prefer to avoid codeowner churn.

jughosta

Changes in packages/deeplinks/analytics LGTM

mohamedhamed-ahmed · 2024-03-28T13:50:31Z

and the dashboard find service on the client:

@flash1293 good find, I oversaw this. TIL
I think we can use both services in this case after we get some thoughts on passing the filtration params.

dej611

Code review only for packages/deeplinks/analytics team owned files 👍

ThomThomson · 2024-04-01T15:00:54Z

It looks like we need only the name, not the full saved object

@flash1293 I remember this was discussed back when we were onboarding Dashboard to content management because there are other places where we only needed a subset of the SO's attributes. We decided to just return the full attributes in all cases but I can't remember the reasoning behind this. Maybe @sebelga would know?

Either way, I'd say it's best practice to use the findDashboardsById method and just throw away the extra data.

mohamedhamed-ahmed · 2024-04-01T16:29:59Z

@ThomThomson @kpollich Thanks both for the input here. I will update the PR shortly to consume both services and I guess I will go with filtering out what is not needed on my side and pass only the needed data to the client afterward.

mohamedhamed-ahmed · 2024-04-01T17:58:50Z

@flash1293 I have made the necessary changes to consume the fleet service.

Created an issue for the presentation team as discussed with @ThomThomson to expose the dashboard service from the server contract so that we can use it on our side.

For the time being will keep the dashboard SO querying code as is until the above ticket is tackled.

ThomThomson

Deep links changes LGTM! Code only review

yngrdyn · 2024-04-02T09:56:50Z

...st_serverless/functional/test_suites/observability/dataset_quality/dataset_quality_flyout.ts

+      await PageObjects.observabilityLogsExplorer.navigateTo();
+
+      // Add initial integrations
+      await PageObjects.observabilityLogsExplorer.setupInitialIntegrations();


Could we move this to before section? I see this is being called in two different it but being cleaned only at the end of these describe. Would that create issues when running the tests?

I see its being called in 3 different tests, I would honestly keep this way so that its not enforced on all tests in case we want to install only 1 specific package or test a case where no packages are installed

yngrdyn · 2024-04-02T10:00:26Z

...st_serverless/functional/test_suites/observability/dataset_quality/dataset_quality_flyout.ts

+
+      await action.click();
+
+      await retry.tryForTime(5000, async () => {


Could we extract this into a helper that checks users have been redirected correctly?

the thing is checking a user is redirected could be done in many ways and some logic internally can be done differently I am not sure if this can be extracted out

x-pack/test/dataset_quality_api_integration/tests/data_streams/integration_dashboards.spec.ts

...lugins/observability_solution/dataset_quality/server/routes/data_streams/get_integrations.ts

...lution/dataset_quality/public/state_machines/dataset_quality_controller/src/state_machine.ts

yngrdyn

LGTM, thanks for this work 🚀

kibana-ci · 2024-04-02T12:35:21Z

💚 Build Succeeded

Buildkite Build
Commit: 6007f62

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`datasetQuality`	174	184	+10
`management`	116	117	+1
total			+11

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/deeplinks-analytics`	6	7	+1

Async chunks

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

id	before	after	diff
`datasetQuality`	146.5KB	151.1KB	+4.6KB

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
`@kbn/router-utils`	0	1	+1

Page load bundle

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

id	before	after	diff
`dashboard`	37.0KB	37.1KB	+106.0B
`datasetQuality`	32.3KB	33.2KB	+912.0B
total			+1018.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/deeplinks-analytics`	6	7	+1

History

💚 Build #200357 succeeded 4675e8f
💚 Build #200300 succeeded 38e59db
💚 Build #199973 succeeded 27e69b3
💚 Build #199874 succeeded 083d567
💚 Build #199842 succeeded e6316b0
💚 Build #199619 succeeded 69ac289

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

cc @yngrdyn

[Dataset quality] Add Flyout Integration Actions

3c66128

mohamedhamed-ahmed added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:obs-onboarding Observability Onboarding Team v8.14.0 labels Mar 27, 2024

mohamedhamed-ahmed added 2 commits March 27, 2024 02:19

Merge branch 'main' into 178843-add-integration-actions

98bf9d1

Added API Tests

ef7ceb9

mohamedhamed-ahmed marked this pull request as ready for review March 27, 2024 02:02

mohamedhamed-ahmed requested a review from a team as a code owner March 27, 2024 02:02

kibanamachine and others added 4 commits March 27, 2024 03:16

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

3aba555

…fix'

Added UI Tests

276f211

Merge branch '178843-add-integration-actions' of https://github.com/m…

c6d8465

…ohamedhamed-ahmed/kibana into 178843-add-integration-actions

Added UI Serverless Tests

69ac289

yngrdyn self-assigned this Mar 27, 2024