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

[Infrastructure UI] Implement inventory views CRUD endpoints #154900

Merged
merged 27 commits into from
Apr 24, 2023

Conversation

tonyghiani
Copy link
Contributor

@tonyghiani tonyghiani commented Apr 13, 2023

📓 Summary

Part of #152617
Closes #155158

This PR implements the CRUD endpoints for the inventory views.
Following the approach used for the LogViews service, it exposes a client that abstracts all the logic concerned to the inventory-view saved objects.

It also follows the guideline provided for Versioning interfaces and Versioning HTTP APIs, preparing for the serverless.

🤓 Tips for the reviewer

You can open the Kibana dev tools and play with the following snippet to test the create APIs, or you can perform the same requests with your preferred client:

// Get all
GET kbn:/api/infra/inventory_views

// Create one
POST kbn:/api/infra/inventory_views
{
  "attributes": {
    "name": "My inventory view"
  }
}

// Get one
GET kbn:/api/infra/inventory_views/<switch-with-id>

// Update one
PUT kbn:/api/infra/inventory_views/<switch-with-id>
{
  "attributes": {
    "name": "My inventory view 2"
  }
}

// Delete one
DELETE kbn:/api/infra/inventory_views/<switch-with-id>

👣 Next steps

  • Replicate the same logic for the metrics explorer saved object
  • Create a client-side abstraction to consume the service
  • Update the existing react custom hooks to consume the endpoint

@tonyghiani tonyghiani added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes labels Apr 13, 2023
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@tonyghiani tonyghiani marked this pull request as ready for review April 17, 2023 12:51
@tonyghiani tonyghiani requested review from a team as code owners April 17, 2023 12:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

Changes in kbn-io-ts-utils package LGTM


export type InventoryViewRequestQuery = rt.TypeOf<typeof inventoryViewRequestQueryRT>;

