Disable checking for conflicts when copying saved objects #83575

jportner · 2020-11-17T18:50:15Z

Resolves #81907 and #82324

Overview

This PR primarily makes two changes:

~~Import and~~ Copy operations now don't check for conflicts by default (changed createNewCopies option to be enabled by default)
We now collect telemetry data on Import, Export, and Copy API calls. This is so we can determine if end users still rely on disabling createNewCopies.

⚠️ This PR is best reviewed on a per-commit basis ⚠️

The changes are split into four commits: two for Import (includes changes to Core), and two for Copy (includes changes to Spaces plugin).

Core and Spaces each now have a telemetry service and telemetry client which are only intended to be used internally, not exposed to external consumers. Whenever an external API call is made, the route handler increments a counter in using the telemetry client -- this is persisted to a "telemetry data" saved object. Whenever the usage collector runs periodically, it queries this telemetry data in addition to its normal behavior which checks config/etc.

Testing:

First, run ~~yarn kbn clean && yarn kbn bootstrap~~ tsc -b --clean tsconfig.refs.json && yarn kbn bootstrap, this is required because of changes to the usage collector package.
After logging into Kibana, you can view what usage data is collected by navigating to Stack Management -> Advanced Settings, scrolling to the bottom, and clicking the "cluster data" link. You will see a flyout that describes all up-to-date usage data in Kibana that would be sent to Elastic during the next usage collection interval. First, Import or Copy a saved object, then clicking the "cluster data" link again, Ctrl+f and search for "apiCalls". You will see the appropriate data recorded.

Docs preview: https://kibana_83575.docs-preview.app.elstc.co/diff

Dev Docs

When copying saved objects, conflict checking is now disabled by default. See the createNewCopies parameter in the Copy saved objects to space API documentation for more information.

jportner

Edit: see #83575 (review).

jportner · 2020-11-19T20:25:52Z

Heads up: due to another PR merging to master recently, there are a significant number of merge conflicts. Since it looks like nobody has started reviewing this yet, I'm going to rebase and force-push here soon to fix these cleanly.

This is data that is recorded over time as Spaces APIs are used. It is collected with the rest of the Spaces usage data.

Made this change on the client side and the server side.

This is data that is recorded over time as Core APIs are used. It is collected with the rest of the Core usage data.

Made this change on the client side and the server side.

jportner

Re-post of author's notes for reviewers.

