[Usage counters] Relocate counters to a dedicated index. Unify / simplify 'ui' and 'server' counters logic. by gsoldevila · Pull Request #187064 · elastic/kibana

gsoldevila · 2024-06-27T12:26:28Z

Summary

This PR sets the basis for allowing namespaced usage counters.
It relocates usage-counters SO type from .kibana to a dedicated .kibana_usage_counters.

Furthermore, the original SO type is removed, and replaced by 2 separate types:

server-counters
ui-counters

Note that these 2 steps are necessary if we want to leverage namespaces property of the saved objects.
We can't currently update the namespaceType: 'agnostic' without causing a migration.
Thus, these two types will be defined as namespaceType: single.

Up until now, UI counters were stored under a special domainId: uiCounter.
This forced a workaround that consisted in storing appName:eventName in the counterName property of the SO.
Having a dedicated SO type for them allows to store appName as domainId, avoiding the need for a workaround.

gsoldevila · 2024-06-27T14:27:01Z

src/plugins/usage_collection/server/usage_counters/usage_counters_service.ts

The UsageCountersService should ensure that a certain domainId is not registered more than once (line 163)

afharo

Overall looks like a great and much-needed refactor of our counters system. Moving them to a custom index will give us the opportunity to scale. Also, moving UI Counters to actually persist the properties in a 1:1 mapping instead of : splits is another great win.

However, I have some reservations with the shared API dealing with multiple SOs: Moving UI Counters to its own SO type is a great move to decouple UI and Server Counters' codebases. Except that it doesn't really decouple them: both SO types are registered from a common place.

We are storing them separately, but a consumer of the API can't create a counter with the same domainId in the server and the browser (which it's a pain since domainId tends to be the plugin ID in most usages, like dashboards).

I wonder if UI counters should prepend ui: to the domainId so that these clashes won't occur. This could also keep the SO type hidden from the public API.

Alternatively, we could specify source: 'ui' | 'server' to hide the persistence layer (SO type). And, if we need to specify the source during the usage counter getOrCreate, I wonder if it could just be one property in a common SO type, that we can filter by. Crazy use case to back it up:
Count views of dashboards coming from the UI (source: 'ui'), and the API (source: 'server').

WDYT?

afharo · 2024-06-27T15:26:54Z

packages/core/saved-objects/core-saved-objects-server/src/saved_objects_index_pattern.ts

...kibana_usage_collection/server/collectors/ui_counters/register_ui_counters_collector.test.ts

...gins/kibana_usage_collection/server/collectors/ui_counters/register_ui_counters_collector.ts

..._usage_collection/server/collectors/usage_counters/register_usage_counters_collector.test.ts

afharo · 2024-06-27T15:48:38Z

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

Q: why would it fail in a way that retries could help? AFAIK, there's no I/O here.

IMO, it's enough with

const counter = counters.getUsageCounterByDomainId(domainId) ?? counters.createUsageCounter(domainId, UI_COUNTERS_SAVED_OBJECT_TYPE);

I had Java and context switching in my head, you're right.

src/plugins/usage_collection/server/usage_counters/saved_objects.ts

afharo · 2024-06-27T15:59:23Z

src/plugins/usage_collection/server/usage_counters/usage_counter.ts

do we want the fallback here? If we fallback here, we're essentially making no distinction for the default space, aren't we?

In the end I'm not creating any agnostic SO types, so they all need to be stored in a specific namespace.

The idea is that whenever users of the API don't specify a namespace, the default one will be used.

afharo · 2024-06-27T16:06:49Z

src/plugins/usage_collection/server/usage_counters/usage_counters_service.ts

🚨 mind to revert this when you're done testing :)

Oops good catch!

src/plugins/usage_collection/server/usage_counters/usage_counters_service.ts

...ges/core/saved-objects/core-saved-objects-migration-server-internal/src/core/unused_types.ts

src/plugins/usage_collection/server/usage_counters/saved_objects.ts

