[data.search] Use incrementCounter for search telemetry by lukasolson · Pull Request #91230 · elastic/kibana

lukasolson · 2021-02-11T22:32:13Z

Summary

Resolves #89744.

When reporting search telemetry, we query a saved object (type search-telemetry), which, prior to this PR, had successCount, errorCount, and averageDuration, and forward these to the telemetry cluster.

When search requests complete, prior to this PR, we would query the saved object, and update each of these properties in the saved object. This often resulted in many 409 conflict errors, because of race conditions in updating the saved object. In addition, there was a bug in how we calculated the averageDuration.

This PR updates the collection to use incrementCounter, which uses a script to update the counts instead of querying/updating the saved object, which should get rid of the race condition. It also replaces averageDuration in the collected saved object with totalDuration, and uses incrementCounter to increment that as well. Then, when reporting to the telemetry cluster, we query the saved object, and calculate the averageDuration, which is then reported to the telemetry cluster.

Open questions

Since averageDuration was being calculated incorrectly, and since the saved object will now have totalDuration instead, we need to "reset" the values for each of the properties stored in the saved object. How do we accomplish this?

elasticmachine · 2021-02-11T22:32:15Z

Pinging @elastic/kibana-app-services (Team:AppServices)

TinaHeiligers

LGTM from a telemetry POV.
As for

"reset" the values for each of the properties stored in the saved object

You'd need a SO migration for that unless we simply throw the 'old' SO out and recreate the new one after the upgrade. I speak under correction here though, I'm not a SO expert! @Bamieh might be able to give more advice.

src/plugins/data/server/search/collectors/register.ts

TinaHeiligers · 2021-02-11T22:52:07Z

src/plugins/data/server/search/collectors/usage.ts

