-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Discover] Create Discover Shared plugin and features registry #181952
Conversation
…nyghiani/kibana into refactor/push-flyout-down-to-discover
Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs) |
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
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.
limits.yml
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 code changes still work well and LGTM 👍 Just left one minor piece of feedback.
.github/CODEOWNERS
Outdated
@@ -369,6 +369,7 @@ examples/developer_examples @elastic/appex-sharedux | |||
examples/discover_customization_examples @elastic/kibana-data-discovery | |||
x-pack/plugins/discover_enhanced @elastic/kibana-data-discovery | |||
src/plugins/discover @elastic/kibana-data-discovery | |||
src/plugins/discover_shared @elastic/kibana-data-discovery |
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.
src/plugins/discover_shared @elastic/kibana-data-discovery | |
src/plugins/discover_shared @elastic/kibana-data-discovery @elastic/obs-ux-logs-team |
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.
@@ -37,6 +38,7 @@ export function LogsOverview({ | |||
<LogsOverviewHeader doc={parsedDoc} /> | |||
<EuiHorizontalRule margin="xs" /> | |||
<LogsOverviewHighlights formattedDoc={parsedDoc} flattenedDoc={hit.flattened} /> | |||
<LogsOverviewAIAssistant doc={hit} /> |
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.
We may need to rethink how we're registering and rendering this a bit once we do the work to support the generic logs flyout + the o11y flyout, but this can be done in steps and what we have here now is a good start.
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.
Correct, this is a temporary way of consuming this features since there is not differentiation yet for flavoured versions of Discover, once we have the resolution in place we can change this to be enabled only for a specific context.
Co-authored-by: Davis McPhee <davismcphee@hotmail.com>
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Canvas Sharable Runtime
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
…ic#181952) ## 📓 Summary Closes elastic#181528 Closes elastic#181976 **N.B.** This work was initially reviewed on a separate PR at tonyghiani#1. This implementation aims to have a stateful layer that allows the management of dependencies between Discover and other plugins, reducing the need for a direct dependency. Although the initial thought was to have a plugin to register features for Discover, there might be other cases in future where we need to prevent cyclic dependencies. With this in mind, I created the plugin as a more generic solution to hold stateful logic as a communication layer for Discover <-> Plugins. ## Discover Shared Based on some recurring naming in the Kibana codebase, `discover_shared` felt like the right place for owning these dependencies and exposing Discover functionalities to the external world. It is initially pretty simple and only exposes a registry for the Discover features, but might be a good place to implement other upcoming concepts related to Discover. Also, this plugin should ideally never depend on other solution plugins and keep its dependencies to a bare minimum of packages and core/data services. ```mermaid flowchart TD A(Discover) -- Get --> E[DiscoverShared] B(Logs Explorer) -- Set --> E[DiscoverShared] C(Security) -- Set --> E[DiscoverShared] D(Any app) -- Set --> E[DiscoverShared] ``` ## DiscoverFeaturesService This service initializes and exposes a strictly typed registry to allow consumer apps to register additional features and Discover and retrieve them. The **README** file explains a real use case of when we'd need to use it and how to do that step-by-step. Although it introduces a more nested folder structure, I decided to implement the service as a client-service and expose it through the plugin lifecycle methods to provide the necessary flexibility we might need: - We don't know yet if any of the features we register will be done on the setup/start steps, so having the registry available in both places opens the door to any case we face. - The service is client-only on purpose. My opinion is that if we ever need to register features such as server services or anything else, it should be scoped to a similar service dedicated for the server lifecycle and its environment. It should never be possible to register the ObsAIAssistant presentational component from the server, as it should not be permitted to register a server service in the client registry. A server DiscoverFeaturesService is not required yet for any feature, so I left it out to avoid overcomplicating the implementation. ## FeaturesRegistry To have a strictly typed utility that suggests the available features on a registry and adheres to a base contract, the registry exposed on the DiscoverFeaturesService is an instance of the `FeaturesRegistry` class, which implements the registration/retrieval logic such that: - If a feature is already registered, is not possible to override it and an error is thrown to notify the user of the collision. - In case we need to react to registry changes, is possible to subscribe to the registry or obtain it as an observable for more complex scenarios. The FeaturesRegistry already takes care of the required logic for the registry, so that `DiscoverFeaturesService`is left with the responsibility of instantiating/exposing an instance and provide the set of allowed features. --------- Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Davis McPhee <davismcphee@hotmail.com>
📓 Summary
Closes #181528
Closes #181976
N.B. This work was initially reviewed on a separate PR at tonyghiani#1.
This implementation aims to have a stateful layer that allows the management of dependencies between Discover and other plugins, reducing the need for a direct dependency.
Although the initial thought was to have a plugin to register features for Discover, there might be other cases in future where we need to prevent cyclic dependencies.
With this in mind, I created the plugin as a more generic solution to hold stateful logic as a communication layer for Discover <-> Plugins.
Discover Shared
Based on some recurring naming in the Kibana codebase,
discover_shared
felt like the right place for owning these dependencies and exposing Discover functionalities to the external world.It is initially pretty simple and only exposes a registry for the Discover features, but might be a good place to implement other upcoming concepts related to Discover.
Also, this plugin should ideally never depend on other solution plugins and keep its dependencies to a bare minimum of packages and core/data services.
DiscoverFeaturesService
This service initializes and exposes a strictly typed registry to allow consumer apps to register additional features and Discover and retrieve them.
The README file explains a real use case of when we'd need to use it and how to do that step-by-step.
Although it introduces a more nested folder structure, I decided to implement the service as a client-service and expose it through the plugin lifecycle methods to provide the necessary flexibility we might need:
It should never be possible to register the ObsAIAssistant presentational component from the server, as it should not be permitted to register a server service in the client registry.
A server DiscoverFeaturesService is not required yet for any feature, so I left it out to avoid overcomplicating the implementation.
FeaturesRegistry
To have a strictly typed utility that suggests the available features on a registry and adheres to a base contract, the registry exposed on the DiscoverFeaturesService is an instance of the
FeaturesRegistry
class, which implements the registration/retrieval logic such that:The FeaturesRegistry already takes care of the required logic for the registry, so that
DiscoverFeaturesService
is left with the responsibility of instantiating/exposing an instance and provide the set of allowed features.