Improve Retry Behaviour. #146

f2prateek · 2018-01-11T01:16:06Z

Previously we were only retrying network errors or a 5xx error on an idempotent request (GET, HEAD, OPTIONS, PUT or DELETE).

The latter doesn't really guard/apply to us since we use POST for uploading messages. Our POST requests are idempotent since we send messages with a unique message ID that is guarded by our dedupe layer (https://segment.com/blog/exactly-once-delivery/).

For us, our retry policy should be (as per https://paper.dropbox.com/doc/analytics-foo-library-guidelines-2trBhLKQD4Soz4VatvuGR#:uid=189560039280582553736180&h2=Handling-Network-Errors):

Retry network errors (socket timed out, etc.)
Retry server errors (HTTP 5xx)
Retry HTTP 429.

f2prateek · 2018-01-11T01:18:25Z

hey @vadimdemedes, @stephenmathieson; wanted to get your thoughts on this. I think the behaviour is functionally correct, but not sure on how to test it. https://github.com/softonic/axios-retry doesn't seem to have tests. My initial idea was to test the custom retry function directly (something simple like t.true(retryCondition({ response: { status: 429 }}))) but wasn't sure how to access access a private function in the test.

vadimdemedes · 2018-01-11T11:05:39Z

index.js

@@ -249,4 +252,26 @@ class Analytics {
  }
 }

+var retryCondition = function (error) {


Should be an arrow function + const instead of var.

I also might suggest adding this method to the Analytics class so we can more easily write tests for it.

what's an arrow function?

Updated to be const.

I also might suggest adding this method to the Analytics class so we can more easily write tests for it.

Why would that be easier? This seems like a stateless function that can be tested without depending on the analytics class.

Also per my understanding, adding it to the analytics class will automatically expose this function to the public API.

In order to test it directly, we'd have to export it. Attaching it directly to exports (or module.exports) make it look like part of the public API. If we add it as a method of the class, it will be exposed, but won't look like it's part of the public API.

Consider the following:

// exporting directly exports.somePrivateMethod = () => {} // exporting as part of the class module.exports = class Analytics { track () {} somePrivateMethod () {} }

IMO the first looks like it's exposed for consumption by the end user, while the second just looks like a regular class.

what's an arrow function?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

Thanks! Makes sense about testing, chatted with Roland and he suggested using _ as a prefix to denote it as a private, so I'll do that.

vadimdemedes · 2018-01-11T11:06:00Z

index.js

+  // Retry if rate limited.
+  if (error.response.status === 429) {
+    return true
+  }


This function should return false at the end.

stephenmathieson · 2018-01-11T19:32:18Z

index.js

@@ -249,4 +252,28 @@ class Analytics {
  }
 }

