Improve Retry Behaviour. by f2prateek · Pull Request #146 · segmentio/analytics-node

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

 }
 }

+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 directlyexports.somePrivateMethod=()=>{}// exporting as part of the classmodule.exports=classAnalytics{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

+ // 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

 }
 }

+const retryCondition = function (error) {


constretryCondition=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

 })
 }
+
+ _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

+ 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 · 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

 }
 }

+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 directlyexports.somePrivateMethod=()=>{}// exporting as part of the classmodule.exports=classAnalytics{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

+ // 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

 }
 }

+const retryCondition = function (error) {


constretryCondition=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

 })
 }
+
+ _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

+ 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 · 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

 }
 }

+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 directlyexports.somePrivateMethod=()=>{}// exporting as part of the classmodule.exports=classAnalytics{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

+ // 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

 }
 }

+const retryCondition = function (error) {


constretryCondition=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

 })
 }
+
+ _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

+ 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 · 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

 }
 }

+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 directlyexports.somePrivateMethod=()=>{}// exporting as part of the classmodule.exports=classAnalytics{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

+ // 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

 }
 }

+const retryCondition = function (error) {


constretryCondition=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

 })
 }
+
+ _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

+ 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 · 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

 }
 }

+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 directlyexports.somePrivateMethod=()=>{}// exporting as part of the classmodule.exports=classAnalytics{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

+ // 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

 }
 }

+const retryCondition = function (error) {


constretryCondition=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

 })
 }
+
+ _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

+ 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 · 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

 }
 }

+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 directlyexports.somePrivateMethod=()=>{}// exporting as part of the classmodule.exports=classAnalytics{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

+ // 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

 }
 }

+const retryCondition = function (error) {


constretryCondition=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

 })
 }
+
+ _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

+ 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 · 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

 }
 }

+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 directlyexports.somePrivateMethod=()=>{}// exporting as part of the classmodule.exports=classAnalytics{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

+ // 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

 }
 }

+const retryCondition = function (error) {


constretryCondition=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

 })
 }
+
+ _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

+ 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 · 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

 }
 }

+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 directlyexports.somePrivateMethod=()=>{}// exporting as part of the classmodule.exports=classAnalytics{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

+ // 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

 }
 }

+const retryCondition = function (error) {


constretryCondition=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

 })
 }
+
+ _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

+ 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 stephenmathieson and vadimdemedes 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 258c6dbCompare 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 b4dc6adCompare January 12, 2018 19:32

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

stephenmathieson approved these changes Jan 12, 2018
View reviewed changes

f2prateek merged commit e1fc2b0 into masterJan 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