-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[Discover] Create Discover Shared plugin and features registry #181952
Merged
tonyghiani
merged 51 commits into
elastic:main
from
tonyghiani:feature/discover-shared-plugin
May 3, 2024
Merged
Changes from 46 commits
Commits
Show all changes
51 commits
Select commit
Hold shift + click to select a range
9e09084
refactor(unified-doc-viewer): switch to dynamic utility
47a094c
wip(unified-doc-viewer): push down logs overview doc view
511b86b
wip(unified-doc-viewer): support enable/disable doc views
16473ec
wip(unified-doc-viewer): move ui actions
3714e80
wip(unified-doc-viewer): minor naming changes
91062be
wip(unified-doc-viewer): move constants into discover-utils
b97e8be
wip(unified-doc-viewer): move all required components
6e9438e
wip(unified-doc-viewer): disable e2e tests for logs explorer flyout
bbee341
wip(unified-doc-viewer): remove flyout code from logs explorer
319ca9a
wip(unified-doc-viewer): update doc view registry code
142239f
refactor(logs-explorer): disable legacy code
9c981a7
Merge branch 'main' into refactor/push-flyout-down-to-discover
tonyghiani f08fe44
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine ec4ccc7
refactor(unified-doc-views): update clone for doc views
fd62031
Merge branch 'refactor/push-flyout-down-to-discover' of github.com:to…
e6a13cd
Merge branch 'main' into refactor/push-flyout-down-to-discover
tonyghiani 9c4cae4
refactor(unified-doc-views): i18n lint
55f2714
refactor(logs-explorer): remove moced code
54367cd
refactor(unified-doc-views): update data test subj
40b3148
Merge branch 'main' into refactor/push-flyout-down-to-discover
tonyghiani eae46a9
refactor(logs-explorer): add doc comment
f1b12a9
Merge branch 'main' into refactor/push-flyout-down-to-discover
tonyghiani ca2b1af
feat(discover-utils):add FeaturesRegistry
3088891
feat(discover-shared): setup plugin
98645ae
feat(discover-shared): add DiscoverFeaturesService
5188094
feat(discover-shared): update exports and codeowners
a3432e1
refactor(discover-shared): remove unused files
bfc2d75
feature(unified-doc-viewer): register and consume ai assistant
df377c4
refactor(logs-explorer): remove legacy code
aa0d3fe
refactor(translations): update translations
2f64609
refactor(discover-shared): update request changes
43e98f5
refactor(discover-shared): remove observe/subscribe methods
5543214
refactor(discover-features-registry): move FeaturesRegistry into own …
5344db3
refactor(discover-shared): update imports
24585d9
refactor(discover-features-registry): remove superflous comment
f53b567
refactor(discover-shared): move FeaturesRegistry into discover-shared
203c1dc
refactor(unified-doc-viewer): fix conflicts
35eb217
Merge branch 'tonyghiani-feature/discover-shared-plugin' into feature…
717ca01
Merge branch 'main' into feature/discover-shared-plugin
tonyghiani a26f033
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine 630ee43
[CI] Auto-commit changed files from 'node scripts/generate codeowners'
kibanamachine dc4c908
[CI] Auto-commit changed files from 'node scripts/build_plugin_list_d…
kibanamachine f3e85b3
refactor(discover-shared): update limits
3d47ea0
refactor(discover-shared): update test loader
3d03ba6
Update .github/CODEOWNERS
tonyghiani 0ee8aae
[CI] Auto-commit changed files from 'node scripts/generate codeowners'
kibanamachine 4864d46
refactor(discover-shared): update ownership
db72111
Merge branch 'main' into feature/discover-shared-plugin
tonyghiani e4532ca
Merge branch 'main' into feature/discover-shared-plugin
tonyghiani 0515415
Merge branch 'main' into feature/discover-shared-plugin
tonyghiani 6a2561f
Merge branch 'main' into feature/discover-shared-plugin
tonyghiani File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
Validating CODEOWNERS rules …
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
# Discover Shared | ||
|
||
A stateful layer to register shared features and provide an access point to discover without a direct dependency. | ||
|
||
## Register new features | ||
|
||
The plugin exposes a service to register features that can be opinionatedly used in Discover on both the setup and start lifecycle hooks. | ||
|
||
Although this allows for greater flexibility, its purpose is not to customize Discover as a default choice but to be used as a solution to prevent cyclic dependency between plugins that interact with Discover. | ||
|
||
To register a new feature, let's take a more practical case. | ||
|
||
> _We want to introduce the LogsAIAssistant in the Discover flyout. Porting all the logic of the Observability AI Assistant into Discover is not an option, and we don't want Discover to directly depend on the AI Assistant codebase._ | ||
|
||
We can solve this case with some steps: | ||
|
||
### Define a feature registration contract | ||
|
||
First of all, we need to define an interface to which the plugin registering the AI Assistant and Discover can adhere. | ||
|
||
The `DiscoverFeaturesService` already defines a union of available features and uses them to strictly type the exposed registry from the discover_shared plugin, so we can update it with the new feature: | ||
|
||
```tsx | ||
// src/plugins/discover_shared/public/services/discover_features/types.ts | ||
|
||
export interface SecurityAIAssistantFeature { | ||
id: 'security-ai-assistant'; | ||
render: (/* Update with deps required for this integration */) => React.ReactNode; | ||
// Add any prop required for the feature | ||
} | ||
|
||
export interface ObservabilityLogsAIAssistantFeature { | ||
id: 'observability-logs-ai-assistant'; | ||
render: (deps: {doc: DataTableRecord}) => React.ReactNode; | ||
// Add any prop required for the feature | ||
} | ||
|
||
// This should be a union of all the available client features. | ||
export type DiscoverFeature = SecurityAIAssistantFeature | ObservabilityLogsAIAssistantFeature; | ||
``` | ||
|
||
### Discover consumes the registered feature | ||
|
||
Once we have an interface for the feature, Discover can now retrieve it and use its content if is registered by any app in Kibana. | ||
|
||
```tsx | ||
// Somewhere in the unified doc viewer | ||
|
||
function LogsOverviewAIAssistant ({ doc }) { | ||
const { discoverShared } = getUnifiedDocViewerServices(); | ||
|
||
const logsAIAssistantFeature = discoverShared.features.registry.getById('observability-logs-ai-assistant') | ||
|
||
if (logsAIAssistantFeature) { | ||
return logsAIAssistantFeature.render({ doc }) | ||
} | ||
} | ||
``` | ||
|
||
### Register the feature | ||
|
||
Having an interface for the feature and Discover consuming its definition, we are left with the registration part. | ||
|
||
For our example, we'll go to the logs app that owns the LogsAIAssistant codebase and register the feature: | ||
|
||
```tsx | ||
// x-pack/plugins/observability_solution/logs_shared/public/plugin.ts | ||
|
||
export class LogsSharedPlugin implements LogsSharedClientPluginClass { | ||
// The rest of the plugin implementation is hidden for a cleaner example | ||
|
||
public start(core: CoreStart, plugins: LogsSharedClientStartDeps) { | ||
const { observabilityAIAssistant } = plugins; | ||
|
||
const LogAIAssistant = createLogAIAssistant({ observabilityAIAssistant }); | ||
|
||
// Strict typing on the registry will let you know which features you can register | ||
plugins.discoverShared.features.registry.register({ | ||
id: 'observability-logs-ai-assistant', | ||
render: ({doc}) => <LogAIAssistant doc={doc}/> | ||
}) | ||
|
||
return { | ||
LogAIAssistant, | ||
}; | ||
} | ||
} | ||
``` | ||
|
||
At this point, the feature should work correctly when registered and we have not created any direct dependency between the Discover and LogsShared apps. |
58 changes: 58 additions & 0 deletions
58
src/plugins/discover_shared/common/features_registry/features_registry.test.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { FeaturesRegistry } from './features_registry'; | ||
|
||
type TestFeature = | ||
| { id: 'feature-id-1'; adHocProperty1?: string } | ||
| { id: 'feature-id-2'; adHocProperty2?: string } | ||
| { id: 'feature-id-3'; adHocProperty3?: string }; | ||
|
||
describe('FeaturesRegistry', () => { | ||
describe('#register', () => { | ||
test('should add a feature to the registry', () => { | ||
const registry = new FeaturesRegistry<TestFeature>(); | ||
|
||
registry.register({ id: 'feature-id-1' }); | ||
|
||
expect(registry.getById('feature-id-1')).toBeDefined(); | ||
}); | ||
|
||
test('should throw an error when a feature is already registered by the given id', () => { | ||
const registry = new FeaturesRegistry<TestFeature>(); | ||
|
||
registry.register({ id: 'feature-id-1' }); | ||
|
||
expect(() => registry.register({ id: 'feature-id-1' })).toThrow( | ||
'FeaturesRegistry#register: feature with id "feature-id-1" already exists in the registry.' | ||
); | ||
}); | ||
}); | ||
|
||
describe('#getById', () => { | ||
test('should retrieve a feature by its id', () => { | ||
const registry = new FeaturesRegistry<TestFeature>(); | ||
|
||
registry.register({ id: 'feature-id-1', adHocProperty1: 'test' }); | ||
registry.register({ id: 'feature-id-2', adHocProperty2: 'test' }); | ||
|
||
expect(registry.getById('feature-id-1')).toEqual({ | ||
id: 'feature-id-1', | ||
adHocProperty1: 'test', | ||
}); | ||
}); | ||
|
||
test('should return undefined if there is no feature registered by the given id', () => { | ||
const registry = new FeaturesRegistry<TestFeature>(); | ||
|
||
registry.register({ id: 'feature-id-1' }); | ||
|
||
expect(registry.getById('feature-id-2')).toBeUndefined(); | ||
}); | ||
}); | ||
}); |
26 changes: 26 additions & 0 deletions
26
src/plugins/discover_shared/common/features_registry/features_registry.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
import { BaseFeature } from './types'; | ||
|
||
export class FeaturesRegistry<Feature extends BaseFeature = BaseFeature> { | ||
private readonly features = new Map<Feature['id'], Feature>(); | ||
|
||
register(feature: Feature): void { | ||
if (this.features.has(feature.id)) { | ||
throw new Error( | ||
`FeaturesRegistry#register: feature with id "${feature.id}" already exists in the registry.` | ||
); | ||
} | ||
|
||
this.features.set(feature.id, feature); | ||
} | ||
|
||
getById<Id extends Feature['id']>(id: Id) { | ||
return this.features.get(id) as Extract<Feature, { id: Id }> | undefined; | ||
} | ||
} |
9 changes: 9 additions & 0 deletions
9
src/plugins/discover_shared/common/features_registry/index.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
export { FeaturesRegistry } from './features_registry'; |
11 changes: 11 additions & 0 deletions
11
src/plugins/discover_shared/common/features_registry/types.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
export interface BaseFeature { | ||
id: string; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
export { FeaturesRegistry } from './features_registry'; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
module.exports = { | ||
preset: '@kbn/test', | ||
rootDir: '../../..', | ||
roots: ['<rootDir>/src/plugins/discover_shared'], | ||
coverageDirectory: '<rootDir>/target/kibana-coverage/jest/src/plugins/discover_shared', | ||
coverageReporters: ['text', 'html'], | ||
collectCoverageFrom: [ | ||
'<rootDir>/src/plugins/discover_shared/{common,public,server}/**/*.{js,ts,tsx}', | ||
], | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
{ | ||
"type": "plugin", | ||
"id": "@kbn/discover-shared-plugin", | ||
"owner": "@elastic/kibana-data-discovery", | ||
"description": "A stateful layer to register shared features and provide an access point to discover without a direct dependency", | ||
"plugin": { | ||
"id": "discoverShared", | ||
"server": false, | ||
"browser": true, | ||
"requiredPlugins": [], | ||
"optionalPlugins": [], | ||
}, | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { DiscoverSharedPlugin } from './plugin'; | ||
|
||
export function plugin() { | ||
return new DiscoverSharedPlugin(); | ||
} | ||
|
||
export type { DiscoverSharedPublicSetup, DiscoverSharedPublicStart } from './types'; | ||
export type { | ||
ObservabilityLogsAIAssistantFeatureRenderDeps, | ||
ObservabilityLogsAIAssistantFeature, | ||
DiscoverFeature, | ||
} from './services/discover_features'; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { | ||
createDiscoverFeaturesServiceSetupMock, | ||
createDiscoverFeaturesServiceStartMock, | ||
} from './services/discover_features/discover_features_service.mock'; | ||
import { DiscoverSharedPublicSetup, DiscoverSharedPublicStart } from './types'; | ||
|
||
export type Setup = jest.Mocked<DiscoverSharedPublicSetup>; | ||
export type Start = jest.Mocked<DiscoverSharedPublicStart>; | ||
|
||
const createSetupContract = (): Setup => { | ||
return { | ||
features: createDiscoverFeaturesServiceSetupMock(), | ||
}; | ||
}; | ||
|
||
const createStartContract = (): Start => { | ||
return { | ||
features: createDiscoverFeaturesServiceStartMock(), | ||
}; | ||
}; | ||
|
||
export const discoverSharedPluginMock = { | ||
createSetupContract, | ||
createStartContract, | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { DiscoverFeaturesService } from './services/discover_features'; | ||
import { DiscoverSharedPublicPlugin } from './types'; | ||
|
||
export class DiscoverSharedPlugin implements DiscoverSharedPublicPlugin { | ||
private discoverFeaturesService: DiscoverFeaturesService = new DiscoverFeaturesService(); | ||
|
||
public setup() { | ||
return { | ||
features: this.discoverFeaturesService.setup(), | ||
}; | ||
} | ||
|
||
public start() { | ||
return { | ||
features: this.discoverFeaturesService.start(), | ||
}; | ||
} | ||
} |
15 changes: 15 additions & 0 deletions
15
...ugins/discover_shared/public/services/discover_features/discover_features_service.mock.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { FeaturesRegistry } from '../../../common'; | ||
import { DiscoverFeature } from './types'; | ||
|
||
const registry = new FeaturesRegistry<DiscoverFeature>(); | ||
|
||
export const createDiscoverFeaturesServiceSetupMock = () => ({ registry }); | ||
export const createDiscoverFeaturesServiceStartMock = () => ({ registry }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The original PR assigned both teams as owners. Do we no longer want to share? 😄
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.
Whops good catch, I probably missed this while fixing conflicts, fixed!
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.
@davismcphee Mmm interesting, I don't know why but the automation is removing the logs team as co-owner, I'll investigate.