+const retryCondition = function (error) {


const retryCondition = error => { // ... }

thanks! roland just explained what this was to me :)

Roland actually suggested not assigning and using it as a function variable at all, which makes sense to me.

function _retryCondition(error) {

I don't follow, but either way, we need to expose this logic somehow in order to test it.

Roland actually suggested not assigning and using it as a function variable at all, which makes sense to me.

Why not?

To me function _retryCondition(error) { looks cleaner than const _retryCondition = error => {. 🤷‍♂️

f2prateek · 2018-01-11T19:33:31Z

reminder to myself, don't merge without tests!

f2prateek · 2018-01-11T21:14:35Z

Ok, should be ready for review now! Added tests and made some changes from the original approach:

function moved to Analytics class for testing
function named with _ prefix to denote this is not meant to be part of the public API.

@Rowno showed me how I could make the function static too. It wasn't clear if that was fully compatible with what we support today and if it cause issues, so I left it as a simple class function for now.

stephenmathieson · 2018-01-11T21:22:06Z

index.js

@@ -247,6 +250,30 @@ class Analytics {
        done(err)
      })
  }
+
+  _retryCondition (error) {


Might rename this shouldRetry or isErrorRetryable for clarity.

Renamed to isErrorRetryable

I think you might have forgotten to push?

Oops! Pushed now.

stephenmathieson · 2018-01-11T21:23:14Z

test.js

+  t.false(client._retryCondition({ code: 'ECONNABORTED' }))
+
+  t.true(client._retryCondition({ response: { status: 500 } }))
+  t.true(client._retryCondition({ response: { status: 429 } }))


We should add one last test to verify the default return false behavior.

Added one, though it's pretty contrived; we'd never really expect to see and error for a 2xx response.

Previously we were only retrying network errors or a 5xx error on an idempotent request (GET, HEAD, OPTIONS, PUT or DELETE). The latter doesn't really guard/apply to us since we use POST for uploading messages. Our POST requests are effectively idempotent since we send messages with a unique message ID that is guarded by our dedupe layer (https://segment.com/blog/exactly-once-delivery/). For us, our retry policy should be (as per https://paper.dropbox.com/doc/analytics-foo-library-guidelines-2trBhLKQD4Soz4VatvuGR#:uid=189560039280582553736180&h2=Handling-Network-Errors): 1. Retry network errors (socket timed out, etc.) 2. Retry server errors (HTTP 5xx) 3. Retry HTTP 429.

stephenmathieson

LGTM!

f2prateek requested review from vadimdemedes and stephenmathieson January 11, 2018 01:16

vadimdemedes suggested changes Jan 11, 2018

View reviewed changes

stephenmathieson reviewed Jan 11, 2018

View reviewed changes

f2prateek force-pushed the retries branch from edda2af to 258c6db Compare January 11, 2018 21:12

stephenmathieson reviewed Jan 11, 2018

View reviewed changes

f2prateek force-pushed the retries branch 2 times, most recently from c48865a to b4dc6ad Compare January 12, 2018 19:32

f2prateek force-pushed the retries branch from b4dc6ad to 6815724 Compare January 12, 2018 19:33

stephenmathieson approved these changes Jan 12, 2018

View reviewed changes

f2prateek merged commit e1fc2b0 into master Jan 15, 2018

f2prateek deleted the retries branch January 15, 2018 18:50

gane5hvarma mentioned this pull request Jan 9, 2024

[Snyk] Security upgrade serialize-javascript from 6.0.1 to 6.0.2 rudderlabs/rudder-sdk-node#133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Retry Behaviour. #146

Improve Retry Behaviour. #146

f2prateek commented Jan 11, 2018 •

edited

Loading

f2prateek commented Jan 11, 2018

vadimdemedes Jan 11, 2018

stephenmathieson Jan 11, 2018

f2prateek Jan 11, 2018

f2prateek Jan 11, 2018

f2prateek Jan 11, 2018

stephenmathieson Jan 11, 2018

stephenmathieson Jan 11, 2018

f2prateek Jan 11, 2018

vadimdemedes Jan 11, 2018

f2prateek Jan 11, 2018

stephenmathieson Jan 11, 2018

f2prateek Jan 11, 2018

f2prateek Jan 11, 2018

stephenmathieson Jan 11, 2018

vadimdemedes Jan 12, 2018

Rowno Jan 12, 2018

stephenmathieson Jan 12, 2018

f2prateek commented Jan 11, 2018

f2prateek commented Jan 11, 2018 •

edited

Loading

stephenmathieson Jan 11, 2018

f2prateek Jan 11, 2018

stephenmathieson Jan 12, 2018

f2prateek Jan 12, 2018

stephenmathieson Jan 11, 2018

f2prateek Jan 11, 2018

stephenmathieson left a comment

Improve Retry Behaviour. #146

Improve Retry Behaviour. #146

Conversation

f2prateek commented Jan 11, 2018 • edited Loading

f2prateek commented Jan 11, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f2prateek commented Jan 11, 2018

f2prateek commented Jan 11, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenmathieson left a comment

Choose a reason for hiding this comment

f2prateek commented Jan 11, 2018 •

edited

Loading

f2prateek commented Jan 11, 2018 •

edited

Loading