I am aware that there are some gaps in testing (we don't have integration tests and functional tests for every permutation with createNewCopies enabled). I opted to update the existing integration tests and functional tests to retain their original assertions. In my opinion, we should keep it this way until we opt to remove pseudo-copy and get rid of the createNewCopies option completely. When we do that, it'll make more sense to update all of the integration tests and functional tests.

jportner · 2020-11-19T21:22:18Z

docs/api/spaces-management/copy_saved_objects.asciidoc

+`createNewCopies`::
+  (Optional, boolean) Creates new copies of saved objects, regenerates each object ID, and resets the origin. When used, potential conflict
+  errors are avoided. The default value is `true`.
+
+NOTE: This cannot be used with the `overwrite` option.


Somehow I forgot to document this when I added this option 🙈 Added it now!

jportner · 2020-11-19T21:23:34Z

src/core/server/core_telemetry/core_telemetry_service.ts

+    const registerTypeMappings = (typeRegistry: SavedObjectTypeRegistry) => {
+      typeRegistry.registerType({
+        name: CORE_TELEMETRY_TYPE,
+        hidden: true,
+        namespaceType: 'agnostic',
+        mappings: CoreTelemetryMappings,
+      });
+    };


We need to register this type to include its mappings in the .kibana index, but we set up this service before the SavedObjectsService. So I exposed a function here to allow the SavedObjectsService to register this type and retain separation of concerns.

jportner · 2020-11-19T21:24:25Z

src/core/server/core_telemetry/types.ts

+export interface CoreTelemetry {
+  apiCalls?: {
+    savedObjectsImport?: {


I thought the attributes for this saved object type should be structured in a flexible manner, so in the future we can:

Record telemetry data for other API calls

Record telemetry data for something besides API calls

jportner · 2020-11-19T21:27:32Z

src/core/server/core_telemetry/core_telemetry_client.ts

+  public async incrementSavedObjectsImport({
+    createNewCopies,
+    overwrite,
+  }: IncrementSavedObjectsImportOptions) {
+    const coreTelemetry = await this.getTelemetryData();


Each of these telemetry client functions has three steps:

Retrieve current telemetry data (or an empty attributes object, if no current telemetry data exists)

Update the attributes, incrementing whatever counter field(s) are necessary

Persist telemetry data to Elasticsearch

There is a race condition if two users call the same API at the same time (either in a single Kibana server, or separate Kibana servers in a HA configuration). However, the impact is minimal, in that edge case we'll simply lose one of the increments. I think that's an acceptable risk.

The alternative is using the document version to ensure the saved object hasn't changed before we update it, and in that scenario, calculate new attributes so we can retry. Seems like overkill to me, though.

jportner · 2020-11-19T21:28:37Z

src/core/server/index.ts

+  CoreConfigUsageData,
+  CoreEnvironmentUsageData,
+  CoreServicesUsageData,
+  CoreTelemetry,


Since CoreUsageData extends CoreTelemetry now, we have to export CoreTelemetry here. However, the CoreTelemetryService is only intended to be used by Core code, so we don't have to export anything else at the src/core/server level.

Edit: see #83575 (comment), I renamed CoreTelemetry to CoreUsageStats.

jportner · 2020-11-19T21:31:43Z

src/core/server/server.ts

+    const coreTelemetrySetup = this.coreTelemetry.setup({
+      savedObjectsStartPromise: this.savedObjectsStartPromise,
+    });


Here is the setup/start process:

~~CoreTelemetryService~~ CoreUsageStatsService setup (depends on SavedObjectsService start)

SavedObjectsService setup (depends on ~~CoreTelemetryService~~ CoreUsageStatsService setup)

CoreUsageDataService setup (depends on ~~CoreTelemetryService~~ CoreUsageStatsService setup)

SavedObjectsService start

CoreUsageDataService start, immediately fetching usage data and querying for any persisted ~~telemetry data~~ usage stats in the process

The ~~CoreTelemetryService~~ CoreUsageStatsService needs a SavedObjectsRepository so it can persist telemetry data to Elasticsearch. I spoke with @pgayvallet and we determined the best way to handle this messy circular dependency is to pass a promise to the ~~CoreTelemetryService~~ CoreUsageStatsService which resolves after the SavedObjectsService is started.

Edit: changed type names based on PR review feedback.

nit: savedObjectsStartPromiseResolver is just a function, rather than an instance of a class, so I would make that clearer with the naming. e.g. resolveSavedObjectsStartPromise

Done in 7ecb815, thanks!

src/plugins/saved_objects_management/public/lib/import_file.ts

jportner · 2020-11-19T21:34:02Z

...bjects_management/public/management_section/objects_table/components/import_mode_control.tsx

-      disabled={createNewCopies}
+      disabled={createNewCopies && !isLegacyFile}


This fixes a bug that I didn't run into until I changed createNewCopies to be enabled by default on the client side.

test/functional/apps/management/_import_objects.ts

thomheymann

Looks great, thanks for all the pointers in your description. ❤️

Verified functionality and that telemetry data is sent through correctly.

Added one suggestion regarding the internal API but feel free to ignore if it's not possible (you've got more context) or if I'm overlooking something.

Appreciate this isn't really part of this PR but maybe we could improve the UI a bit while working on it:

Now that "create new objects with random IDs" is selected by default I would make that the first option so that it's read first when users scan the form
Labels should be the same font size for all fields / field sets ("Select spaces / file" labels are currently much smaller than "copy / import options") even though those are the main action
"Select spaces" should be the first field as that's the main action the user is trying to achieve and the only one they really need to interact with in "copy to space" flyover. Ideally I would also hide the filter options input unless there are more than 5 spaces or so (I don't think the majority of users have that many spaces and it's distracting more than anything when there's only a few spaces in the list)

Import flyover

Copy to space flyover

thomheymann · 2020-11-22T16:44:20Z

src/core/server/server.ts

+    const coreTelemetrySetup = this.coreTelemetry.setup({
+      savedObjectsStartPromise: this.savedObjectsStartPromise,
+    });


nit: savedObjectsStartPromiseResolver is just a function, rather than an instance of a class, so I would make that clearer with the naming. e.g. resolveSavedObjectsStartPromise

thomheymann · 2020-11-22T17:31:49Z

x-pack/plugins/spaces/server/lib/telemetry_client/telemetry_client.ts

+      },
+    };
+    await this.updateTelemetryData(attributes);
+  }


I feel like this class is doing the exact same thing as the CoreTelemetryClient except that it's tailored to the Spaces use case.