const inventoryViewAttributesResponseRT = rt.intersection([
Copy link
Contributor Author

@tonyghiani tonyghiani Apr 19, 2023

Choose a reason for hiding this comment

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

note: for the attributes of this saved object, the configuration has always been dynamic and not strictly typed. As they can vary depending on the user usage on the page, I left it open to all the attributes as it was before, marking only those that I'm sure will be present in the response.

This applies to any other attributes definition you'll find for this attribute's saved object.

Comment on lines +30 to +31
isDefault: rt.boolean,
isStatic: rt.boolean,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note:

  • isDefault represent whether a saved view is the default one by user preference.
  • isStatic is used to distinguish the static saved view that will always be present and served, now hardcoded in the server and not configurable.

Copy link
Member

Choose a reason for hiding this comment

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

Can those values change using the API? For example, using the update endpoint to set another default view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As those values are derived as:

  • isDefault: match with the inventoryDefaultView configuration saved in the source configuration
  • isStatic is a statically set value to false

They could insert those values triggering a manual request to the endpoint, but I guess is better to handle the case, I'll change that!

Copy link
Contributor Author

@tonyghiani tonyghiani Apr 19, 2023

Choose a reason for hiding this comment

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

note: moved the inventory-view saved object type here among the others, with #155117 I'll clean up the legacy usage of this SO defined in common/saved_objects. (Same applies for the metrics-explorer-view SO)

private readonly infraSources: IInfraSources
) {}

static STATIC_VIEW_ID = '0';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: keep the previous id for the static view always present.
I initially wanted to change it to the static string, but as it relies on a value saved in the infrastructure-ui-source SO, changing it would imply also writing a migration for that SO, resulting in overengineering a simple rename.

Comment on lines +52 to +64
const defaultView = InventoryViewsClient.createStaticView(
sourceConfiguration.configuration.inventoryDefaultView
);
const views = inventoryViewSavedObject.saved_objects.map((savedObject) =>
this.mapSavedObjectToInventoryView(
savedObject,
sourceConfiguration.configuration.inventoryDefaultView
)
);

const inventoryViews = [defaultView, ...views];

const sortedInventoryViews = this.moveDefaultViewOnTop(inventoryViews);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: When getting all the available inventory views, we want them to:

  • Always include the hardcoded static inventory view.
  • Map into a reduced version to avoid large payloads.
  • Sorted by creation date, but having on top the default view selected by the user.

To calculate which one is the default view and not delegate this to the client, we retrieve in parallel the source configuration to compute in runtime the isDefault attribute for each inventory view.

Comment on lines +177 to +190
private async assertNameConflict(name: string, whitelist: string[] = []) {
const results = await this.savedObjectsClient.find<InventoryViewAttributes>({
type: inventoryViewSavedObjectName,
perPage: 1000,
});

const hasConflict = [InventoryViewsClient.createStaticView(), ...results.saved_objects].some(
(obj) => !whitelist.includes(obj.id) && obj.attributes.name === name
);

if (hasConflict) {
throw Boom.conflict('A view with that name already exists.');
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: we can't delegate this search to the savedObjectsClient.find method since, being the attributes mapping set to dynamic: false, searching for the name won't return results.

@jennypavlova jennypavlova self-requested a review April 20, 2023 15:39
Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

Great work 👏

Tried the requests, and everything worked 🎉 I have several questions regarding the update endpoint (it should be PUT not POST right? - the PR description can be updated in that case)

  • I tried to update isStatic or isDefault and didn't work - that is correct, right? Only the name can be changed?
  • The default view can't be updated/deleted right? Because the API returns 404 if I try to (using 0 as id).

Comment on lines +30 to +31
isDefault: rt.boolean,
isStatic: rt.boolean,
Copy link
Member

Choose a reason for hiding this comment

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

Can those values change using the API? For example, using the update endpoint to set another default view?


return response.noContent();
} catch (error) {
if (isBoom(error)) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: If the id is set to 0 (Default view id) the response is 404, should we have a message explaining that the default view can't be deleted or something like that - because the view is returned from the GET and maybe this can be confusing 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the client will not be possible to delete the static view, and in case they try to do it manually, they'll get the 404 value, but I agree it could be confusing, I'll introduce an error for this case to handle it

}
```

## Update one: `PUT /api/infra/inventory_views/{inventoryViewId}`
Copy link
Member

Choose a reason for hiding this comment

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

Should we mention which properties can be updated?

@tonyghiani
Copy link
Contributor Author

Thanks for the review Jenny!

Tried the requests, and everything worked 🎉 I have several questions regarding the update endpoint (it should be PUT not POST right? - the PR description can be updated in that case)

Correct, it is implemented as a PUT, it was my mistake on the PR description, corrected 👍

I tried to update isStatic or isDefault and didn't work - that is correct, right? Only the name can be changed?

Exactly, those values are not writable, I'll take care of updating the validation against those two values!

The default view can't be updated/deleted right? Because the API returns 404 if I try to (using 0 as id).

The static view (I called it like this to distinguish it from the default view, which is a preference setting chosen by the user) is not configurable/deletable, I will add proper error handling in case the user attempt to change it.

Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Thank you for adding the changes!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1345 1352 +7

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/io-ts-utils 38 39 +1
infra 41 42 +1
total +2

Async chunks

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

id before after diff
infra 2.0MB 2.0MB +1.0KB

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
infra 9 10 +1
Unknown metric groups

API count

id before after diff
@kbn/io-ts-utils 38 39 +1
infra 44 45 +1
total +2

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 395 398 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 475 478 +3
total +5

History

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

@tonyghiani tonyghiani merged commit 6ac4e19 into elastic:main Apr 24, 2023
@tonyghiani tonyghiani deleted the 152617-saved-views-http-endpoints branch April 24, 2023 09:54
@kibanamachine kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Apr 24, 2023
nikitaindik pushed a commit to nikitaindik/kibana that referenced this pull request Apr 25, 2023
…#154900)

## 📓  Summary

Part of elastic#152617 
Closes elastic#155158 

This PR implements the CRUD endpoints for the inventory views.
Following the approach used for the LogViews service, it exposes a
client that abstracts all the logic concerned to the `inventory-view`
saved objects.

It also follows the guideline provided for [Versioning
interfaces](https://docs.elastic.dev/kibana-dev-docs/versioning-interfaces)
and [Versioning HTTP
APIs](https://docs.elastic.dev/kibana-dev-docs/versioning-http-apis),
preparing for the serverless.

## 🤓 Tips for the reviewer
You can open the Kibana dev tools and play with the following snippet to
test the create APIs, or you can perform the same requests with your
preferred client:
```
// Get all
GET kbn:/api/infra/inventory_views

// Create one
POST kbn:/api/infra/inventory_views
{
  "attributes": {
    "name": "My inventory view"
  }
}

// Get one
GET kbn:/api/infra/inventory_views/<switch-with-id>

// Update one
PUT kbn:/api/infra/inventory_views/<switch-with-id>
{
  "attributes": {
    "name": "My inventory view 2"
  }
}

// Delete one
DELETE kbn:/api/infra/inventory_views/<switch-with-id>
```

## 👣 Next steps
- Replicate the same logic for the metrics explorer saved object
- Create a client-side abstraction to consume the service
- Update the existing react custom hooks to consume the endpoint

---------

Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
tonyghiani added a commit that referenced this pull request Apr 26, 2023
…ient (#155126)

## 📓 Summary

Depends on #154900
Closes #155110 

This PR implements the `InventoryViewsService` and
`InventoryViewsClient`, injecting an instance of the client in the
KibanaContextForPlugin and exposing so a set of utilities to
retrieve/update inventory views:
- `findInventoryViews`
- `getInventoryView`
- `createInventoryView`
- `updateInventoryView`
- `deleteInventoryView`

## 👣 Next steps
- Implement #154725 to consume the service

---------

Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
tonyghiani added a commit that referenced this pull request Apr 27, 2023
…lorerViewsClient (#155878)

## 📓 Summary

Depends on #154900
Closes #155112  

This PR implements the `InventoryViewsService` and
`InventoryViewsClient`, injecting an instance of the client in the
KibanaContextForPlugin and exposing so a set of utilities to
retrieve/update inventory views:
- `findMetricsExplorerViews`
- `getMetricsExplorerView`
- `createMetricsExplorerView`
- `updateMetricsExplorerView`
- `deleteMetricsExplorerView`

## 👣 Next steps
- Implement #154725 to consume the service

---------

Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co>
Co-authored-by: Carlos Crespo <crespocarlos@users.noreply.github.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 release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Infrastructure UI] Create inventory views service and CRUD endpoints
8 participants