Update core usage stats collection #85706

jportner · 2020-12-11T19:33:49Z

Resolves #85621.

Testing

Log into Kibana, navigate to Stack Management -> Advanced Settings, scroll to the bottom, click on the cluster data link, and Ctrl+F for "apiCalls". This will show you the cluster usage data payload, specifically the fields that this PR adds/changes.

Added fields for namespace (default or custom).

First implementation made a best-effort attempt to determine whether or not a request was "first-party" (from the Kibana client) by checking "kbn-version", "origin", and "referer". If these three headers are all present, we thought it was very likely that the request was first-party. However, further testing after adding usage stats collection for additional SO APIs determined this to be inaccurate; some actual first-party requests do not include the "origin" header. I am not sure exactly why this is the case, but I have changed the check to use "user-agent" instead. This is another indicator that a request likely did not come from a programmatic consumer.

jportner

Author's notes for reviewers.

jportner · 2020-12-12T20:13:18Z

src/plugins/kibana_usage_collection/server/collectors/core/core_usage_collector.ts

      'apiCalls.savedObjectsImport.total': { type: 'long' },
-      'apiCalls.savedObjectsImport.kibanaRequest.yes': { type: 'long' },
-      'apiCalls.savedObjectsImport.kibanaRequest.no': { type: 'long' },
+      'apiCalls.savedObjectsImport.namespace.default.total': { type: 'long' },
+      'apiCalls.savedObjectsImport.namespace.default.kibanaRequest.yes': { type: 'long' },
+      'apiCalls.savedObjectsImport.namespace.default.kibanaRequest.no': { type: 'long' },
+      'apiCalls.savedObjectsImport.namespace.custom.total': { type: 'long' },
+      'apiCalls.savedObjectsImport.namespace.custom.kibanaRequest.yes': { type: 'long' },
+      'apiCalls.savedObjectsImport.namespace.custom.kibanaRequest.no': { type: 'long' },


For each existing SO API, I removed these two usage stats fields in favor of these 6 additional ones. This gives us the ability to tell how many consumers, first- or third-party, are using SO APIs in non-default spaces.

src/core/server/core_usage_data/core_usage_stats_client.ts

jportner · 2020-12-12T20:16:02Z

test/api_integration/apis/saved_objects/bulk_create.js

-                  version: 'WzgsMV0=',
+                  version: resp.body.saved_objects[1].version,


Some SO API integration tests already had a "relaxed" version check like I've changed this to.

The usage stats collection additions caused flaky behavior for those tests that have "strict" version checks (version could be slightly different depending upon whether the usage stats incrementCounter finished before the operation in question, I am guessing). At any rate it seemed prudent to change these integration test assertions so that slight version deviations don't cause flaky behavior.

pgayvallet

Looking good. Just got a question regarding the schema / data format

src/core/server/core_usage_data/core_usage_stats_client.ts

pgayvallet · 2020-12-14T09:17:00Z

src/core/server/core_usage_data/core_usage_stats_client.ts

+    const pathToCheck = this.basePath.remove(requestBasePath); // remove the server basePath from the request basePath
+    const matchResult = pathToCheck.match(SPACE_CONTEXT_REGEX); // Look for `/s/space-url-context` in the base path


🙈 , but I don't see any other easy option if we need this to be in core...

I haven't tested this (sorry...not at my laptop at the moment), but could we infer this by checking to see if the requestBasePath === this.basePath.serverBasePath? If they match, then we'd be in the default space. If the request has a modified base path, then we'd be in the non-default space. This wouldn't account for users who explicitly target the default space via /s/default, but I'd expect (with no evidence) that this is uncommon.

Feel free to ignore this, just pitching a potential alternative

I tested it and it does seem to work. I'd be hesitant to use this without accounting for /s/default, though, and we have an explicit check for that it doesn't really simplify our code. At least with the current approach, it's the same in Core and the Spaces plugin, so if it ever needs to change for some reason, it'll be more evident that the same change needs to be made in both places.

I'll leave it as-is for now and if we decide to revisit in #85869 we can do so.

pgayvallet · 2020-12-14T09:18:27Z

src/core/server/core_usage_data/core_usage_stats_client.ts

+export const BULK_CREATE_STATS_PREFIX = 'apiCalls.savedObjectsBulkCreate';
+export const BULK_GET_STATS_PREFIX = 'apiCalls.savedObjectsBulkGet';
+export const BULK_UPDATE_STATS_PREFIX = 'apiCalls.savedObjectsBulkUpdate';
+export const CREATE_STATS_PREFIX = 'apiCalls.savedObjectsCreate';


NIT: I think I'm starting to see a factorizable pattern here 😅

pgayvallet · 2020-12-14T09:22:06Z

src/core/server/core_usage_data/types.ts

+  // Saved Objects Client APIs
+  'apiCalls.savedObjectsBulkCreate.total'?: number;
+  'apiCalls.savedObjectsBulkCreate.namespace.default.total'?: number;
+  'apiCalls.savedObjectsBulkCreate.namespace.default.kibanaRequest.yes'?: number;
+  'apiCalls.savedObjectsBulkCreate.namespace.default.kibanaRequest.no'?: number;
+  'apiCalls.savedObjectsBulkCreate.namespace.custom.total'?: number;
+  'apiCalls.savedObjectsBulkCreate.namespace.custom.kibanaRequest.yes'?: number;