src/plugins/kibana_usage_collection/server/collectors/common/counters.ts

gsoldevila · 2024-06-28T13:19:07Z

Answering comment

After discussion, we've decided to use a single so type 'counters' and introduce a source: 'server' | 'ui' property in the events.

This way we can better filter for it through the SO client.
This also prevents name clashes in plugins that register both server-side and ui-side counters. A counter will now be "source-agnostic" per se, and the events themselves will specify the source.
Done in 267b263

…fix'

…' Vs 'server'. Address PR feedback.

…fix'

…te --fix'

…kibana_usage_counters`

gsoldevila · 2024-07-02T11:10:11Z

src/core/server/integration_tests/saved_objects/migrations/group2/check_target_mappings.test.ts

      // Check for migration steps present in the logs
      logs = await fs.readFile(logFilePath, 'utf-8');
-      expect(logs).not.toMatch('CREATE_NEW_TARGET');
+      expect(logs).not.toMatch('[.kibana] CREATE_NEW_TARGET');


The test now creates a new index for the new type.
I made the constraint more specific and ensure that a new .kibana is not being created.

gsoldevila · 2024-07-02T11:11:33Z

src/core/server/integration_tests/saved_objects/migrations/group5/dot_kibana_split.test.ts

+
+      expect(Array.isArray(typesMap['.kibana'])).toEqual(true);
+      expect(typesMap['.kibana'].length > 50).toEqual(true);
+      expect(typesMap['.kibana'].includes('action')).toEqual(true);


Checking that the map contains an exhaustive list of mappings is not necessary.
This test is broken everytime anyone adds a new SO type.
I've relaxed the constraint a bit.

gsoldevila · 2024-07-02T11:13:58Z

src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts

+
+    // the deleteByNamespace performs an updateByQuery under the hood.
+    // It targets all SO indices that have namespace-aware types
+    const nsIndices = getIndicesWithNamespaceAwareTypes(setup.savedObjects.getTypeRegistry());


Previously we were running a very costly operation, registering thousands of routes, when 99% of requests target .kibana solely.
The only case where multiple indices were specified in the HTTP request path is with namespace-aware indices.
The test was timing out due to the new SO index, which added a lot more permutations.
Hence, I used the opportunity to simplify it.
The test should now be noticeably ligher/faster.

gsoldevila · 2024-07-02T11:18:21Z

src/plugins/usage_collection/server/usage_counters/saved_objects.ts

 /**
 * Parameters to the `serializeCounterKey` method
- * @internal used in kibana_usage_collectors
+ * @internal bused in kibana_usage_collectors


gsoldevila · 2024-07-02T11:18:39Z

src/plugins/kibana_usage_collection/server/collectors/usage_counters/rollups/rollups.test.ts

+      createMockSavedObjectDoc(
+        moment().subtract(6, 'days'),
+        'doc-id-3',
+        USAGE_COUNTERS_SAVED_OBJECT_TYPE


remove, useless

kibana-ci · 2024-07-02T12:50:28Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: fff9e1c

Failed CI Steps

FTR Configs #8

Test Failures

[job] [logs] FTR Configs #8 / Actions and Triggers app Stack alerts page Loads the page Applies the correct quick filter

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/core-saved-objects-server`	133	134	+1
`usageCollection`	16	17	+1
total			+2

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-server`	561	562	+1
`usageCollection`	55	57	+2
total			+3

History

💔 Build #218801 failed f835bfac0d21c0c7bb8184788f73a10b0e1ad641
💔 Build #218708 failed f5a9287d69e8293a97ae2bf60111bc99ba1e1498
💔 Build #218456 failed 267b263e7b1f58de006482c64e6e7ccf33e75b2a
💔 Build #218230 failed a9576db5210c59d8a0d65112ba9888b1b044bb45

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

elasticmachine · 2024-07-02T14:10:57Z

Pinging @elastic/kibana-core (Team:Core)

afharo

Telemetry LGTM

afharo · 2024-07-02T23:59:29Z

...gins/kibana_usage_collection/server/collectors/ui_counters/register_ui_counters_collector.ts

-  return { dailyEvents };
-}
+const UI: UsageCounters.v1.CounterEventSource = 'ui';
+const UI_COUNTERS_FILTER = `${USAGE_COUNTERS_SAVED_OBJECT_TYPE}.attributes.source: ${UI}`;


Do we need the .attributes bit? I thought the core logic introduced it automatically

We do. the core rewrite is {type}.attributes.{attrPath} => {type}.{attrPath} (as each type's attributes are stored under a property matching their name, for historical reasons)

afharo · 2024-07-03T00:01:52Z

src/plugins/kibana_usage_collection/server/collectors/usage_counters/rollups/rollups.ts


    return await Promise.all(
-      docsToDelete.map(({ id }) => savedObjectsClient.delete(USAGE_COUNTERS_SAVED_OBJECT_TYPE, id))
+      docsToDelete.map(({ id, type }) => savedObjectsClient.delete(type, id))


Q: do we need to specify the namespace when deleting it

Q2: Should we take this opportunity to use bulkDelete?

As discussed on slack: because we're using an unscoped repository, we can (and need to) pass the namespace when calling delete

(and yeah, using bulkDelete would be ideal)

Will update to take namespaces into account.
Unfortunately bulkDelete does not allow for a multi-namespace delete operation, and I believe it relies on the spaces extension.

Addressed on b7a2d54

pgayvallet

Overall LGTM.

Only mandatory change is adapting the delete calls (#187064 (comment))

pgayvallet · 2024-07-03T07:56:50Z

packages/kbn-check-mappings-update-cli/current_fields.json

    "tokenType"
  ],
  "core-usage-stats": [],
+  "counter": [


I wonder if we shouldn't go with something slightly less generic than just counter for the type identifier? core-counters at least? Maybe with an additional modifier if we're planning on having multiple SO types for our counter needs?

WDYT?

I'll update it to usage-counter (it's not a core feature).
We finally have a single type and store both agnostic and namespace-aware under namespaceType: single, using default when no namespace is provided.

Addressed on b7a2d54

pgayvallet · 2024-07-03T08:03:15Z

src/plugins/kibana_usage_collection/server/collectors/common/counters.ts

+      namespaces: ['*'],
+      fields: ['count', 'counterName', 'counterType', 'domainId'],
+      filter,
+      perPage: 1000,


NIT: (I doubt it will ever be relevant, but still) one of the upsidesof the PIT is to release memory preasure by having smaller batches of fetched objects, so we usually want to have smaller batches than 1k (like 100 or so).

pgayvallet · 2024-07-03T08:04:08Z

src/plugins/kibana_usage_collection/server/collectors/common/counters.ts

+            dailyEvents.push(event);
+          }
+        } catch (_) {
+          // swallow error; allows sending successfully transformed objects.


NIT: we don't even want a warning or debug level message here?

pgayvallet · 2024-07-03T08:09:11Z

src/plugins/kibana_usage_collection/server/collectors/common/counters.ts

+    acc[key] = {
+      ...existingCounter,
+      ...counter,
+      total: existingCounter.total + counter.total,


Is there any scenario where the spreading properties of existingCounter and counter will differ?

My question is, could this simply be replaced by:

acc[key].total = acc[key].total + counter.total

? (just minor perf increase, avoiding 2 spreads + an object creation)

The only property we want to systematically override is lastUpdatedAt with the most recent one.
I'll update the code with your perf. enhancement.

pgayvallet · 2024-07-03T08:13:08Z

...gins/kibana_usage_collection/server/collectors/ui_counters/register_ui_counters_collector.ts

-  return { dailyEvents };
-}
+const UI: UsageCounters.v1.CounterEventSource = 'ui';
+const UI_COUNTERS_FILTER = `${USAGE_COUNTERS_SAVED_OBJECT_TYPE}.attributes.source: ${UI}`;


We do. the core rewrite is {type}.attributes.{attrPath} => {type}.{attrPath} (as each type's attributes are stored under a property matching their name, for historical reasons)

pgayvallet · 2024-07-03T08:19:25Z

src/plugins/kibana_usage_collection/server/collectors/usage_counters/rollups/rollups.ts


    return await Promise.all(
-      docsToDelete.map(({ id }) => savedObjectsClient.delete(USAGE_COUNTERS_SAVED_OBJECT_TYPE, id))
+      docsToDelete.map(({ id, type }) => savedObjectsClient.delete(type, id))


As discussed on slack: because we're using an unscoped repository, we can (and need to) pass the namespace when calling delete

(and yeah, using bulkDelete would be ideal)

pgayvallet · 2024-07-03T08:26:22Z

src/plugins/usage_collection/server/plugin.ts

   * Creates and registers a usage counter to collect daily aggregated plugin counter events
   */
-  createUsageCounter: (type: string) => UsageCounter;
+  createUsageCounter: (type: string, source?: 'server' | 'ui') => UsageCounter;


NIT: how much work would it represen to have this additional parameter be mandatory? How many calls to adapt?

In fact this is wrong/outdated.
The counters will not be bound to a source anymore. The source will be specified in the events.

createUsageCounter only reserves a certain 'domainId', but it does not register / persist any counter on that domain. Perhaps it should be named registerUsageCounterDomain instead.

Renaming would involve a bunch of places / owners. I'll remove the 'source' for now, and we can tackle that on a separate PR.

pgayvallet · 2024-07-03T08:51:32Z

src/plugins/usage_collection/server/usage_counters/usage_counter.test.ts

 import * as Rx from 'rxjs';
 import * as rxOp from 'rxjs';


NIT: unrelated to the PR. but 🙃

gsoldevila · 2024-07-03T15:06:41Z

...s/kibana_usage_collection/server/collectors/usage_counters/integration_tests/rollups.test.ts

+          serializeCounterKey({ ...attributes, date: updatedAt, namespace: namespaces?.[0] })
+        )
+        .sort()
+    ).toEqual(RECENT_COUNTERS);


Added an integration test to ensure we delete old ones, across namespaces, but preserve recent ones (even when their keys match those of the old ones).

…fix'

… src/core/server/integration_tests/ci_checks'

pgayvallet · 2024-07-04T07:01:46Z

Took a look at the latest changes - restamping with LGTM

elasticmachine · 2024-07-05T11:19:05Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: f0fe90c

Failed CI Steps

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/core-saved-objects-server`	133	134	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-server`	561	562	+1
`usageCollection`	55	56	+1
total			+2

History

💔 Build #219786 failed 6c7a6cf
💚 Build #219584 succeeded aed12b4
💔 Build #219458 failed c61df3b
💛 Build #219131 was flaky a273eb1

## Summary Follow up to #187064 * Improves the `rollups.test.ts` integration test. * Adds the `count` field to the mappings, so that it can be aggregated.

## Summary Part of #186530 Follow-up of #187064 The goal of this PR is to provide the necessary means to allow implementing the [Counting views](https://docs.google.com/document/d/1W77qoweixcjrq0sEKh_LjIk3j33Xyy9umod9mG9BlOM/edit) part of the _Dashboards++_ initiative. We do this by extending the capabilities of the _usage counters_ APIs: * We support custom retention periods. Currently data is only kept in SO indices for 5 days. Having 90 days worth of counting was required for Dashboards++. * We expose a Search API that will allow retrieving persisted counters. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

gsoldevila added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting labels Jun 27, 2024

elastic deleted a comment from kibana-ci Jun 27, 2024

gsoldevila commented Jun 27, 2024

View reviewed changes

afharo reviewed Jun 27, 2024

View reviewed changes

gsoldevila commented Jun 28, 2024

View reviewed changes

...ges/core/saved-objects/core-saved-objects-migration-server-internal/src/core/unused_types.ts Outdated Show resolved Hide resolved

afharo reviewed Jun 28, 2024

View reviewed changes

src/plugins/usage_collection/server/usage_counters/saved_objects.ts Outdated Show resolved Hide resolved

afharo reviewed Jun 28, 2024

View reviewed changes

src/plugins/kibana_usage_collection/server/collectors/common/counters.ts Outdated Show resolved Hide resolved

gsoldevila and others added 7 commits July 2, 2024 11:20

Relocate usage-counters to dedicated index and split server Vs ui

6f94e8a

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

f1eca22

…fix'

Use a single SO type, introduce 'source' attribute to distinguish 'ui…

b9a828f

…' Vs 'server'. Address PR feedback.

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

c656fb3

…fix'

[CI] Auto-commit changed files from 'node scripts/check_mappings_upda…

1c226c6

…te --fix'

Update tests to take into account new SO type counters and index `.…

