Prepares usage-collection plugin for versioned HTTP APIs #152770

TinaHeiligers · 2023-03-07T00:22:25Z

WIP: for discussion only.

`stats`

The stats route reports metrics from core's Metrics service. However, as-is, these metrics are assumed as unknowns, meaning any subsequent change to what is reported is passed through.
One of the requirements for versioned APIs is to explicitly declare (with appropriate types) the response.

As such, there are a few approaches we could take:

Assume the API response is validated elsewhere (and run the risk of not detecting potential issues for consumers)
Duplicate the types from Core
Move the stats api directly to core's metrics service (or somewhere more appropriate)
something else?

This draft PR creates a copy of the OpsMetrics and explicitly types the response using the copy.

`usage_counters`

There isn't an explicit HTTP API for these, so I'm not sure if versioning the saved object is needed right now.
However, the usage_counters_service reports these metrics through a browser-side API.

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

…ants of reporting usage as empty object

TinaHeiligers · 2023-03-07T02:02:31Z

@elasticmachine merge upstream

…-fix'

kibana-ci · 2023-03-07T02:36:48Z

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: metrics for d16c068 were not reported

History

💔 Build #112273 failed f5f5dc1
💔 Build #112272 failed aae570b
💔 Build #112269 failed 2b045050c59c9c579d413398528d1d83ed2fb549
💔 Build #112268 failed 6068f6a

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

jloleysens

Overall this is looking great @TinaHeiligers ! Happy with the direction to provide full typings for the response as that gives the strongest guarantees for adhering to versioning. I'll finish my review on another pass. Let me know what you think!

jloleysens · 2023-03-07T19:18:14Z

src/plugins/usage_collection/common/types/stats/core_metrics_duplicated.ts

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


Overall this makes sense, it is giving us the full types of the endpoint and we will be able to detect when our response changes👌🏻

Nit: I would name this file "core_metrics" then add a doc comment explaining this starts as a duplicate of the core ops metrics.

jloleysens · 2023-03-07T19:18:48Z

src/plugins/usage_collection/public/services/create_reporter.ts

      // eslint-disable-next-line @typescript-eslint/no-explicit-any
      const response = await fetch.post<any>('/api/ui_counters/_report', {
        body: JSON.stringify({ report }),
        asSystemRequest: true,
      });
-
-      if (response.status !== 'ok') {
+      const okStatus: v1.UiCountersResponseOk = response.status;


jloleysens · 2023-03-07T19:21:18Z

src/plugins/usage_collection/server/report/schema.ts

@@ -43,5 +43,6 @@ export const reportSchema = schema.object({
  application_usage: schema.maybe(schema.recordOf(schema.string(), applicationUsageReportSchema)),
 });

+// should the schema types also be versioned explicitly?


Good question, for now I think it is OK not to. But it would be consistent with our types. I could go either way on this one 😁

or now I think it is OK not to

Cool! I'm going with that :-)

TinaHeiligers · 2023-03-08T00:23:22Z

Closing in favor of #152875

TinaHeiligers added 9 commits March 5, 2023 16:02

Prepares usage-collection plugin for versioned HTTP APIs

3fae351

Duplicates OpsMetrics in usage_collection stats types, cleans up remn…

8323d20

…ants of reporting usage as empty object

Versions ui_counters API:

9fd1891

WIP: ui_counters

04b4f40

Creates new types for ui_counters ok and fail response

86b6ef8

Explicitly type ui_counter saved objects mapping

5ac0a19

Explcitly type CounterMetric

bf5ed2b

Update createReporter fetch types

677eb64

Adds code comments to share for discussing

6068f6a

TinaHeiligers requested a review from jloleysens March 7, 2023 00:22

update type imports

aae570b

TinaHeiligers force-pushed the usage-collection-explicitly-type-all-apis branch from 2b04505 to aae570b Compare March 7, 2023 01:51

kibanamachine added 2 commits March 6, 2023 21:02

Merge branch 'main' into usage-collection-explicitly-type-all-apis

f5f5dc1

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

d16c068

…-fix'

jloleysens reviewed Mar 7, 2023

View reviewed changes

TinaHeiligers closed this Mar 8, 2023

TinaHeiligers deleted the usage-collection-explicitly-type-all-apis branch March 8, 2023 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepares usage-collection plugin for versioned HTTP APIs #152770

Prepares usage-collection plugin for versioned HTTP APIs #152770

TinaHeiligers commented Mar 7, 2023 •

edited

Loading

TinaHeiligers commented Mar 7, 2023

kibana-ci commented Mar 7, 2023 •

edited

Loading

jloleysens left a comment

jloleysens Mar 7, 2023

jloleysens Mar 7, 2023

jloleysens Mar 7, 2023

TinaHeiligers Mar 7, 2023

TinaHeiligers commented Mar 8, 2023

Prepares usage-collection plugin for versioned HTTP APIs #152770

Prepares usage-collection plugin for versioned HTTP APIs #152770

Conversation

TinaHeiligers commented Mar 7, 2023 • edited Loading

stats

usage_counters

Summary

Checklist

Risk Matrix

For maintainers

TinaHeiligers commented Mar 7, 2023

kibana-ci commented Mar 7, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Metrics [docs]

History

jloleysens left a comment

Choose a reason for hiding this comment

jloleysens Mar 7, 2023

Choose a reason for hiding this comment

jloleysens Mar 7, 2023

Choose a reason for hiding this comment

jloleysens Mar 7, 2023

Choose a reason for hiding this comment

TinaHeiligers Mar 7, 2023

Choose a reason for hiding this comment

TinaHeiligers commented Mar 8, 2023

TinaHeiligers commented Mar 7, 2023 •

edited

Loading

`stats`

`usage_counters`

kibana-ci commented Mar 7, 2023 •

edited

Loading