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.

Sign up for GitHub

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

Jump to bottom

[Inventory] Remove inventory dependency from observability plugin #193251

Merged
merged 8 commits into from
Sep 18, 2024
Merged

[Inventory] Remove inventory dependency from observability plugin #193251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d74c52a
[Inventory] Remove inventory dependencies
kpatticha Sep 17, 2024
9acb105
Use deep links to show the inventory nav
kpatticha Sep 18, 2024
cfcbf71
Merge branch 'main' of github.com:elastic/kibana into remove-inventor…
kpatticha Sep 18, 2024
ff54d98
Address PR comments
kpatticha Sep 18, 2024
660929c
Merge branch 'main' into remove-inventory-dependency-from-observabili…
kpatticha Sep 18, 2024
7124e05
Fix match path for inventory
kpatticha Sep 18, 2024
be72c31
Merge branch 'remove-inventory-dependency-from-observability-plugin' …
kpatticha Sep 18, 2024
a300a5f
Merge branch 'main' into remove-inventory-dependency-from-observabili…
kpatticha Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions x-pack/plugins/observability_solution/inventory/public/plugin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import React from 'react';
import ReactDOM from 'react-dom';
import { i18n } from '@kbn/i18n';
import { from, map } from 'rxjs';
import {
AppMountParameters,
APP_WRAPPER_CLASS,
Expand Down Expand Up @@ -49,6 +50,31 @@ export class InventoryPlugin
): InventoryPublicSetup {
const inventoryAPIClient = createCallInventoryAPI(coreSetup);

pluginsSetup.observabilityShared.navigation.registerSections(
kpatticha marked this conversation as resolved.
Show resolved Hide resolved
from(coreSetup.getStartServices()).pipe(
map(([coreStart, pluginsStart]) => {
return [
{
label: '',
sortKey: 101,
entries: [
{
label: i18n.translate('xpack.inventory.inventoryLinkTitle', {
defaultMessage: 'Inventory',
}),
app: INVENTORY_APP_ID,
path: '/',
matchPath(currentPath: string) {
return ['/', ''].some((testPath) => currentPath.startsWith(testPath));
},
},
],
},
];
})
)
);

coreSetup.application.register({
id: INVENTORY_APP_ID,
title: i18n.translate('xpack.inventory.appTitle', {
Expand Down
14 changes: 4 additions & 10 deletions x-pack/plugins/observability_solution/observability/kibana.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@
"id": "observability",
"server": true,
"browser": true,
"configPath": [
"xpack",
"observability"
],
"configPath": ["xpack", "observability"],
"requiredPlugins": [
"aiops",
"alerting",
Expand Down Expand Up @@ -52,19 +49,16 @@
"serverless",
"guidedOnboarding",
"observabilityAIAssistant",
"investigate",
"inventory"
"investigate"
],
"requiredBundles": [
"data",
"kibanaReact",
"kibanaUtils",
"unifiedSearch",
"stackAlerts",
"spaces",
"spaces"
],
"extraPublicDirs": [
"common"
]
"extraPublicDirs": ["common"]
}
}
19 changes: 3 additions & 16 deletions x-pack/plugins/observability_solution/observability/public/navigation_tree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,22 +49,6 @@ export function createNavTree(pluginsStart: ObservabilityPublicPluginsStart) {
return pathNameSerialized.startsWith(prepend('/app/dashboards'));
},
},
...(pluginsStart.inventory
? [
{
link: 'inventory' as const,
getIsActive: ({
pathNameSerialized,
prepend,
}: {
pathNameSerialized: string;
prepend: (path: string) => string;
}) => {
return pathNameSerialized.startsWith(prepend('/app/observability/inventory'));
},
},
]
: []),
{
link: 'observability-overview:alerts',
},
Expand All @@ -83,6 +67,9 @@ export function createNavTree(pluginsStart: ObservabilityPublicPluginsStart) {
{
link: 'slo',
},
{
link: 'inventory',
Copy link
Contributor

Choose a reason for hiding this comment

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

I found that the navigation tree affecting Serverless is located in kibana/x-pack/plugins/serverless_observability/public/navigation_tree.ts. I still need to determine how the one under /observability/ (the one you modified) impacts the system.

We can keep this in mind and I'll handle it in my issue. If we decide to leave it here, I recommend keeping it under the pluginsStart.inventory flag until we update it with the observability:entityCentricExperience advanced setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The navigation tree uses deeps links. Deeps links AFAICT used in the new stateful observability side bar and serverless.

still need to determine how the one under /observability/ (the one you modified) impacts the system.

This will update the stateful classic navigation. Instead of showing the inventory link in the observability section. it will render in a different section .

Screenshot 2024-09-18 at 11 30 48

different section in side nav
inventory_section

Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn’t block the PR from being merged, but I believe there may be a misunderstanding.

Removing the Inventory item from the xpack/plugins/observability_solution/observability/public/navigation_tree.ts doesn’t seem to impact the UI.

The Inventory is still visible in the side navigation simply by registering the plugin the way you’ve done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, wait—if the new stateful observability sidebar requires some configuration to be activated, I might be missing that. Is it the one you get when configuring a space (in that case I checked it), or is it something different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xpack/plugins/observability_solution/observability/public/navigation_tree.ts

This file is not used for the stateful classic view that you're shared the screenshot .

  • navigation_tree affects the stateful observability and probably serverless (I haven't verified this one_

Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned earlier, Serverless seems to be impacted by kibana/x-pack/plugins/serverless_observability/public/navigation_tree.ts, not kibana/x-pack/plugins/observability/public/navigation_tree.ts.

Regarding the stateful observability sidebar, I’ll reach out to you privately since it seems like I’m missing some context on that.

Thanks!

Copy link
Contributor

@mgiota mgiota Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@iblancof Since this PR is already merged, I guess the confusion is already clarified. Feel free to ping me in case something is not clear, since I am working on a o11y navigation related issue. We have a well documented issue, where the three different types of navigation are explained.

  • stateful classic
  • stateful observability (defined as stateful cloud in our issue)
  • serverless

@iblancof To your question, yep the new stateful observability sidebar is the one you get when configuring a space. You can check our issue with the flags you need to add to your kibana.dev.yml as well and the change you need to do under Stack Management > Spaces to activate this view. This new o11y navigation is controlled by the file you listed xpack/plugins/observability_solution/observability/public/navigation_tree.ts

@kpatticha I confirm that serverless nav is controlled by x-pack/plugins/serverless_observability/public/navigation_tree.ts as @iblancof mentioned above

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @mgiota,

Thank you for providing all the context!

As you mentioned, after experimenting with the code a bit, we were able to fully understand how everything works. It all makes sense to us now. Thanks again for your help!

},
{
id: 'aiMl',
title: i18n.translate('xpack.observability.obltNav.ml.aiAndMlGroupTitle', {
Expand Down
17 changes: 0 additions & 17 deletions x-pack/plugins/observability_solution/observability/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,9 @@ import { BehaviorSubject, from, map, mergeMap } from 'rxjs';

import type { AiopsPluginStart } from '@kbn/aiops-plugin/public/types';
import type { DataViewFieldEditorStart } from '@kbn/data-view-field-editor-plugin/public';
import { INVENTORY_APP_ID } from '@kbn/deeplinks-observability/constants';
import type { EmbeddableSetup } from '@kbn/embeddable-plugin/public';
import type { ExploratoryViewPublicStart } from '@kbn/exploratory-view-plugin/public';
import type { GuidedOnboardingPluginStart } from '@kbn/guided-onboarding-plugin/public';
import type { InventoryPublicSetup, InventoryPublicStart } from '@kbn/inventory-plugin/public';
import type { InvestigatePublicStart } from '@kbn/investigate-plugin/public';
import type { LicenseManagementUIPluginSetup } from '@kbn/license-management-plugin/public';
import type { LicensingPluginStart } from '@kbn/licensing-plugin/public';
Expand Down Expand Up @@ -126,7 +124,6 @@ export interface ObservabilityPublicPluginsSetup {
licensing: LicensingPluginSetup;
serverless?: ServerlessPluginSetup;
presentationUtil?: PresentationUtilPluginStart;
inventory?: InventoryPublicSetup;
}
export interface ObservabilityPublicPluginsStart {
actionTypeRegistry: ActionTypeRegistryContract;
Expand Down Expand Up @@ -165,7 +162,6 @@ export interface ObservabilityPublicPluginsStart {
dataViewFieldEditor: DataViewFieldEditorStart;
toastNotifications: ToastsStart;
investigate?: InvestigatePublicStart;
inventory?: InventoryPublicStart;
}
export type ObservabilityPublicStart = ReturnType<Plugin['start']>;

Expand Down Expand Up @@ -361,18 +357,6 @@ export class Plugin
]
: [];

const inventoryLink = pluginsSetup.inventory
? [
{
label: i18n.translate('xpack.observability.inventoryLinkTitle', {
defaultMessage: 'Inventory',
}),
app: INVENTORY_APP_ID,
path: '',
},
]
: [];

const isAiAssistantEnabled =
pluginsStart.observabilityAIAssistant?.service.isEnabled();

Expand Down Expand Up @@ -436,7 +420,6 @@ export class Plugin
sortKey: 100,
entries: [
...overviewLink,
...inventoryLink,
...alertsLink,
...sloLink,
...casesLink,
Expand Down
9 changes: 3 additions & 6 deletions x-pack/plugins/observability_solution/observability/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"public/**/*.json",
"server/**/*",
"typings/**/*",
"../../../../typings/**/*",
"../../../../typings/**/*"
],
"kbn_references": [
"@kbn/rule-data-utils",
Expand Down Expand Up @@ -94,7 +94,6 @@
"@kbn/home-plugin",
"@kbn/data-view-field-editor-plugin",
"@kbn/guided-onboarding-plugin",
"@kbn/inventory-plugin",
"@kbn/investigate-plugin",
"@kbn/license-management-plugin",
"@kbn/presentation-util-plugin",
Expand All @@ -114,9 +113,7 @@
"@kbn/io-ts-utils",
"@kbn/core-ui-settings-server-mocks",
"@kbn/es-types",
"@kbn/logging-mocks",
"@kbn/logging-mocks"
],
"exclude": [
"target/**/*"
]
"exclude": ["target/**/*"]
}