Maybe instead of having many of these clients, each one implementing basically the same logic, we could just have one telemetry service in core that every plugin can use to add telemetry data.

As far as I can tell the logic is the following when we want to record usage data:

Register saved object type mappings for each use case / plugin

Retrieve saved object

Increment count (a bit like a redux reducer)

Update saved object

Something like this could encapsulate this logic:

// Plugin setup const update = core.telemetryService.register(name, mappings); // Route handler await update((currentAttributes = defaultAttributes) => nextAttributes);

e.g. for Spaces plugin:

// Plugin setup const updateSpacesUsage = core.telemetryService.register('spaces-telemetry', mappings); // Route handler await updateSpacesUsage((current = CREATE_NEW_COPIES_DEFAULT) => ({ total: current.total + 1, createNewCopies: incrementBooleanCount(current.createNewCopies, createNewCopies), overwrite: incrementBooleanCount(current.overwrite, overwrite), }))

Just thinking out loud here, but this would make it a lot easier to add additional telemetry events. The only thing you'd need to implement and test as a plugin developer would be the reducer function.

I like the idea and it sounds elegant, and I agree this is essentially duplicated code. However, I know we are trying hard to keep the core API surface as small as possible, and I'm not sure we want to add a whole service for something that we don't yet have additional use cases for. I'll defer to @elastic/kibana-core since they would be the owners of said service.

afharo

Telemetry schema changes LGTM!

cc @mindbat

jportner · 2020-11-23T15:10:23Z

Looks great, thanks for all the pointers in your description. ❤️

Verified functionality and that telemetry data is sent through correctly.

Thanks!

Now that "create new objects with random IDs" is selected by default I would make that the first option so that it's read first when users scan the form

I'm glad you mentioned it. I thought about this but I was hesitant to move around the form layout as I thought that might bring more confusion for existing users. @mdefazio do you have any opinion either way?

Labels should be the same font size for all fields / field sets ("Select spaces / file" labels are currently much smaller than "copy / import options") even though those are the main action

Can't change the label size for EuiFormRow ("Select spaces" / "Select a file to import"). We could change the other sub-header labels to be smaller, but that might look weird? Again I'll ask @mdefazio to weigh in.

"Select spaces" should be the first field as that's the main action the user is trying to achieve and the only one they really need to interact with in "copy to space" flyover. Ideally I would also hide the filter options input unless there are more than 5 spaces or so (I don't think the majority of users have that many spaces and it's distracting more than anything when there's only a few spaces in the list)

I have the same concern regarding changing the form layout. @mdefazio

rudolf · 2020-11-30T10:59:20Z

x-pack/plugins/spaces/server/usage_stats/usage_stats_client.ts

+        },
+      },
+    };
+    await this.updateUsageStats(attributes);


getUsageStats() silently swallows any exceptions and returns an empty object. So if there's an exception in getUsageStats we will reset all counters to 0. If we can't get the existing stats it would be better not to do an update operation (thereby losing one increment instead of all previous counters).

This behaviour is present in the other methods too (and the Core service), just leaving one comment for all of them.

Good point, sounds like this will be resolved when using incrementCounter per #83575 (comment)

x-pack/plugins/spaces/server/usage_stats/usage_stats_client.ts

x-pack/plugins/spaces/server/routes/api/external/copy_to_space.ts

src/core/server/core_usage_stats/core_usage_stats_service.ts

src/core/server/core_usage_data/types.ts

pgayvallet

Only reviewed owned code

src/core/server/core_usage_stats/core_usage_stats_client.ts

src/core/server/core_usage_stats/core_usage_stats_service.ts

pgayvallet · 2020-11-30T15:47:40Z

src/core/server/core_usage_stats/types.ts

