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

[Discover] Create Discover Shared plugin and features registry #181952

Merged
merged 51 commits into from May 3, 2024

Conversation

tonyghiani
Copy link
Contributor

@tonyghiani tonyghiani commented Apr 29, 2024

📓 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.

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 DiscoverFeaturesServiceis left with the responsibility of instantiating/exposing an instance and provide the set of allowed features.

Marco Antonio Ghiani and others added 30 commits April 3, 2024 11:39
…nyghiani/kibana into refactor/push-flyout-down-to-discover
@tonyghiani tonyghiani added the Project:OneDiscover Enrich Discover with contextual awareness / Merge with Logs Explorer label Apr 29, 2024
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Apr 29, 2024
@tonyghiani tonyghiani requested a review from a team as a code owner April 29, 2024 14:02
Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

limits.yml

Copy link
Contributor

@davismcphee davismcphee left a 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.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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? 😄

Copy link
Contributor Author

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!

Copy link
Contributor Author

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} />
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@weltenwort weltenwort self-requested a review May 2, 2024 13:04
@kibana-ci
Copy link
Collaborator

kibana-ci commented May 3, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discoverShared - 10 +10
observabilityLogsExplorer 214 213 -1
unifiedDocViewer 171 172 +1
total +10

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
discoverShared - 15 +15
logsExplorer 122 117 -5
logsShared 276 277 +1
total +11

Async chunks

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

id before after diff
observabilityLogsExplorer 155.1KB 154.7KB -433.0B
unifiedDocViewer 774.3KB 774.4KB +183.0B
total -250.0B

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count - 5407 +5407
total size - 9.1MB +9.1MB

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
discoverShared - 2 +2
logsExplorer 24 22 -2
total -0

Page load bundle

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

id before after diff
discoverShared - 2.0KB +2.0KB
logsExplorer 56.5KB 54.3KB -2.2KB
logsShared 221.9KB 222.1KB +272.0B
unifiedDocViewer 11.1KB 11.1KB +34.0B
total +94.0B
Unknown metric groups

API count

id before after diff
discoverShared - 16 +16
logsExplorer 122 117 -5
logsShared 302 303 +1
total +12

ESLint disabled line counts

id before after diff
discoverShared - 2 +2
logsExplorer 5 6 +1
total +3

Total ESLint disabled count

id before after diff
discoverShared - 2 +2
logsExplorer 7 8 +1
total +3

History

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

@tonyghiani tonyghiani merged commit 747ecea into elastic:main May 3, 2024
38 checks passed
@tonyghiani tonyghiani deleted the feature/discover-shared-plugin branch May 3, 2024 09:27
@kibanamachine kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels May 3, 2024
yuliacech pushed a commit to yuliacech/kibana that referenced this pull request May 3, 2024
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project Project:OneDiscover Enrich Discover with contextual awareness / Merge with Logs Explorer release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) Team:obs-ux-logs Observability Logs User Experience Team v8.15.0
Projects
None yet
9 participants