-
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
[Spacetime] Fields metadata services #183806
[Spacetime] Fields metadata services #183806
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
x-pack/plugins/fleet/server/services/register_integration_fields_extractor.ts
Show resolved
Hide resolved
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.
I left a number of comments which are more discussion items than requirements for approval. I'll approve once we've had a discussion.
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.
Code looks good, we had a good conversation about choices made. This is a very nice step forward independent of whether follow up work occurs or not.
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.
Fleet changes LGTM, I don't see any impact on existing service methods.
It would be good to cover the changes with unit tests.
@juliaElastic a test for the The PackageService related method is tested here along the other methods. |
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.
@tonyghiani Something that probably should be considered - as the fields extracted from integrations are cached on the server, it's possible for them to go stale:
- Kibana is started with package abc in version 1.0
- User happily uses the field meta service
- User upgrades the package to version 1.1 which adds a lot of new fields
- User is confused about what the new fields mean, but they won't get help in the UI, because the Kibana server still has the fields from version 1.0 of the abc package cached and will return them
- User needs to restart Kibana to be able to get the field help
Doesn't need to happen on this PR, I think we should get it in, but IMHO we should take care of this as especially after a package upgrade it's important to users to have accurate information.
Maybe I read the code wrong and that's already considered - if not, adding the version of the package to the cache key should resolve it.
@flash1293 this is an excellent point, thanks for raising it! It is not currently handling this scenario, and I'm 100% in this should be handled properly, clearing the cache as soon as a new package version is released and the schema might change. I'll make a note of this and take car of this behaviour in a follow-up work to keep the scope of this PR the same, as there are plans to revisit the cache also for client-side requests. |
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.
Great work on this PR mate, LGTM, left quite small nits
import pick from 'lodash/pick'; | ||
import { FieldAttribute, FieldMetadataPlain, PartialFieldMetadataPlain } from '../types'; | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-empty-interface |
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.
nit: Can you please also add a comment above as to why the ES Lint exception is in place. This helps in future to navigate why a certain line was added
@@ -0,0 +1,8 @@ | |||
/* |
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.
Liked the idea to call it the latest, you can then switch the versions in this file and others could still continue to use the latest. Hopefully it does not break for them as they will not be aware that latest has now changed. What about also exposing this V1 in addition to latest. This way consumers has choice to either import latest or specific version
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.
It makes sense, also the documentation suggests it!
The reason I avoided this is that we don't only export types but also runtime types, which would increase the size of the bundle. We can add this as soon as we need it and keep the bundle small otherwise :)
describe('useFieldsMetadata', () => { | ||
let fieldsMetadataClient: jest.Mocked<IFieldsMetadataClient>; | ||
beforeEach(async () => { | ||
jest.clearAllMocks(); |
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.
nit: I always feel with this pattern, that we have a spillover somewhere. Can we add clearAllMocks in an afterEach instead here
```ts | ||
const fields = await client.find({ | ||
fieldNames: ['@timestamp', 'onepassword.client.platform_version'], | ||
integration: '1password' |
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.
nit:missing a comma here
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
📓 Summary
Closes https://github.com/elastic/observability-dev/issues/3331
Given the needs described in the linked issue about having a centralized and async way to consume field metadata across Kibana, this work focuses on providing server/client services to consume field metadata on demand from static ECS definition and integration manifests, with the chance to extend further the possible resolution sources.
demo.mov
💡 Reviewers hints
This PR got quite long as it involves and touches different parts of the codebase, so I'll break down the interesting parts for an easier review.
More details, code examples and mechanics description can be found in the README file for the plugin.
@kbn/fields-metadata-plugin
To avoid bundling and consuming the whole ECS static definition client-side, a new plugin
@kbn/fields-metadata-plugin
is created to expose the server/client services which enable retrieving only the fields needed on a use-case basis.FieldsMetadataService server side
A
FieldsMetadataService
is instantiated on the plugin setup/start server lifecycle, exposing a client to consume the fields and setup tools for registering external dependencies.The start contract exposes a
FieldsMetadataClient
instance. With this, any application in Kibana can query for some fields using the available methods, currently:FieldsMetadataClient.prototype.getByName()
: retrieves a singleFieldMetadata
instance.FieldsMetadataClient.prototype.find()
: retrieves a record of matchingFieldMetadata
instances.FieldsMetadataClient
is instantiated with the source repositories dependencies. They act as encapsulated sources which are responsible for fetching fields from their related source. Currently, there are 2 field repository sources used in the resolution step, but we can use this concept to extend the resolution step in future with more sources (LLM, OTel, ...).The currently used sources are:
EcsFieldsRepository
: allows fetching static ECS field metadata.IntegrationFieldsRepository
: allows fetching fields from an integration package from EPR, where the fields metadata are stored. To correctly consume these fields, thefleet
plugin must be enabled, otherwise, the service won't be able to access the registered fields extractor implemented with the fleet services.As this service performs a more expensive retrieval process than the
EcsFieldsRepository
constant complexity access, a caching layer is applied to the retrieved results from the external source to minimize latency.Fields metadata API
To expose this service to the client, a first API endpoint is created to find field metadata and filter the results to minimize the served payload.
GET /internal/fields_metadata/find
supports some initial query parameters to narrow the fields' search.FieldsMetadataService client side
As we have a server-side
FieldsMetadataService
, we need a client counterpart to consume the exposed API safely and go through the validation steps.The client
FieldsMetadataService
works similarly to the server-side one, exposing a client which is returned by the public start contract of the plugin, allowing any other to directly use fields metadata client-side.This client would work well with existing state management solutions, as it's not decoupled from any library.
useFieldsMetadata
For simpler use cases where we need a quick and easy way to consume fields metadata client-side, the plugin start contract also exposes a
useFieldsMetadata
react custom hook, which is pre-created accessing the FieldsMetadataService client described above. It is important to retrieve the hook from the start contract of this plugin, as it already gets all the required dependencies injected minimizing the effort on the consumer side.The
UnifiedDocViewer
plugin changes exemplify how we can use this hook to access and filter fields' metadata quickly.registerIntegrationFieldsExtractor
(@elastic/fleet)Getting fields from an integration dataset is more complex than accessing a static dictionary of ECS fields, and to achieve that we need access to the PackageService implemented by the fleet team.
To get access to the package, maintain a proper separation of concerns and avoid a direct dependency on the fleet plugin, some actions were taken:
PackageService.prototype.getPackageFieldsMetadata()
method is implemented to keep the knowledge about retrieving package details on this service instead of mixing it on parallel services.registerIntegrationFieldsExtractor
service is created and used during the fleet plugin setup to register a callback that accesses the service as an internal user and retrieves the fields by the given parameters.registerIntegrationFieldsExtractor
function from its server setup so that we can use it to register the above-mentioned callback that retrieves fields from an integration.This inverts the dependency between
fields_metadata
andfleet
plugins so that thefields_metadata
plugin keeps zero dependencies on external apps.Adoption
We currently have places where the
@elastic/ecs
package is directly accessed and where we might be able to refactor the codebase to consume this service.EcsFlat usages in Kibana