I guess this reflects the ES schema? Don't we want nested properties instead of flattening everything?

Yes this reflects the schema. In the original PR I had nested properties, but I flattened the fields so we could use the incrementCounter API to increment the counter fields instead of get+update. At any rate, the fields are flattened inside of ES so it doesn't make a difference for how we'll consume the usage data 🙂

kibanamachine · 2020-12-14T17:05:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 882d05f

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	47129	47889	+760

History

💚 Build #93829 succeeded 4d9d7f8
💔 Build #93819 failed 673536bcdb9dd82dfc250a9bdb48dbc44ee3b7c8
💔 Build #93818 failed b46f5e641f76296ba483fcc5d9700b66606a9d5a
💚 Build #93800 succeeded 459c2f591c4dc67d7408e89cc9bcfd9932cf66b5
💔 Build #93796 failed e719ab4d59b610f05574386a56829b240af87c57

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

legrego

LGTM, code review only. Let me know if you'd like me to manually test, and I can do so tomorrow

legrego · 2020-12-14T21:03:53Z

src/core/server/core_usage_data/core_usage_stats_client.ts

+    const pathToCheck = this.basePath.remove(requestBasePath); // remove the server basePath from the request basePath
+    const matchResult = pathToCheck.match(SPACE_CONTEXT_REGEX); // Look for `/s/space-url-context` in the base path


I haven't tested this (sorry...not at my laptop at the moment), but could we infer this by checking to see if the requestBasePath === this.basePath.serverBasePath? If they match, then we'd be in the default space. If the request has a modified base path, then we'd be in the non-default space. This wouldn't account for users who explicitly target the default space via /s/default, but I'd expect (with no evidence) that this is uncommon.

Feel free to ignore this, just pitching a potential alternative

jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Dec 11, 2020

jportner force-pushed the issue-85621-saved-object-usage-data branch 2 times, most recently from b46f5e6 to 673536b Compare December 12, 2020 06:01

jportner added 3 commits December 12, 2020 12:54

Update core usage stats collection

9b962d9

Added fields for namespace (default or custom).

Add usage stats for other saved objects APIs

7fcaf5f

jportner force-pushed the issue-85621-saved-object-usage-data branch from 673536b to 4d9d7f8 Compare December 12, 2020 17:54

jportner commented Dec 12, 2020

View reviewed changes

jportner marked this pull request as ready for review December 12, 2020 20:16

jportner requested review from a team as code owners December 12, 2020 20:16

jportner mentioned this pull request Dec 12, 2020

Sharing saved objects, phase 2 #80945

Merged

pgayvallet approved these changes Dec 14, 2020

View reviewed changes

jportner added 2 commits December 14, 2020 09:37

Merge branch 'master' into issue-85621-saved-object-usage-data

58e36d4

PR review feedback

882d05f

legrego approved these changes Dec 14, 2020

View reviewed changes

jportner mentioned this pull request Dec 14, 2020

Improve usage stats collection - first- and third-party requests #85869

Closed

jportner merged commit dd34712 into elastic:master Dec 14, 2020

jportner deleted the issue-85621-saved-object-usage-data branch December 14, 2020 22:12

jportner mentioned this pull request Dec 14, 2020

[7.x] Update core usage stats collection (#85706) #85879

Merged

jportner added a commit to jportner/kibana that referenced this pull request Dec 14, 2020

Update core usage stats collection (elastic#85706)

2ee0933

jportner added the backport pending label Dec 14, 2020

jportner added a commit that referenced this pull request Dec 15, 2020

Update core usage stats collection (#85706) (#85879)

d2d98e8

jportner added backported and removed backport pending labels Dec 15, 2020

rudolf mentioned this pull request Feb 13, 2023

Data views usage collection causes large amount of Elasticsearch queries #151064

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update core usage stats collection #85706

Update core usage stats collection #85706

jportner commented Dec 11, 2020 •

edited

Loading

jportner left a comment

jportner Dec 12, 2020

jportner Dec 12, 2020

pgayvallet left a comment

pgayvallet Dec 14, 2020

jportner Dec 14, 2020

legrego Dec 14, 2020

jportner Dec 14, 2020

pgayvallet Dec 14, 2020

pgayvallet Dec 14, 2020

jportner Dec 14, 2020

kibanamachine commented Dec 14, 2020

legrego left a comment

legrego Dec 14, 2020

		version: 'WzgsMV0=',
		version: resp.body.saved_objects[1].version,

		const pathToCheck = this.basePath.remove(requestBasePath); // remove the server basePath from the request basePath
		const matchResult = pathToCheck.match(SPACE_CONTEXT_REGEX); // Look for `/s/space-url-context` in the base path

Update core usage stats collection #85706

Update core usage stats collection #85706

Conversation

jportner commented Dec 11, 2020 • edited Loading

Testing

jportner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet 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

kibanamachine commented Dec 14, 2020

💚 Build Succeeded

Metrics [docs]

Distributable file count

History

legrego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner commented Dec 11, 2020 •

edited

Loading