[Infrastructure UI] Implement Metrics explorer views CRUD endpoints #155621

tonyghiani · 2023-04-24T13:14:42Z

📓 Summary

This PR implements the CRUD endpoints for the metrics explorer views.
Following the approach used for the InventoryView service, it exposes a client that abstracts all the logic concerned to the metrics-explorer-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/metrics_explorer_views

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

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

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

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

apmmachine · 2023-04-24T13:14:57Z

🤖 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!)

…-fix'

elasticmachine · 2023-04-25T07:15:33Z

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

tonyghiani · 2023-04-25T13:11:42Z

@crespocarlos as this is an implementation mirroring the one done for the inventory-view saved object, you can find contextual comments useful for the review in the related PR.

crespocarlos

Great job! The CRUD works as expected. Loved how simple and easy to understand the endpoints are.

I just left a few minor comments and I missed 2 possible additional test cases that could be nice to have.

crespocarlos · 2023-04-25T13:07:09Z

x-pack/plugins/infra/common/http_api/metrics_explorer_views/v1/update_metrics_explorer_view.ts

@@ -0,0 +1,29 @@
+/*


nit: This is similar to create_metrics_explorer_view.ts. I'm wondering if we could have types like create_or_update_explorer_view.ts types instead.

I agree they are the same, but since the request payloads don't strictly represent the same structure in future, I preferred to keep them split for isolated responsibilities.

crespocarlos · 2023-04-25T13:12:08Z

x-pack/plugins/infra/server/routes/metrics_explorer_views/create_metrics_explorer_view.ts

+    },
+    async (_requestContext, request, response) => {
+      const { body } = request;
+      const { metricsExplorerViews } = (await getStartServices())[2];


nit: you can completely ignore this. For me, it's easier to understand this:

const [, , { metricsExplorerViews }] = await getStartServices();

crespocarlos · 2023-04-25T13:25:23Z

...k/plugins/infra/server/services/metrics_explorer_views/metrics_explorer_views_client.test.ts

+    });
+  });
+
+  describe('.update', () => {


I couldn't find a test here to validate PUT /api/infra/metrics_explorer_views/0 and DELETE /api/infra/metrics_explorer_views/0 to check the message when metrics explorer view with id is "0". Do you think it's worth it to have a test case for this?

These tests don't validate that piece of logic because the gate for the accepted values is at the endpoint level, when the parameters are validated, it shouldn't even reach the client if the value is not acceptable.

ah. indeed.

crespocarlos · 2023-04-25T13:28:03Z

x-pack/plugins/infra/server/services/metrics_explorer_views/metrics_explorer_views_client.ts

+  public async find(query: MetricsExplorerViewRequestQuery): Promise<MetricsExplorerView[]> {
+    this.logger.debug('Trying to load metrics explorer views ...');
+
+    const sourceId = query.sourceId ?? 'default';


default is repeated in the get and update functions. We could create a constant for this, wdyt?

crespocarlos · 2023-04-25T13:30:28Z

x-pack/plugins/infra/server/services/metrics_explorer_views/metrics_explorer_views_client.ts

+
+    if (defaultViewPosition !== -1) {
+      const element = views.splice(defaultViewPosition, 1)[0];
+      views.unshift(element);


suggestion: We could create a new array instead of mutating the views array passed as a parameter.

It would make sense for immutability purposes, but as we already modified the original instance with .splice to remove the item, there shouldn't be any issue with this mutation.
I measure the performance impact and it's irrelevant in this case.

crespocarlos · 2023-04-25T13:31:23Z

x-pack/plugins/infra/server/services/metrics_explorer_views/metrics_explorer_views_client.ts

+      this.infraSources.getSourceConfiguration(this.savedObjectsClient, sourceId),
+      this.savedObjectsClient.find({
+        type: metricsExplorerViewSavedObjectName,
+        perPage: 1000, // Fetch 1 page by default with a max of 1000 results


suggestion:perPage: 1000 is also repeated in assertNameConflict. We could create a constant for the 1000.

I will extract the all views retrieval into a method

crespocarlos · 2023-04-25T13:32:47Z

x-pack/plugins/infra/server/routes/metrics_explorer_views/README.md

+}
+```
+
+## Update one: `PUT /api/infra/metrics_explorer_views/{metricsExplorerViewId}`


Does it make sense to document the 400 returned when PUT /api/infra/metrics_explorer_views/0 is called?

crespocarlos · 2023-04-25T13:36:04Z

x-pack/plugins/infra/server/routes/metrics_explorer_views/README.md

+}
+```
+
+## Delete one: `DELETE /api/infra/metrics_explorer_views/{metricsExplorerViewId}`


Does it make sense to document the 400 returned when DELETE /api/infra/metrics_explorer_views/0 is called?

tonyghiani · 2023-04-25T14:49:31Z

@crespocarlos thanks for the review, I addressed the comments where we could improve the implementation and updated respectively also the inventory views counterpart to keep them in sync.

crespocarlos

LGTM! Thanks for making the changes.

tonyghiani · 2023-04-26T09:55:51Z

@elasticmachine merge upstream

kibana-ci · 2023-04-26T11:30:29Z

💚 Build Succeeded

Buildkite Build
Commit: 30d153c

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`infra`	1365	1371	+6

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
`infra`	43	44	+1

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	+14.0B

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`	11	12	+1

Page load bundle

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

id	before	after	diff
`infra`	96.7KB	97.8KB	+1.1KB

Unknown metric groups

API count

id	before	after	diff
`infra`	46	47	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	17	19	+2
`securitySolution`	399	402	+3
total			+5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	18	20	+2
`securitySolution`	479	482	+3
total			+5

History

💔 Build #123389 failed 0ebb039
💛 Build #123360 was flaky 7709c92
💚 Build #123038 succeeded 67f713d
💚 Build #122800 succeeded 75439cc
💚 Build #122491 succeeded 76c0e77
💔 Build #122442 failed ae7755f

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

Marco Antonio Ghiani added 4 commits April 24, 2023 12:15

feat(infra): add metrics explorer types

e71dbec

feat(infra): add metrics explorer CRUD API

5f4a427

refactor(infra): upadte static view id

bb9cf15

refactor(infra): update saved object attributes

ae7755f

tonyghiani added Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Apr 24, 2023

Marco Antonio Ghiani and others added 3 commits April 24, 2023 15:56

refactor(infra): remove log

0662999

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

5fa35cb

…-fix'

Merge branch 'main' into 155111-create-metrics-explorer-views

76c0e77

tonyghiani marked this pull request as ready for review April 25, 2023 07:15

tonyghiani requested a review from a team as a code owner April 25, 2023 07:15

Merge branch 'main' into 155111-create-metrics-explorer-views

75439cc

tonyghiani added the release_note:skip Skip the PR/issue when compiling release notes label Apr 25, 2023

crespocarlos self-requested a review April 25, 2023 13:04

crespocarlos reviewed Apr 25, 2023

View reviewed changes

Marco Antonio Ghiani added 2 commits April 25, 2023 16:32

refactor(infra): improve destructuring readability

719ba5f

refactor(infra): requests changes

0636edd

Merge branch 'main' into 155111-create-metrics-explorer-views

67f713d

crespocarlos approved these changes Apr 25, 2023

View reviewed changes

tonyghiani added 3 commits April 26, 2023 08:53

Merge branch 'main' into 155111-create-metrics-explorer-views

5d5dd50

Merge branch 'main' into 155111-create-metrics-explorer-views

7709c92

Merge branch 'main' into 155111-create-metrics-explorer-views

0ebb039

kibanamachine and others added 2 commits April 26, 2023 05:55

Merge branch 'main' into 155111-create-metrics-explorer-views

e93bf56

Merge branch 'main' into 155111-create-metrics-explorer-views

30d153c

tonyghiani merged commit 61bb52c into elastic:main Apr 26, 2023

kibanamachine added the v8.8.0 label Apr 26, 2023

tonyghiani deleted the 155111-create-metrics-explorer-views branch April 26, 2023 12:24

kibanamachine added the backport:skip This commit does not require backporting label Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Infrastructure UI] Implement Metrics explorer views CRUD endpoints #155621

[Infrastructure UI] Implement Metrics explorer views CRUD endpoints #155621

tonyghiani commented Apr 24, 2023

apmmachine commented Apr 24, 2023

elasticmachine commented Apr 25, 2023

tonyghiani commented Apr 25, 2023

crespocarlos left a comment

crespocarlos Apr 25, 2023

tonyghiani Apr 25, 2023

crespocarlos Apr 25, 2023

crespocarlos Apr 25, 2023

tonyghiani Apr 25, 2023

crespocarlos Apr 25, 2023

crespocarlos Apr 25, 2023

crespocarlos Apr 25, 2023

tonyghiani Apr 25, 2023

crespocarlos Apr 25, 2023

tonyghiani Apr 25, 2023

crespocarlos Apr 25, 2023

crespocarlos Apr 25, 2023

tonyghiani commented Apr 25, 2023

crespocarlos left a comment

tonyghiani commented Apr 26, 2023

kibana-ci commented Apr 26, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

[Infrastructure UI] Implement Metrics explorer views CRUD endpoints #155621

[Infrastructure UI] Implement Metrics explorer views CRUD endpoints #155621

Conversation

tonyghiani commented Apr 24, 2023

📓 Summary

🤓 Tips for the reviewer

apmmachine commented Apr 24, 2023

🤖 GitHub comments

elasticmachine commented Apr 25, 2023

tonyghiani commented Apr 25, 2023

crespocarlos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyghiani commented Apr 25, 2023

crespocarlos left a comment

Choose a reason for hiding this comment

tonyghiani commented Apr 26, 2023

kibana-ci commented Apr 26, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History