6c91c98

…kibana_usage_counters`

Fix typo in test

fff9e1c

gsoldevila force-pushed the kbn-dashboards-plus-plus branch from f835bfa to fff9e1c Compare July 2, 2024 09:20

gsoldevila commented Jul 2, 2024

View reviewed changes

Misc enhancements

a273eb1

gsoldevila marked this pull request as ready for review July 2, 2024 14:10

gsoldevila requested a review from a team as a code owner July 2, 2024 14:10

afharo approved these changes Jul 3, 2024

View reviewed changes

pgayvallet approved these changes Jul 3, 2024

View reviewed changes

Address PR feedback

b7a2d54

gsoldevila commented Jul 3, 2024

View reviewed changes

kibanamachine added 2 commits July 3, 2024 15:14

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

b28d019

…fix'

[CI] Auto-commit changed files from 'node scripts/jest_integration -u…

c61df3b

… src/core/server/integration_tests/ci_checks'

Refactor / fix UTs

aed12b4

gsoldevila added the v8.16.0 label Jul 5, 2024

gsoldevila added 2 commits July 5, 2024 11:44

Small tweaks

6c7a6cf

Merge branch 'main' into kbn-dashboards-plus-plus

f0fe90c

gsoldevila enabled auto-merge (squash) July 5, 2024 09:44

gsoldevila merged commit e524fb6 into elastic:main Jul 5, 2024

gsoldevila mentioned this pull request Jul 5, 2024

[Usage Counters] enhancements #187664

Merged

gsoldevila added a commit that referenced this pull request Jul 5, 2024

[Usage Counters] enhancements (#187664)

0f43e8a

## Summary Follow up to #187064 * Improves the `rollups.test.ts` integration test. * Adds the `count` field to the mappings, so that it can be aggregated.

gsoldevila changed the title ~~Relocate usage-counters to dedicated index and split server Vs ui~~ [Usage counters] Relocate counters to a dedicated index. Unify / simplify 'ui' and 'server' counters logic. Jul 9, 2024

gsoldevila mentioned this pull request Jul 9, 2024

[Usage Counters] Enhancements to the APIs #187665

Merged

gsoldevila mentioned this pull request Jul 17, 2024

[Dashboards++] Usage counters enhancements #186530

Closed

3 tasks

gsoldevila mentioned this pull request Apr 1, 2025

[Upgrade 9.0] Kibana updates elastic/docs-content#991

Merged

Conversation

gsoldevila commented Jun 27, 2024

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afharo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gsoldevila commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Jul 2, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Public APIs missing comments

API count

History

Uh oh!

elasticmachine commented Jul 2, 2024

Uh oh!

afharo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsoldevila Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

gsoldevila commented Jun 28, 2024 •

edited

Loading

pgayvallet left a comment •

edited

Loading

gsoldevila Jul 3, 2024 •

edited

Loading

gsoldevila Jul 3, 2024 •

edited

Loading