feat: Backoff based on HTTP statuses #537

chriswk · 2023-11-15T09:16:57Z

What

This PR adds incremental backoff to both metrics posting as well as feature fetching.
It does so by keeping track of the number of failures we've seen and adding interval + (failureCount * interval) to the next setTimeout call. Each succeeding call of the same type will decrement failure count, reducing the interval respectively.

Discussion points.

I've set maximum failure count to 10 to ensure we never wait forever before the next poll/post of metrics.

…being sent

coveralls · 2023-11-15T09:18:03Z

coverage: 91.222% (-0.2%) from 91.376%
when pulling 75c83e9 on task/obeyHttpStatuses
into 35e0e29 on main.

chriswk · 2023-11-15T09:18:31Z

src/metrics.ts


    if (process.env.NODE_ENV !== 'test' && typeof this.timer.unref === 'function') {
      this.timer.unref();
    }
  }

  start(): void {
-    if (typeof this.metricsInterval === 'number' && this.metricsInterval > 0) {


Typescript compiler stated that this was no longer necessary

src/math.ts

sighphyre · 2023-11-16T08:38:07Z

src/repository/index.ts

@@ -89,6 +95,7 @@ export default class Repository extends EventEmitter implements EventEmitter {
    instanceId,
    projectName,
    refreshInterval = 15_000,
+    maxRefreshInterval = 120_000,


If we expose this in the constructor, it gives consumers the ability to just turn off our backoff. Do we want that? Tempted to use a constant here

It's removed, and replaced by putting a maximum backoff of 10 * interval

sighphyre · 2023-11-16T08:41:56Z

yarn.lock

@@ -812,18 +812,16 @@ acorn@^8.9.0:

 agent-base@6, agent-base@^6.0.2:
  version "6.0.2"
-  resolved "https://registry.npmjs.org/agent-base/-/agent-base-6.0.2.tgz"
+  resolved "https://registry.yarnpkg.com/agent-base/-/agent-base-6.0.2.tgz#49fff58577cfee3f37176feab4c22e00f86d7f77"


The yarn lock update needed or just boyscouting?

Neither is an issue, just curious for context

This happened automatically when I ran yarn install.

sighphyre · 2023-11-16T08:52:15Z

src/repository/index.ts

+    this.failures = max(this.failures - 1, 0);
+    return this.nextFetch();
+  }
+
  async fetch(): Promise<void> {


fetch is now just short of 100 lines. Might be worth breaking this down into smaller methods at this point

agreed. Something like what we did for metrics. I'll have a look.

sighphyre

Looks like it does what it says on the tin, tests seem to cover it nicely too

Suggested some small things but I'll leave the choices up to you

nunogois

LGTM

gastonfournier

A few comments, will continue reviewing

gastonfournier · 2023-11-20T10:37:17Z

src/metrics.ts

+  }
+
+  getInterval(): number {
+    if(this.metricsInterval === 0 || this.failures === -1) {


Should failures be initialized to -1? I can see this working but it makes it hard to reason about when we're checking against -1 here

failures start at 0, if we initialize it to -1 we'd never actually try to post metrics.

But since you're asking, I guess it isn't as clear as I thought it'd be. extract to a method called stopPosting() instead?

gastonfournier · 2023-11-20T10:44:00Z

src/metrics.ts

      if (!res.ok) {
+        if (res.status === 404 || res.status === 403 || res.status == 401) {
+          this.unrecoverableError(url, res.status);


Is it really unrecoverable? If our API is down for a small period of time you may get a 404 response and it may recover later. Or you may get a 403 because your token does not have access to a resource, and then some admin might grant that token permissions. If we make this unrecoverable, they will need a restart.

I remember from the quick discovery/discussion session we had that we thought maybe not stop polling just treat it as a backoff-response, for the reasons @gastonfournier mentions here

For 404:

if your api is down, you should be getting 500 (service error) or a type of 502 or 504 (gateway timeouts), not 404, at least from our hosted instances. I guess there are reverse proxies out there that returns 404 if the service they're proxying is not available.

For 401/403:
Currently, you can't give tokens access after creation, so I'd be very surprised if you went from 403 to 200. You either have a client token or not. A client token which gets new access would simply see more features returned, not suddenly move from a FORBIDDEN to an OK response.

But doing what the Java SDK did instead, immediately log at error level that the API most likely is incorrectly configured and then back off to our maximum backoff (10 * interval), could also be an answer

Ok, that makes sense! I'm still a bit worried we stop posting and we will not notice what's wrong. If we backoff might mean we get a request every now and then but not hammering our servers

Also, for metrics is ok to be more aggressive and stop posting

renamed the method to configurationError after discussion with Gaston

gastonfournier · 2023-11-20T10:45:23Z

src/metrics.ts

@@ -170,6 +190,19 @@ export default class Metrics extends EventEmitter {
    return true;
  }

+  unrecoverableError(url: string, statusCode: number) {
+    this.emit(UnleashEvents.Warn, `${url} returning ${statusCode}, stopping metrics`);
+    this.failures = -1;


Suggested change

this.failures = -1;

this.metricsInterval = 0;

wouldn't this be enough? Just to avoid dealing with failures = -1 which is a bit confusing IMO

I can see the benefit in having a value indicate unrecoverable error, but I think it should in that case be a separate variable?

hmm, we're calling an explicit this.stop() here already, so dropping the -1 seems fair. Will fix.

src/metrics.ts

daveleek

Looks good! I noticed the implementations are a little bit different between the metrics and the features pollings. Is that intentional?

chriswk · 2023-11-20T13:32:52Z

Looks good! I noticed the implementations are a little bit different between the metrics and the features pollings. Is that intentional?

Yes, the metrics is a POST action, whereas the polling is a GET. We're also more OK with losing metrics than not being able to get latest feature configuration.

gastonfournier

LGTM!

nunogois

I noticed some approvals got stale after recent changes, so here's my approval in case it unblocks you 👍

chriswk added 5 commits November 14, 2023 16:23

feat: Start work on obeying http statuses

5060750

Remove last nock.cleanAll()

39e51be

chore: clean up/remove console.log usage

30c9dff

Removed a confusing assertion, and made sure we actually test events …

374c44d

…being sent

Added planned assertions

1e598c6

chriswk requested review from ivarconr, daveleek, Tymek and sighphyre November 15, 2023 09:16

chriswk self-assigned this Nov 15, 2023

chriswk commented Nov 15, 2023

View reviewed changes

fix: lints

204e19b

sighphyre reviewed Nov 16, 2023

View reviewed changes

src/math.ts Outdated Show resolved Hide resolved

sighphyre reviewed Nov 16, 2023

View reviewed changes

sighphyre previously approved these changes Nov 16, 2023

View reviewed changes

refactor: use Math.min and Math.max instead of custom methods

799a51c

chriswk dismissed sighphyre’s stale review via 799a51c November 16, 2023 10:52

chriswk added 2 commits November 16, 2023 13:48

refactor: make error handling into separate methods per type of error

a6df198

fix: remove only from repository test

0549a2e

chriswk requested a review from sighphyre November 16, 2023 13:20

Removed unused maxrefreshinterval, base it on failure count instead

56d5908

chriswk force-pushed the task/obeyHttpStatuses branch from 9f9332d to 56d5908 Compare November 16, 2023 13:23

nunogois previously approved these changes Nov 20, 2023

View reviewed changes

gastonfournier reviewed Nov 20, 2023

View reviewed changes

daveleek previously approved these changes Nov 20, 2023

View reviewed changes

fix: updates after PR discussion

404d610

chriswk dismissed stale reviews from daveleek and nunogois via 404d610 November 20, 2023 14:08

Update assumptions after PR comments

75c83e9

gastonfournier previously approved these changes Nov 20, 2023

View reviewed changes

chriswk dismissed gastonfournier’s stale review via 75c83e9 November 20, 2023 14:13

nunogois approved these changes Nov 20, 2023

View reviewed changes

chriswk merged commit 748d793 into main Nov 21, 2023
4 checks passed

chriswk deleted the task/obeyHttpStatuses branch November 21, 2023 08:46

HenrikBacher mentioned this pull request May 22, 2024

Unleash client stops fetching #615

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Backoff based on HTTP statuses #537

feat: Backoff based on HTTP statuses #537

chriswk commented Nov 15, 2023

coveralls commented Nov 15, 2023 •

edited

Loading

chriswk Nov 15, 2023

sighphyre Nov 16, 2023

chriswk Nov 17, 2023

sighphyre Nov 16, 2023

chriswk Nov 16, 2023 •

edited

Loading

sighphyre Nov 16, 2023

chriswk Nov 16, 2023

sighphyre left a comment

nunogois left a comment

gastonfournier left a comment

gastonfournier Nov 20, 2023

chriswk Nov 20, 2023

chriswk Nov 20, 2023

gastonfournier Nov 20, 2023

daveleek Nov 20, 2023

chriswk Nov 20, 2023

chriswk Nov 20, 2023

gastonfournier Nov 20, 2023

gastonfournier Nov 20, 2023

chriswk Nov 21, 2023

gastonfournier Nov 20, 2023

daveleek Nov 20, 2023

chriswk Nov 20, 2023

daveleek left a comment

chriswk commented Nov 20, 2023

gastonfournier left a comment

nunogois left a comment

feat: Backoff based on HTTP statuses #537

feat: Backoff based on HTTP statuses #537

Conversation

chriswk commented Nov 15, 2023

What

Discussion points.

coveralls commented Nov 15, 2023 • 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

chriswk Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

nunogois left a comment

Choose a reason for hiding this comment

gastonfournier 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

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

daveleek left a comment

Choose a reason for hiding this comment

chriswk commented Nov 20, 2023

gastonfournier left a comment

Choose a reason for hiding this comment

nunogois left a comment

Choose a reason for hiding this comment

coveralls commented Nov 15, 2023 •

edited

Loading

chriswk Nov 16, 2023 •

edited

Loading