+export interface CoreUsageStats {
+  apiCalls?: {
+    savedObjectsImport?: {
+      total: number;
+      kibanaRequest: {
+        yes: number;
+        no: number;
+      };


Question: is this yes/no format required (or here for consistency)? As total = yes + no, I feel like one of these fields may be redundant?

src/core/server/saved_objects/routes/export.ts

src/core/server/saved_objects/routes/integration_tests/export.test.ts

src/core/server/server.ts

src/core/server/core_usage_stats/core_usage_stats_client.ts

pgayvallet

LGTM for my part of the review. Thanks for the changes 😄

rudolf · 2020-12-01T12:05:32Z

Somehow I can't comment on the thread...

Question: is this yes/no format required (or here for consistency)? As total = yes + no, I feel like one of these fields may be redundant?

Don't have a lot of experience with this, but I think this could make visualising the data a bit more challenging since Kibana would have to calculate one of the metrics from the other two numbers. It would be possible to use a scripted field to do this kind of math (or something in the telemetry service that changes the data on ingest) but I would err towards duplicating this data.

src/plugins/telemetry/schema/oss_plugins.json

The way these were declared in Typescript caused the generated telemetry schema JSON to be malformed.

rudolf

Added some nits, but otherwise 👍

src/core/server/core_usage_data/core_usage_stats_client.test.ts

Co-authored-by: Rudolf Meijering <skaapgif@gmail.com>

thomheymann

LGTM!

Nice one on the UX improvements! ❤️

)

…84922)

* master: (28 commits) [Actions] fixes bug where severity is auto selected but not applied to the action in PagerDuty (elastic#84891) Only attempt to rollover signals index if version < builtin version (elastic#84982) skip flaky suite (elastic#84978) skip lens rollup tests Add geo containment tracking alert type (elastic#84151) Changed rollup tests to use test user rather than elastic super user. (elastic#79567) skip 'should allow creation of lens xy chart' elastic#84957 [APM] Add log_level/sanitize_field_names config options to Python Agent (elastic#84810) [Maps] geo line source (elastic#76572) [data.search] Move search method inside session service and add tests (elastic#84715) skip lens drag and drop test. elastic#84941 [Ingest Node Pipelines] Integrate painless autocomplete (elastic#84554) [Lens] allow drag and drop reorder on xyChart for y dimension (elastic#84640) [Lens] Fix error when selecting the current field again (elastic#84817) [Metrics UI] Add metadata tab to node details flyout (elastic#84454) [CI] Enables APM collection (elastic#81731) [Workplace Search] Migrate Sources Schema tree (elastic#84847) Disable checking for conflicts when copying saved objects (elastic#83575) [SECURITY_SOLUTION] delete advanced Policy fields when they are empty (elastic#84368) y18n 4.0.0 -> 4.0.1 (elastic#84905) ...

kibanamachine · 2020-12-04T21:35:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 60de78c

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`savedObjectsManagement`	187.6KB	187.8KB	+194.0B

Distributable file count

id	before	after	diff
`default`	43460	43468	+8
`oss`	22744	22747	+3

Page load bundle

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

id	before	after	diff
`spaces`	290.9KB	291.1KB	+225.0B

History

💚 Build #91717 succeeded 60de78c
💚 Build #91589 succeeded 21188ed
💚 Build #91442 succeeded 8cd3433
💔 Build #91429 failed 787ee5e
💚 Build #91152 succeeded 07cb882

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

jportner added release_note:breaking Feature:Saved Objects v8.0.0 v7.11.0 labels Nov 17, 2020

jportner force-pushed the issue-81907-and-82324 branch 3 times, most recently from 496f14a to a72f52a Compare November 18, 2020 14:06

jportner linked an issue Nov 18, 2020 that may be closed by this pull request

Change defaults for Import and Copy APIs to use new "createNewCopies" mode #82324

Closed

jportner force-pushed the issue-81907-and-82324 branch 4 times, most recently from 4e1e791 to 5f1d254 Compare November 19, 2020 14:14

jportner commented Nov 19, 2020

View reviewed changes

jportner marked this pull request as ready for review November 19, 2020 17:04

jportner requested review from a team as code owners November 19, 2020 17:04

jportner added 4 commits November 19, 2020 16:18

Add Spaces telemetry data

40a9f4c

This is data that is recorded over time as Spaces APIs are used. It is collected with the rest of the Spaces usage data.

Disable checking for conflicts by default when copying objects

10998e4

Made this change on the client side and the server side.

Add Core telemetry data

e8ded81

This is data that is recorded over time as Core APIs are used. It is collected with the rest of the Core usage data.

Disable checking for conflicts by default when importing objects

ecabf59

Made this change on the client side and the server side.

jportner force-pushed the issue-81907-and-82324 branch from 5f1d254 to ecabf59 Compare November 19, 2020 21:19

jportner commented Nov 19, 2020

View reviewed changes

thomheymann reviewed Nov 22, 2020

View reviewed changes

afharo approved these changes Nov 23, 2020

View reviewed changes

jportner requested review from a team as code owners November 23, 2020 15:17

jportner requested a review from a team November 23, 2020 15:17

rudolf requested changes Nov 30, 2020

View reviewed changes

Merge branch 'master' into pr/jportner/83575

b9428d3

pgayvallet reviewed Nov 30, 2020

View reviewed changes

Even more PR review feedback

cfcf686

jportner changed the title ~~Disable checking for conflicts when importing and copying saved objects~~ Disable checking for conflicts when copying saved objects Nov 30, 2020

pgayvallet approved these changes Dec 1, 2020

View reviewed changes

jportner added 3 commits December 1, 2020 09:03

Merge branch 'master' into pr/jportner/83575

eb78588

Refactor

69d7369

Use SavedObjectsRepository.incrementCounter()

60c3820

jportner commented Dec 1, 2020

View reviewed changes

src/plugins/telemetry/schema/oss_plugins.json Outdated Show resolved Hide resolved

jportner added 6 commits December 1, 2020 12:50

Merge branch 'master' into pr/jportner/83575

d1124f8

node scripts/check_published_api_changes.js --accept

07cb882

Fix usage stats fields

787ee5e

The way these were declared in Typescript caused the generated telemetry schema JSON to be malformed.

Merge branch 'master' into pr/jportner/83575

33b32bb

node scripts/check_published_api_changes.js --accept

8cd3433

Merge branch 'master' into pr/jportner/83575

21188ed

rudolf approved these changes Dec 3, 2020

View reviewed changes

Apply suggestions from code review

60de78c

Co-authored-by: Rudolf Meijering <skaapgif@gmail.com>

thomheymann approved these changes Dec 3, 2020

View reviewed changes

jportner merged commit c39d14f into elastic:master Dec 3, 2020

jportner deleted the issue-81907-and-82324 branch December 3, 2020 16:08

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

Disable checking for conflicts when copying saved objects (elastic#83575

ceb20a5

)

jportner mentioned this pull request Dec 3, 2020

[7.x] Disable checking for conflicts when copying saved objects (#83575) #84922

Merged

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

Disable checking for conflicts when copying saved objects (#83575) (#…

039dc67

…84922)

jportner added the backported label Dec 3, 2020

jportner added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:breaking labels Dec 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable checking for conflicts when copying saved objects #83575

Disable checking for conflicts when copying saved objects #83575

jportner commented Nov 17, 2020 •

edited

Loading

jportner left a comment •

edited

Loading

jportner commented Nov 19, 2020

jportner left a comment

jportner Nov 19, 2020

jportner Nov 19, 2020

jportner Nov 19, 2020

jportner Nov 19, 2020

jportner Nov 19, 2020 •

edited

Loading

jportner Nov 19, 2020 •

edited

Loading

thomheymann Nov 22, 2020

jportner Nov 23, 2020

jportner Nov 19, 2020

thomheymann left a comment •

edited

Loading

thomheymann Nov 22, 2020

thomheymann Nov 22, 2020 •

edited

Loading

jportner Nov 23, 2020

afharo left a comment

jportner commented Nov 23, 2020

rudolf Nov 30, 2020

jportner Nov 30, 2020

pgayvallet left a comment

pgayvallet Nov 30, 2020

pgayvallet left a comment

rudolf commented Dec 1, 2020

rudolf left a comment

thomheymann left a comment

kibanamachine commented Dec 4, 2020

		disabled={createNewCopies}
		disabled={createNewCopies && !isLegacyFile}

Disable checking for conflicts when copying saved objects #83575

Disable checking for conflicts when copying saved objects #83575

Conversation

jportner commented Nov 17, 2020 • edited Loading

Overview

Dev Docs

jportner left a comment • edited Loading

Choose a reason for hiding this comment

jportner commented Nov 19, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

jportner Nov 19, 2020 • edited Loading

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

thomheymann left a comment • edited Loading

Choose a reason for hiding this comment

Import flyover

Copy to space flyover

Choose a reason for hiding this comment

thomheymann Nov 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

jportner commented Nov 23, 2020

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

pgayvallet left a comment

Choose a reason for hiding this comment

rudolf commented Dec 1, 2020

rudolf left a comment

Choose a reason for hiding this comment

thomheymann left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 4, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

Page load bundle

History

jportner commented Nov 17, 2020 •

edited

Loading

jportner left a comment •

edited

Loading

jportner Nov 19, 2020 •

edited

Loading

jportner Nov 19, 2020 •

edited

Loading

thomheymann left a comment •

edited

Loading

thomheymann Nov 22, 2020 •

edited

Loading