Exponentially backs off retry attempts for sending usage data #117955

TinaHeiligers · 2021-11-08T22:27:59Z

Resolves #115221
In some cases, a call to fetch all the usage data for a specific report time period might take a long time and add additional strain to a server already under a high load.

This PR exponential backs off retry fetch and send attempts within a report time window and changes the once-per-minute checks to only check if the next report is due.

In this PR we're changing the behavior of when we update the report date, from after a successful send request to before the first send request is attempted. Optimistically updating the report date before the calls avoids subsequent retries every minute.

This has the risk of ever increasing payloads that might eventually also put a large strain on the browser if any usage collector was previously relying on an at-most 24 hour usage data fetch window.

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

Risk Matrix

Risk	Probability	Severity	Mitigation/Notes
Browser becomes unresponsive;unexpected behavior for large usage data payloads.	Low	High	usage collectors need to implement a time-period restriction on data collection

For maintainers

This was checked for breaking API changes and was labeled appropriately

src/plugins/telemetry/public/services/telemetry_sender.test.ts

TinaHeiligers

self review

TinaHeiligers · 2021-11-08T22:32:35Z

src/plugins/telemetry/public/services/telemetry_sender.ts

@@ -53,13 +53,22 @@ export class TelemetrySender {
    return false;
  };

