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

[Streams] Dashboard linking #204309

Merged
merged 149 commits into from
Jan 7, 2025
Merged

Conversation

dgieselaar
Copy link
Member

@dgieselaar dgieselaar commented Dec 14, 2024

Links dashboard to Streams.

Changes:

simianhacker and others added 30 commits November 1, 2024 11:22
Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

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

Found an issue when you delete a linked Dashboard (from the Dashboard plugin) then reload the stream. When a dashboard is "missing" or "not found" the SavedObject interface returns an error object.

From a code style / organization perspective... All the dashboard routes are in the same file where all the other APIs each individual route is in a separate file. I don't care which way we go but I think we should try to maintain consistency.


return idsByType.dashboard.flatMap((dashboardId): Asset[] => {
const dashboard = dashboardsById[dashboardId];
if (dashboard) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (dashboard) {
if (dashboard && dashboard.error == null) {

Copy link
Member

Choose a reason for hiding this comment

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

When I delete the dashboard Saved Object, dashboard will become:

{
  id: 'ca635bdd-5f15-44a0-93d1-d4edbe24d98e',
  type: 'dashboard',
  error: {
    statusCode: 404,
    error: 'Not Found',
    message: 'Saved object [dashboard/ca635bdd-5f15-44a0-93d1-d4edbe24d98e] not found'
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for catching! will add an API test for this as well

import { useStreamsAppFetch } from './use_streams_app_fetch';

export const useDashboardsFetch = (id?: string) => {
const { signal } = useAbortController();
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to use signal from const { signal } = useAbortController() vs useStreamsAppFetch((signal) => ...)?

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 assume it's not intentional, it's mostly something Joe wrote. I'll change and confirm with him

@dgieselaar
Copy link
Member Author

@simianhacker re:

From a code style / organization perspective... All the dashboard routes are in the same file where all the other APIs each individual route is in a separate file. I don't care which way we go but I think we should try to maintain consistency.

should have elaborated - I use {folder}/route.ts because that allows us to configure an ESLint rule to require explicit return types for the route handlers. The reason for that is that a route repository is a very heavy type, as it contains all the routes. if there's types in there that need to be inferred (from e.g. the handler function body), TypeScript ends up re-evaluating a lot of this code anything changes in the routes, which hurts TS responsiveness (a lot). So ideally we should use route.ts (and enable the linting rule:

files: ['x-pack/solutions/observability/plugins/apm/server/**/route.ts'],
. Are you OK with that as a follow-up? I'll also add docs in @kbn/server-route-repository.

storageSettings
);

// get the client (which is shared across all adapters)
Copy link
Contributor

Choose a reason for hiding this comment

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

(which is shared across all adapters)

What does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

its interface, will update the comment

Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the dashboard deletion bug.

@dgieselaar dgieselaar enabled auto-merge (squash) January 7, 2025 15:59
gsoldevila added a commit that referenced this pull request Jan 7, 2025
## Summary

So that it can be used from `streams_app` (o11y).
See  #204309

Steps to relocate:

1. Fetch latest `main`
2. Update the `group` and/or `visibility` in the module's manifest.
3. Run `node scripts/relocate --moveOnly
@kbn/saved-objects-tagging-plugin`
@dgieselaar dgieselaar merged commit 28414ce into elastic:main Jan 7, 2025
10 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12659295244

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
streamsApp 178 200 +22

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
streams 9 13 +4

Async chunks

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

id before after diff
streamsApp 182.2KB 190.3KB +8.1KB

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
streams 2 8 +6
Unknown metric groups

API count

id before after diff
streams 9 13 +4

References to deprecated APIs

id before after diff
streamsApp 0 2 +2

History

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 204309

Questions ?

Please refer to the Backport tool documentation

kowalczyk-krzysztof pushed a commit to kowalczyk-krzysztof/kibana that referenced this pull request Jan 7, 2025
## Summary

So that it can be used from `streams_app` (o11y).
See  elastic#204309

Steps to relocate:

1. Fetch latest `main`
2. Update the `group` and/or `visibility` in the module's manifest.
3. Run `node scripts/relocate --moveOnly
@kbn/saved-objects-tagging-plugin`
kowalczyk-krzysztof pushed a commit to kowalczyk-krzysztof/kibana that referenced this pull request Jan 7, 2025
Links dashboard to Streams.

Changes:
- Introduces `IndexStorageAdapter` to manage ES indices - see
https://github.com/dgieselaar/kibana/blob/streams-app-asset-linking/x-pack/solutions/observability/packages/utils_server/es/storage/README.md
for motivation
- Introduces `AssetClient` and `AssetService` to manage asset links with
`IndexStorageAdapter`
- `RepositorySupertestClient` to make it easier to use
`@kbn/server-route-repository` with FTR tests
- refactors related to above changes

---------

Co-authored-by: Chris Cowan <[email protected]>
Co-authored-by: Joe Reuter <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
@dgieselaar
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Jan 8, 2025
Links dashboard to Streams.

Changes:
- Introduces `IndexStorageAdapter` to manage ES indices - see
https://github.com/dgieselaar/kibana/blob/streams-app-asset-linking/x-pack/solutions/observability/packages/utils_server/es/storage/README.md
for motivation
- Introduces `AssetClient` and `AssetService` to manage asset links with
`IndexStorageAdapter`
- `RepositorySupertestClient` to make it easier to use
`@kbn/server-route-repository` with FTR tests
- refactors related to above changes

---------

Co-authored-by: Chris Cowan <[email protected]>
Co-authored-by: Joe Reuter <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 28414ce)

# Conflicts:
#	.github/CODEOWNERS
#	x-pack/test/tsconfig.json
gsoldevila added a commit to gsoldevila/kibana that referenced this pull request Jan 8, 2025
## Summary

So that it can be used from `streams_app` (o11y).
See  elastic#204309

Steps to relocate:

1. Fetch latest `main`
2. Update the `group` and/or `visibility` in the module's manifest.
3. Run `node scripts/relocate --moveOnly
@kbn/saved-objects-tagging-plugin`

(cherry picked from commit a8579bb)

# Conflicts:
#	.github/CODEOWNERS
#	packages/kbn-relocate/utils/relocate.ts
crespocarlos pushed a commit to crespocarlos/kibana that referenced this pull request Jan 8, 2025
## Summary

So that it can be used from `streams_app` (o11y).
See  elastic#204309

Steps to relocate:

1. Fetch latest `main`
2. Update the `group` and/or `visibility` in the module's manifest.
3. Run `node scripts/relocate --moveOnly
@kbn/saved-objects-tagging-plugin`
crespocarlos pushed a commit to crespocarlos/kibana that referenced this pull request Jan 8, 2025
Links dashboard to Streams.

Changes:
- Introduces `IndexStorageAdapter` to manage ES indices - see
https://github.com/dgieselaar/kibana/blob/streams-app-asset-linking/x-pack/solutions/observability/packages/utils_server/es/storage/README.md
for motivation
- Introduces `AssetClient` and `AssetService` to manage asset links with
`IndexStorageAdapter`
- `RepositorySupertestClient` to make it easier to use
`@kbn/server-route-repository` with FTR tests
- refactors related to above changes

---------

Co-authored-by: Chris Cowan <[email protected]>
Co-authored-by: Joe Reuter <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels Feature:Streams This is the label for the Streams Project release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants