[Telemetry] Default to `namespaces:['*']` in soClient.find requests #93289

afharo · 2021-03-02T19:11:19Z

Summary

This PR creates a SavedObjectsClient wrapper that is provided in the fetch context's soClient. This wrapper sets namespaces:['*'] to the .find operations so collectors can retrieve all the related SOs, no matter the space.

Resolves #92001.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

afharo

Self-review

afharo · 2021-03-03T09:43:45Z

src/plugins/telemetry/server/telemetry_collection/get_local_stats.test.ts

@@ -14,6 +14,7 @@ import {
  createCollectorFetchContextMock,


NIT: Changes to this file are not actually needed, but I had to go through these because I was having weird bootstrap errors. It turned out it was due to the local bootstrap cache. Happy to revert the changes if anyone thinks they should.

afharo · 2021-03-03T09:46:51Z

src/plugins/telemetry_collection_manager/server/plugin.ts

-    const soClient = config.unencrypted
-      ? this.savedObjectsService?.getScopedClient(config.request)
+    const soRepository = config.unencrypted
+      ? this.savedObjectsService?.createScopedRepository(config.request)


We're now getting the scoped repository instead of the client straight away. This way we can extend both clients: the scoped and internal with the same class TelemetrySavedObjectsClient.

afharo · 2021-03-03T09:59:20Z

Q: Should we backport it to 7.12 as well?

rudolf

Changes look good. Can you add a test to assert that using the saved objects client from the telemetry collection context passes namespaces: [*] to the repository?

…crypted requests

rudolf

left some nits, but otherwise 👍

rudolf · 2021-03-03T15:34:10Z

src/plugins/telemetry_collection_manager/server/plugin.ts

-    const soClient = config.unencrypted
-      ? this.savedObjectsService?.getScopedClient(config.request)
-      : this.savedObjectsService?.createInternalRepository();
+    const soClient =


nit: this ternary / conditional is quite long, I think the code would be easier to read if we had an if(config.unencrypted) and then configured the es client and soClient for each of the cases.

Thanks for the comment! Can you add that we scope it because this is a returned for a user request.

I think it's also worth adding a comment here to explain why we're returning a TelemetrySavedObjectsClient if it isn't a user request (to make it clear that we're using it to collect telemetry across all spaces)

Good point! I've refactored that part and added a few tests to make sure we are correctly picking the right client based on the unencrypted flag.

…sure expected behaviour

afharo · 2021-03-04T14:43:07Z

@elasticmachine merge upstream

…spaces

TinaHeiligers · 2021-03-04T14:57:22Z

src/plugins/telemetry_collection_manager/server/telemetry_saved_objects_client.ts