+  private delayRetrySend = () => {
+    return 60 * (1000 * Math.min(Math.pow(2, this.retryCount), 64)); // 120s, 240s, 480s, 960s, 1920s, 3840s, 3840s, 3840s


This could be written as 60000 * Math.min(.....) but writing it this way makes it easier to read at a glance.

We limit the backoff period to just over once per hour.

TinaHeiligers · 2021-11-08T22:33:44Z

src/plugins/telemetry/public/services/telemetry_sender.ts

  private sendIfDue = async (): Promise<void> => {
-    if (this.isSending || !this.shouldSendReport()) {
+    if (!this.shouldSendReport()) {


Exit early if and only if the next report isn't due to be sent. This backs-off on the once-per minute fetch and send request calls we used to make.

TinaHeiligers · 2021-11-08T22:35:27Z

src/plugins/telemetry/public/services/telemetry_sender.ts

      return;
    }
+    // optimistically update the report date and reset the retry counter for a new time report interval window
+    this.lastReported = `${Date.now()}`;


We have a behavior change here where we're now updating the report date before sending the data vs only after a successful send. We need to do this to ensure we're backing off the retry attempts.

src/plugins/telemetry/public/services/telemetry_sender.ts

pgayvallet · 2021-11-09T07:10:27Z

src/plugins/telemetry/public/services/telemetry_sender.ts

  private sendIfDue = async (): Promise<void> => {
-    if (this.isSending || !this.shouldSendReport()) {


Given the removal of isSending, could we add unit tests where sendIfDue is called multiple time in a row without delay just to be sure that shouldSendReport properly handles it (aka sendUsageData should only be called once)

sendIfDue is private and we’re the only ones calling the method. It will never be called without a delay.

pgayvallet · 2021-11-09T07:11:32Z

src/plugins/telemetry/public/services/telemetry_sender.ts

+        window.setTimeout(this.sendUsageData, this.delayRetrySend());
+      }


NIT: maybe we want to add else { console.log(something) } to at least surface the error in the console logs? I feel like it's better than just trapping the error.

Done: https://github.com/elastic/kibana/pull/117955/files#diff-5a45ad5f1ca947b4b167609152746b1599cf51aac704cd7b181bb73cec63e208R100-R102 with tests updated.
Thanks!

pgayvallet · 2021-11-09T07:17:02Z

src/plugins/telemetry/public/services/telemetry_sender.test.ts

+      const telemetryService = mockTelemetryService();
+      const telemetrySender = new TelemetrySender(telemetryService);
+      telemetrySender['shouldSendReport'] = jest.fn().mockReturnValue(false);
+      telemetrySender['retryCount'] = 0;
+      await telemetrySender['sendIfDue']();


(Thinking out loud, not related to the PR as it was already present) This is a code smell of bad isolation. Proper unit testing should never require to mock the blackboxed component.

e.g shouldSendReport mocking could be replaced with telemetryService.canSendTelemetry mocking (which would be the correct way to test the method's behavior btw). Here, we need to mock some of the TelemetrySender's methods to test the behavior of other methods of the class, which is not unit testing the class.

pgayvallet · 2021-11-09T07:19:06Z

src/plugins/telemetry/public/services/telemetry_sender.ts

+      this.retryCount = this.retryCount + 1;
+      if (this.retryCount < 20) {
+        // exponentially backoff the time between subsequent retries to up to 19 attempts, after which we give up until the next report is due
+        window.setTimeout(this.sendUsageData, this.delayRetrySend());


NIT/optional: delayRetrySend can be extracted as a static function getRetryDelay(retryCount: number)

Excellent suggestion, thank you! It also made the testing much easier.

…ethod

src/plugins/telemetry/public/services/telemetry_sender.test.ts

kibanamachine · 2021-11-10T03:31:43Z

💚 Build Succeeded

Metrics [docs]

Page load bundle

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

id	before	after	diff
`telemetry`	23.9KB	24.2KB	+327.0B

History

💛 Build #5222 was flaky 5ab66fa

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

…c#117955)

kibanamachine · 2021-11-17T16:51:13Z

💚 Backport successful

Status	Branch	Result
✅	8.0
✅	7.16

The backport PRs will be merged automatically after passing CI.

… (#118900) Co-authored-by: Christiane (Tina) Heiligers <christiane.heiligers@elastic.co>

… (#118901) Co-authored-by: Christiane (Tina) Heiligers <christiane.heiligers@elastic.co>

…c#117955)

Exponentially backs off retry attempts for sending usage data

10226eb

TinaHeiligers added backport:skip This commit does not require backporting Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes v8.1.0 labels Nov 8, 2021

TinaHeiligers commented Nov 8, 2021

View reviewed changes

src/plugins/telemetry/public/services/telemetry_sender.test.ts Outdated Show resolved Hide resolved

TinaHeiligers commented Nov 8, 2021

View reviewed changes

Apply suggestions from self code review

5ab66fa

TinaHeiligers marked this pull request as ready for review November 9, 2021 02:31

TinaHeiligers requested a review from a team as a code owner November 9, 2021 02:31

TinaHeiligers requested a review from Bamieh November 9, 2021 02:32

pgayvallet approved these changes Nov 9, 2021

View reviewed changes

TinaHeiligers added 3 commits November 9, 2021 09:22

Merge branch 'main' into telemetry/exponential_sending_retry_attempts

8fdf14b

Logs when we've exceeded the retry counts, makes retry delay static m…

a81be14

…ethod

code formatting

7dc97b4

TinaHeiligers commented Nov 10, 2021

View reviewed changes

src/plugins/telemetry/public/services/telemetry_sender.test.ts Outdated Show resolved Hide resolved

Cleanup

7024db1

TinaHeiligers enabled auto-merge (squash) November 10, 2021 01:36

TinaHeiligers merged commit ad1e2ad into elastic:main Nov 10, 2021

kpatticha pushed a commit to kpatticha/kibana that referenced this pull request Nov 10, 2021

Exponentially backs off retry attempts for sending usage data (elasti…

92d07cb

…c#117955)

fkanout pushed a commit to fkanout/kibana that referenced this pull request Nov 17, 2021

Exponentially backs off retry attempts for sending usage data (elasti…

c671e9f

…c#117955)

TinaHeiligers added v7.16.0 v8.0.0 auto-backport Deprecated: Automatically backport this PR after it's merged and removed backport:skip This commit does not require backporting labels Nov 17, 2021

kibanamachine mentioned this pull request Nov 17, 2021

[8.0] Exponentially backs off retry attempts for sending usage data (#117955) #118900

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 17, 2021

Exponentially backs off retry attempts for sending usage data (elasti…

3be61f0

…c#117955)

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 17, 2021

Exponentially backs off retry attempts for sending usage data (elasti…

847975b

…c#117955)

kibanamachine mentioned this pull request Nov 17, 2021

[7.16] Exponentially backs off retry attempts for sending usage data (#117955) #118901

Merged

kibanamachine added a commit that referenced this pull request Nov 17, 2021

Exponentially backs off retry attempts for sending usage data (#117955)…

caa452d

… (#118900) Co-authored-by: Christiane (Tina) Heiligers <christiane.heiligers@elastic.co>

kibanamachine added a commit that referenced this pull request Nov 17, 2021

Exponentially backs off retry attempts for sending usage data (#117955)…

f5a5a74

… (#118901) Co-authored-by: Christiane (Tina) Heiligers <christiane.heiligers@elastic.co>

rudolf added the bug Fixes for quality problems that affect the customer experience label Nov 18, 2021

Bamieh mentioned this pull request Nov 23, 2021

[Meta][Telemetry] Reduce telemetry footprint #119466

Closed

19 tasks

roeehub pushed a commit to build-security/kibana that referenced this pull request Dec 16, 2021

Exponentially backs off retry attempts for sending usage data (elasti…

a4fe348

…c#117955)

lizozom mentioned this pull request Jan 17, 2022

APM data shows telemetry being called ~30 times a day #123144

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exponentially backs off retry attempts for sending usage data #117955

Exponentially backs off retry attempts for sending usage data #117955

TinaHeiligers commented Nov 8, 2021 •

edited

Loading

TinaHeiligers left a comment

TinaHeiligers Nov 8, 2021 •

edited

Loading

TinaHeiligers Nov 8, 2021

TinaHeiligers Nov 8, 2021

pgayvallet Nov 9, 2021

TinaHeiligers Nov 10, 2021 •

edited

Loading

pgayvallet Nov 9, 2021

TinaHeiligers Nov 10, 2021 •

edited

Loading

pgayvallet Nov 9, 2021

pgayvallet Nov 9, 2021

TinaHeiligers Nov 10, 2021

kibanamachine commented Nov 10, 2021

kibanamachine commented Nov 17, 2021

		private sendIfDue = async (): Promise<void> => {
		if (this.isSending \|\| !this.shouldSendReport()) {

		window.setTimeout(this.sendUsageData, this.delayRetrySend());
		}

Exponentially backs off retry attempts for sending usage data #117955

Exponentially backs off retry attempts for sending usage data #117955

Conversation

TinaHeiligers commented Nov 8, 2021 • edited Loading

Checklist

Risk Matrix

For maintainers

TinaHeiligers left a comment

Choose a reason for hiding this comment

TinaHeiligers Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers Nov 8, 2021

Choose a reason for hiding this comment

TinaHeiligers Nov 8, 2021

Choose a reason for hiding this comment

pgayvallet Nov 9, 2021

Choose a reason for hiding this comment

TinaHeiligers Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

pgayvallet Nov 9, 2021

Choose a reason for hiding this comment

TinaHeiligers Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

pgayvallet Nov 9, 2021

Choose a reason for hiding this comment

pgayvallet Nov 9, 2021

Choose a reason for hiding this comment

TinaHeiligers Nov 10, 2021

Choose a reason for hiding this comment

kibanamachine commented Nov 10, 2021

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

kibanamachine commented Nov 17, 2021

💚 Backport successful

TinaHeiligers commented Nov 8, 2021 •

edited

Loading

TinaHeiligers Nov 8, 2021 •

edited

Loading

TinaHeiligers Nov 10, 2021 •

edited

Loading

TinaHeiligers Nov 10, 2021 •

edited

Loading