+    trackSuccess: async (duration: number) => {
+      const repository = await getRepository();
+      await repository.incrementCounter(SAVED_OBJECT_ID, SAVED_OBJECT_ID, [
+        { fieldName: 'successCount' },


IIRC, we only migrate SO between versions id the data from one version to the next needs to be persisted (i.e. end-user facing SO). Since telemetry-related SO is fetched once every 24 hours and indexed and that hasn't changed, it doesn't really matter if we blow the SO object out the water and reinitialize a new one when the cluster upgrades. If you did want to persist the data, then I guess you'd need to add a migration. @Bamieh WDYT?

I agree with tina. if a transformation is not possible of the old data simply using a new saved object id and dropping this one is fine. We can drop the existing data in the same savedobject type via migrations.

const migrate713 = (doc) => { return { ...doc, attributes: {}, }; };

I've updated this PR with a migration as suggested, @Bamieh could you take one more look? Thanks!

Bamieh

LGTM

Bamieh · 2021-02-18T10:04:27Z

@elasticmachine merge upstream

lizozom · 2021-02-18T17:07:55Z

src/plugins/data/server/search/collectors/usage.ts

+        },
+      ]);
+    } catch (e) {
+      if (e.statusCode === 409 && retryCount < MAX_RETRY_COUNT) {


Can you use SavedObjectsErrorHelpers.isConflictError(err) ?

lizozom · 2021-02-18T17:08:40Z

src/plugins/data/server/search/collectors/usage.ts

+        { fieldName: 'errorCount' },
+      ]);
+    } catch (e) {
+      if (e.statusCode === 409 && retryCount < MAX_RETRY_COUNT) {


SavedObjectsErrorHelpers.isConflictError(err)

lizozom

I would use SavedObjectsErrorHelpers.isConflictError(err) to detect conflicts, but otherwise LGTM

kibanamachine · 2021-02-18T19:49:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4868c01

Metrics [docs]

✅ unchanged

History

💔 Build #107743 failed 1487a91
💚 Build #107605 succeeded ecdd770
💚 Build #106953 succeeded 1ced937
💚 Build #106891 succeeded d41dcb1
💚 Build #106322 succeeded cb263e1

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

* [data.search] Use incrementCounter for search telemetry * Update reported type * Retry conflicts * Fix telemetry check * Use saved object migration to drop previous document * Review feedback * Fix import Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

) * [data.search] Use incrementCounter for search telemetry * Update reported type * Retry conflicts * Fix telemetry check * Use saved object migration to drop previous document * Review feedback * Fix import Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: (111 commits) [Logs UI] Replace dependencies in the infra bundle (elastic#91503) [Search Source] Do not request unmapped fields if source filters are provided (elastic#91921) [APM] Kql Search Bar suggests values outside the selected time range (elastic#91918) Refactored component edit policy tests into separate folders and using client integration testing setup (elastic#91657) [Fleet] Don't error on missing package_assets value (elastic#91744) [Lens] Pass used histogram interval to chart (elastic#91370) [Indexpattern management] Use indexPatterns Service instead of savedObjects client (elastic#91839) [Security Solutions] Fixes Cypress tests for indicator match by making the selectors more specific (elastic#91947) [CI] backportrc can skip CI (elastic#91886) Revert "[SOM] fix flaky suites (elastic#91809)" [Fleet] Install Elastic Agent integration by default during setup (elastic#91676) [Fleet] Silently swallow 404 errors when deleting ingest pipelines (elastic#91778) [data.search] Use incrementCounter for search telemetry (elastic#91230) [Fleet] Bootstrap functional test suite (elastic#91898) [Alerts][Docs] Added API documentation for alerts plugin (elastic#91067) Use correct environment in anomaly detection setup link (elastic#91877) [FTSR] Convert to tasks and add jest/api integration suites (elastic#91770) [CI] Build and publish storybooks (elastic#87701) docs: add PHP agent info to docs (elastic#91773) [DOCS] Adds and updates Visualization advanced settings (elastic#91904) ...

lukasolson added 2 commits February 11, 2021 14:58

[data.search] Use incrementCounter for search telemetry

8114137

Update reported type

29d165e

lukasolson added review Feature:Search Querying infrastructure in Kibana Feature:Telemetry v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Feb 11, 2021

lukasolson requested review from Dosant, TinaHeiligers and lizozom February 11, 2021 22:32

lukasolson self-assigned this Feb 11, 2021

lukasolson requested a review from a team as a code owner February 11, 2021 22:32

TinaHeiligers reviewed Feb 11, 2021

View reviewed changes

lukasolson added 4 commits February 11, 2021 16:55

Retry conflicts

85d882e

Fix telemetry check

cb263e1

Merge branch 'master' into telemetry-increment

d41dcb1

Use saved object migration to drop previous document

1ced937

Bamieh approved these changes Feb 18, 2021

View reviewed changes

Merge branch 'master' into telemetry-increment

ecdd770

lizozom reviewed Feb 18, 2021

View reviewed changes

lizozom approved these changes Feb 18, 2021

View reviewed changes

lukasolson added 3 commits February 18, 2021 10:12

Review feedback

1487a91

Merge branch 'master' into telemetry-increment

33cf2ca

Fix import

4868c01

lukasolson merged commit 2408d00 into elastic:master Feb 18, 2021

lukasolson mentioned this pull request Feb 18, 2021

[7.12] [data.search] Use incrementCounter for search telemetry (#91230) #91946

Merged

lukasolson mentioned this pull request Feb 18, 2021

[7.x] [data.search] Use incrementCounter for search telemetry (#91230) #91948

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[data.search] Use incrementCounter for search telemetry#91230

[data.search] Use incrementCounter for search telemetry#91230
lukasolson merged 10 commits intoelastic:masterfrom
lukasolson:telemetry-increment

lukasolson commented Feb 11, 2021 •

edited

Loading

Uh oh!

elasticmachine commented Feb 11, 2021

Uh oh!

TinaHeiligers left a comment

Uh oh!

Uh oh!

TinaHeiligers Feb 11, 2021

Uh oh!

Bamieh Feb 15, 2021

Uh oh!

lukasolson Feb 16, 2021

Uh oh!

Bamieh left a comment

Uh oh!

Bamieh commented Feb 18, 2021

Uh oh!

lizozom Feb 18, 2021

Uh oh!

lizozom Feb 18, 2021

Uh oh!

lizozom left a comment

Uh oh!

kibanamachine commented Feb 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

lukasolson commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Open questions

Uh oh!

elasticmachine commented Feb 11, 2021

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TinaHeiligers Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

Bamieh Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

lukasolson Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

Bamieh left a comment

Choose a reason for hiding this comment

Uh oh!

Bamieh commented Feb 18, 2021

Uh oh!

lizozom Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

lizozom Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Feb 18, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lukasolson commented Feb 11, 2021 •

edited

Loading