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

[Inventory][ECO] Replace Entity with InventoryEntityLatest type #198760

Merged
merged 34 commits into from
Nov 12, 2024

Conversation

crespocarlos
Copy link
Contributor

@crespocarlos crespocarlos commented Nov 4, 2024

closes #198758

Summary

This PR removes the Entity type used across the Inventory and replaces it with InventoryEntityLatest, which provides strong typing for the latest entity object. This change makes the code leverage TypeScript’s intellisense and autocompletion in the editor, making the code easier to work with and more maintainable across the codebase.

InventoryEntityLatest is the interface that the API returns and what the UI consumes. Note that this is distinct from the index mapping defined by entityLatestSchema, creating a separation layer between Elasticsearch and the UI.

@crespocarlos crespocarlos force-pushed the 196142-adjust-types branch 2 times, most recently from 222c45c to a597446 Compare November 4, 2024 16:41
@crespocarlos crespocarlos changed the title [Inventory][ECO] Refactor entity type [Inventory][ECO] Replace Entity with InventoryEntityLatest type Nov 4, 2024
@@ -11,9 +11,9 @@ import { arrayOfStringsSchema } from './common';
export const entityBaseSchema = z.object({
id: z.string(),
type: z.string(),
identity_fields: arrayOfStringsSchema,
identity_fields: z.union([arrayOfStringsSchema, z.string()]),
Copy link
Contributor Author

@crespocarlos crespocarlos Nov 4, 2024

Choose a reason for hiding this comment

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

esql returns a string when fields hold a single value.

display_name: z.string(),
metrics: z.record(z.string(), z.number()),
metrics: z.optional(z.record(z.string(), z.number())),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps we should remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The framework still supports metrics even if the Inventory doesn't use it, let's leave this for now

@crespocarlos crespocarlos added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Nov 4, 2024
@crespocarlos
Copy link
Contributor Author

/ci

Comment on lines 110 to 120
export type InventoryEntityLatest = {
entityId: string;
entityType: string;
entityIdentityFields: string | string[];
entityDisplayName: string;
entityDefinitionId: string;
entityLastSeenTimestamp: string;
entityDefinitionVersion: string;
entitySchemaVersion: string;
alertsCount?: number;
};
} & z.infer<typeof entityMetadataSchema>;
Copy link
Contributor Author

@crespocarlos crespocarlos Nov 5, 2024

Choose a reason for hiding this comment

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

The idea here is to make the API return an interface that doesn't depend in the index mapping. The Metadata, however, will return the objects as they are returned from the query

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@crespocarlos
Copy link
Contributor Author

/ci

@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 8, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #34 / @ess @serverless SecuritySolution Explore Overview Overview Network With packetbeat Make sure that we get OverviewNetwork data

Metrics [docs]

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
@kbn/entities-schema 43 45 +2

Async chunks

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

id before after diff
home 152.0KB 151.9KB -39.0B
inventory 233.7KB 233.7KB +19.0B
total -20.0B

Page load bundle

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

id before after diff
inventory 12.4KB 12.3KB -58.0B
Unknown metric groups

API count

id before after diff
@kbn/entities-schema 43 45 +2

History

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

const identityFieldsValue = this.getIdentityFieldsValue(entityLatest);
asKqlFilter(
entityInstance: {
entity: Pick<EntityInstance['entity'], 'identity_fields'>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not an entity this is an identity_fields, can 't you create a type for the identity_fields and use it here and in the entityBaseSchema?

const identityFieldsSchema = z.union([arrayOfStringsSchema, z.string()]);
export type IdentityFields = z.infer<typeof identityFieldsSchema>;
export const entityBaseSchema = z.object({
  id: z.string(),
  type: z.string(),
  identity_fields: identityFieldsSchema,
Suggested change
entity: Pick<EntityInstance['entity'], 'identity_fields'>;
identityFields: IdentityFields;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a partial entity. I'll see if this can be improved.

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@miltonhultgren miltonhultgren left a comment

Choose a reason for hiding this comment

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

Entities changes LGTM

display_name: z.string(),
metrics: z.record(z.string(), z.number()),
metrics: z.optional(z.record(z.string(), z.number())),
Copy link
Contributor

Choose a reason for hiding this comment

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

The framework still supports metrics even if the Inventory doesn't use it, let's leave this for now

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 12, 2024

💚 Build Succeeded

Metrics [docs]

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
@kbn/entities-schema 43 45 +2

Async chunks

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

id before after diff
home 152.0KB 151.9KB -39.0B
inventory 236.4KB 236.4KB +34.0B
total -5.0B

Page load bundle

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

id before after diff
inventory 12.2KB 12.1KB -58.0B
Unknown metric groups

API count

id before after diff
@kbn/entities-schema 43 45 +2

History

@crespocarlos crespocarlos merged commit c4d3de8 into elastic:main Nov 12, 2024
28 checks passed
@crespocarlos crespocarlos deleted the 196142-adjust-types branch November 12, 2024 17:47
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11802828881

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

You might need to backport the following PRs to 8.x:
- [Inventory][ECO] Use ControlGroupRenderer to filter by entity types (#199174)

Manual backport

To create the backport manually run:

node scripts/backport --pr 198760

Questions ?

Please refer to the Backport tool documentation

@crespocarlos
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

crespocarlos added a commit to crespocarlos/kibana that referenced this pull request Nov 13, 2024
…lastic#198760)

closes [elastic#198758](elastic#198758)

## Summary

This PR removes the Entity type used across the Inventory and replaces
it with `InventoryEntityLatest`, which provides strong typing for the
latest entity object. This change makes the code leverage TypeScript’s
intellisense and autocompletion in the editor, making the code easier to
work with and more maintainable across the codebase.

`InventoryEntityLatest` is the interface that the API returns and what
the UI consumes. Note that this is distinct from the index mapping
defined by `entityLatestSchema`, creating a separation layer between
Elasticsearch and the UI.

---------

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit c4d3de8)

# Conflicts:
#	x-pack/plugins/observability_solution/apm/ftr_e2e/cypress/e2e/transaction_details/transaction_details.cy.ts
crespocarlos added a commit that referenced this pull request Nov 13, 2024
…type (#198760) (#199967)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Inventory][ECO] Replace `Entity` with `InventoryEntityLatest` type
(#198760)](#198760)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Carlos
Crespo","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-12T17:47:00Z","message":"[Inventory][ECO]
Replace `Entity` with `InventoryEntityLatest` type (#198760)\n\ncloses
[#198758](https://github.com/elastic/kibana/issues/198758)\r\n\r\n##
Summary\r\n\r\nThis PR removes the Entity type used across the Inventory
and replaces\r\nit with `InventoryEntityLatest`, which provides strong
typing for the\r\nlatest entity object. This change makes the code
leverage TypeScript’s\r\nintellisense and autocompletion in the editor,
making the code easier to\r\nwork with and more maintainable across the
codebase.\r\n\r\n`InventoryEntityLatest` is the interface that the API
returns and what\r\nthe UI consumes. Note that this is distinct from the
index mapping\r\ndefined by `entityLatestSchema`, creating a separation
layer between\r\nElasticsearch and the
UI.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"c4d3de83162904d3db19e82720b2dd747dcfc5e6","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-infra_services"],"number":198760,"url":"https://github.com/elastic/kibana/pull/198760","mergeCommit":{"message":"[Inventory][ECO]
Replace `Entity` with `InventoryEntityLatest` type (#198760)\n\ncloses
[#198758](https://github.com/elastic/kibana/issues/198758)\r\n\r\n##
Summary\r\n\r\nThis PR removes the Entity type used across the Inventory
and replaces\r\nit with `InventoryEntityLatest`, which provides strong
typing for the\r\nlatest entity object. This change makes the code
leverage TypeScript’s\r\nintellisense and autocompletion in the editor,
making the code easier to\r\nwork with and more maintainable across the
codebase.\r\n\r\n`InventoryEntityLatest` is the interface that the API
returns and what\r\nthe UI consumes. Note that this is distinct from the
index mapping\r\ndefined by `entityLatestSchema`, creating a separation
layer between\r\nElasticsearch and the
UI.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"c4d3de83162904d3db19e82720b2dd747dcfc5e6"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198760","number":198760,"mergeCommit":{"message":"[Inventory][ECO]
Replace `Entity` with `InventoryEntityLatest` type (#198760)\n\ncloses
[#198758](https://github.com/elastic/kibana/issues/198758)\r\n\r\n##
Summary\r\n\r\nThis PR removes the Entity type used across the Inventory
and replaces\r\nit with `InventoryEntityLatest`, which provides strong
typing for the\r\nlatest entity object. This change makes the code
leverage TypeScript’s\r\nintellisense and autocompletion in the editor,
making the code easier to\r\nwork with and more maintainable across the
codebase.\r\n\r\n`InventoryEntityLatest` is the interface that the API
returns and what\r\nthe UI consumes. Note that this is distinct from the
index mapping\r\ndefined by `entityLatestSchema`, creating a separation
layer between\r\nElasticsearch and the
UI.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"c4d3de83162904d3db19e82720b2dd747dcfc5e6"}}]}]
BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Inventory][ECO] Use type derived from entityLatestSchema
9 participants