+   * Find the SavedObjects matching the search query in all the Spaces by default
+   * @param options
+   */
+  async find<T = unknown>(options: SavedObjectsFindOptions): Promise<SavedObjectsFindResponse<T>> {


I understand the need to create a new find method for telemetry needs but am worried that if every service/plugin creates their own override, we'll end up with a mess.

Would it not be better to enhance the SavedObjectsClient itself rather than implement our own home-rolled version?

To fully understand this comment: are you suggesting we default to namespaces:['*'] in the base SavedObjectsClient instead of creating a Telemetry-specific wrapper?

Not exactly. I'm suggesting we have an enhanced SavedObjectsClient find method (maybe findFromAllSpaces(name TBD)), that everyone can use without home-rolling their own. My comment isn't a direct reflection on the implementation here though, it's more of a 'food for thought' comment.

TinaHeiligers

We should think about the benefits to everyone if the SavedObjectsClient were enhanced rather than home-rolling our own override for the find method.
There are likely other services/plugins that need to ensure their SavedObjectClient's are properly scoped.

TinaHeiligers · 2021-03-04T15:02:54Z

src/plugins/telemetry_collection_manager/server/telemetry_saved_objects_client.ts

+   * @param options
+   */
+  async find<T = unknown>(options: SavedObjectsFindOptions): Promise<SavedObjectsFindResponse<T>> {
+    return super.find({ namespaces: ['*'], ...options });


This call to super makes me nervous.

kibanamachine · 2021-03-04T20:12:34Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #111072 failed c012d6e
💚 Build #111025 succeeded 0c9e41d
💚 Build #110759 succeeded 953aba5
💚 Build #110727 succeeded 1af2e48
💚 Build #110469 succeeded 1cfefed

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

cc @afharo

TinaHeiligers

LGTM

rudolf

Thanks for the additional comments!

rudolf · 2021-03-05T10:56:47Z

Like Tina I'm also a bit nervous about creating and exposing many different flavours of SavedObjectsClient which have slightly different underlying behaviour.

In this case we want collectors to work across all spaces when the request is encrypted, and only on the user's current space if the request isn't encrypted. So I don't really see a more elegant way to achieve that.

kibanamachine · 2021-03-05T11:13:54Z

💔 Backport failed

❌ 7.x: Commit could not be cherrypicked due to conflicts

To backport manually, check out the target branch and run:
node scripts/backport --pr 93289

…lastic#93289) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # api_docs/telemetry_collection_manager.json # api_docs/usage_collection.json

…93289) (#93759) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # api_docs/telemetry_collection_manager.json # api_docs/usage_collection.json

[Telemetry] Default to namespaces:['*'] in soClient.find requests

1cfefed

afharo added Feature:Telemetry v7.13.0 labels Mar 2, 2021

afharo self-assigned this Mar 2, 2021

afharo commented Mar 3, 2021

View reviewed changes

afharo marked this pull request as ready for review March 3, 2021 09:47

afharo requested a review from a team as a code owner March 3, 2021 09:47

afharo added the release_note:skip Skip the PR/issue when compiling release notes label Mar 3, 2021

rudolf requested changes Mar 3, 2021

View reviewed changes

Add unit tests to the TelemetrySavedObjectsClient

1af2e48

afharo requested a review from rudolf March 3, 2021 14:08

Use SO.getScopedClient to be sure that security is applied for unen…

953aba5

…crypted requests

rudolf approved these changes Mar 3, 2021

View reviewed changes

Refactored getClients methods to make them clearer + some tests to en…

0c9e41d

…sure expected behaviour

afharo requested a review from rudolf March 4, 2021 14:42

Merge branch 'master' into telemetry/savedObjectsClient-find-all-name…

c012d6e

…spaces

TinaHeiligers reviewed Mar 4, 2021

View reviewed changes

Update API docs

707c0c2

TinaHeiligers self-requested a review March 5, 2021 01:09

TinaHeiligers approved these changes Mar 5, 2021

View reviewed changes

rudolf approved these changes Mar 5, 2021

View reviewed changes

afharo added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 5, 2021

afharo mentioned this pull request Mar 5, 2021

[discuss] SavedObjects: does it make sense to create a findFromAllSpaces method? #93755

Closed

afharo merged commit 00d448b into elastic:master Mar 5, 2021

afharo deleted the telemetry/savedObjectsClient-find-all-namespaces branch March 5, 2021 11:11

afharo mentioned this pull request Mar 5, 2021

[7.x] [Telemetry] Default to namespaces:['*'] in soClient.find requests (#93289) #93759

Merged

afharo mentioned this pull request Apr 27, 2021

[Telemetry] Remove all usages of any #98338

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Telemetry] Default to `namespaces:['*']` in soClient.find requests #93289

[Telemetry] Default to `namespaces:['*']` in soClient.find requests #93289

afharo commented Mar 2, 2021

afharo left a comment

afharo Mar 3, 2021

afharo Mar 3, 2021

afharo commented Mar 3, 2021

rudolf left a comment

rudolf left a comment

rudolf Mar 3, 2021 •

edited

Loading

afharo Mar 4, 2021

afharo commented Mar 4, 2021

TinaHeiligers Mar 4, 2021

afharo Mar 4, 2021

TinaHeiligers Mar 4, 2021 •

edited

Loading

TinaHeiligers left a comment

TinaHeiligers Mar 4, 2021

kibanamachine commented Mar 4, 2021

TinaHeiligers left a comment

rudolf left a comment

rudolf commented Mar 5, 2021

kibanamachine commented Mar 5, 2021

[Telemetry] Default to namespaces:['*'] in soClient.find requests #93289

[Telemetry] Default to namespaces:['*'] in soClient.find requests #93289

Conversation

afharo commented Mar 2, 2021

Summary

Checklist

For maintainers

afharo left a comment

Choose a reason for hiding this comment

afharo Mar 3, 2021

Choose a reason for hiding this comment

afharo Mar 3, 2021

Choose a reason for hiding this comment

afharo commented Mar 3, 2021

rudolf left a comment

Choose a reason for hiding this comment

rudolf left a comment

Choose a reason for hiding this comment

rudolf Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

afharo Mar 4, 2021

Choose a reason for hiding this comment

afharo commented Mar 4, 2021

TinaHeiligers Mar 4, 2021

Choose a reason for hiding this comment

afharo Mar 4, 2021

Choose a reason for hiding this comment

TinaHeiligers Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

TinaHeiligers Mar 4, 2021

Choose a reason for hiding this comment

kibanamachine commented Mar 4, 2021

💚 Build Succeeded

Metrics [docs]

History

TinaHeiligers left a comment

Choose a reason for hiding this comment

rudolf left a comment

Choose a reason for hiding this comment

rudolf commented Mar 5, 2021

kibanamachine commented Mar 5, 2021

💔 Backport failed

[Telemetry] Default to `namespaces:['*']` in soClient.find requests #93289

[Telemetry] Default to `namespaces:['*']` in soClient.find requests #93289

rudolf Mar 3, 2021 •

edited

Loading

TinaHeiligers Mar 4, 2021 •

edited

Loading