Skip to content
This repository has been archived by the owner on Feb 13, 2024. It is now read-only.

Add jitter to retry timeout #284

Closed
Dahaden opened this issue Jun 28, 2021 · 4 comments · Fixed by #285
Closed

Add jitter to retry timeout #284

Dahaden opened this issue Jun 28, 2021 · 4 comments · Fixed by #285
Assignees

Comments

@Dahaden
Copy link
Contributor

Dahaden commented Jun 28, 2021

We would love to be able to add jitter to the retry logic in order to prevent retry storms from the hosts impacted by an outage.

This is something that axiosRetry can support through a custom function but I am wondering what the preferred approach to this is.

The options I see are:

  1. Add an option for jitter like in the @segment/localstorage-retry library,
  2. Add an option for the axiosRetryConfig which we can merge with the defaults and pass into axiosRetry, or
  3. Add an option for axiosRetryInstance like axiosInstance.

My only concern with 3 is that axiosInstance need to be passed into axiosRetry and this may be easy to miss-configure if someone is playing around with this.

@Dahaden
Copy link
Contributor Author

Dahaden commented Jun 28, 2021

Actually I might even go with option 1 (Add option for jitter) as then users cant mistakenly overwrite the retryCondition option

@Dahaden
Copy link
Contributor Author

Dahaden commented Jun 28, 2021

Sorry just noticed that axios retry does add 20% jitter to their delays via exponentialDelay https://github.com/softonic/axios-retry/blob/master/es/index.js#L77-L81

I know it sounds counter intuitive, but I am wondering if this should be random_between(0, exponentialDelay) to give a greater variance for the jitter.
https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/

@pbassut
Copy link
Contributor

pbassut commented Jun 29, 2021

How would users mistakenly override the retryCondition option?

I like option 2 better but at the same time it leaves things too open. Maybe create an "allowance list" of the parameters that can be passed into retryConfig? Maybe that's why you're saying users can mistakenly overwrite things.

Something to keep in mind is that you can pass your own axiosInstance to the constructor. The only issue is that it'll "apply" axiosRetry again. And axiosRetry adds request/response interceptors so I'm thinking that wouldn't look nice. But we can always check if retryCount is 0, and if it is completely skip axiosRetry. Which is essentially the option #3(?)

@nd4p90x nd4p90x added this to To do in analytics-node via automation Jun 29, 2021
@Dahaden
Copy link
Contributor Author

Dahaden commented Jun 30, 2021

Yea possibly being a bit naive there, but mainly thinking if someone replaces the retryCondition thinking that they needed to?

An allowance list sounds like a great idea though!

Huh yea didnt think of wrapping the axiosInstance prior to passing it in either. Checking retryCount === 0 is a great idea, will chuck this in a PR.

Thanks!

Dahaden pushed a commit to Dahaden/analytics-node that referenced this issue Jun 30, 2021
nd4p90x added a commit to Dahaden/analytics-node that referenced this issue Jul 21, 2021
@nd4p90x nd4p90x moved this from To do to In progress in analytics-node Jul 26, 2021
analytics-node automation moved this from In progress to Done Jul 28, 2021
pooyaj added a commit that referenced this issue Jul 28, 2021
#284 Added options for axiosRetryConfig, disable axiosRetry if retryCount is 0
MoumitaM added a commit to rudderlabs/rudder-sdk-node that referenced this issue Jan 4, 2023
* Don't start a timer

When the queue has hit the maximum length don't start a timer. If a
timer exists flush will cancel it.

* Bump extend from 3.0.1 to 3.0.2

Bumps [extend](https://github.com/justmoon/node-extend) from 3.0.1 to 3.0.2.
- [Release notes](https://github.com/justmoon/node-extend/releases)
- [Changelog](https://github.com/justmoon/node-extend/blob/master/CHANGELOG.md)
- [Commits](justmoon/node-extend@v3.0.1...v3.0.2)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump eslint-utils from 1.3.1 to 1.4.2

Bumps [eslint-utils](https://github.com/mysticatea/eslint-utils) from 1.3.1 to 1.4.2.
- [Release notes](https://github.com/mysticatea/eslint-utils/releases)
- [Commits](mysticatea/eslint-utils@v1.3.1...v1.4.2)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump mixin-deep from 1.3.1 to 1.3.2

Bumps [mixin-deep](https://github.com/jonschlinkert/mixin-deep) from 1.3.1 to 1.3.2.
- [Release notes](https://github.com/jonschlinkert/mixin-deep/releases)
- [Commits](jonschlinkert/mixin-deep@1.3.1...1.3.2)

Signed-off-by: dependabot[bot] <support@github.com>

* fix data parameter always undefined on callback for track method

* Fixed a typo to reflect actual code behaviour

* Allow passing axios instance or config in options.

* v3.4.0-beta.2

* Update History.md

* Add missing callback function to a remaining flush call

* removed appended path from host

* allow to configure batch events path from initialization

* Bump elliptic from 6.4.1 to 6.5.3

Bumps [elliptic](https://github.com/indutny/elliptic) from 6.4.1 to 6.5.3.
- [Release notes](https://github.com/indutny/elliptic/releases)
- [Commits](indutny/elliptic@v6.4.1...v6.5.3)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump handlebars from 4.1.2 to 4.7.6

Bumps [handlebars](https://github.com/wycats/handlebars.js) from 4.1.2 to 4.7.6.
- [Release notes](https://github.com/wycats/handlebars.js/releases)
- [Changelog](https://github.com/handlebars-lang/handlebars.js/blob/master/release-notes.md)
- [Commits](handlebars-lang/handlebars.js@v4.1.2...v4.7.6)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump node-fetch from 2.6.0 to 2.6.1

Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
- [Release notes](https://github.com/bitinn/node-fetch/releases)
- [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
- [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)

Signed-off-by: dependabot[bot] <support@github.com>

* Fix for infinite axios-retry on 5xx responses and axios client options leakage

* Fixing a formatting issue with test

* Enabled snyk for my account

* Added additonal unit test to prevent infinite axios retry from coming recurring

* Update history and bump the package

* Bump codecov from 3.5.0 to 3.7.1

Bumps [codecov](https://github.com/codecov/codecov-node) from 3.5.0 to 3.7.1.
- [Release notes](https://github.com/codecov/codecov-node/releases)
- [Commits](https://github.com/codecov/codecov-node/commits/v3.7.1)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump ini from 1.3.5 to 1.3.7

Bumps [ini](https://github.com/isaacs/ini) from 1.3.5 to 1.3.7.
- [Release notes](https://github.com/isaacs/ini/releases)
- [Commits](npm/ini@v1.3.5...v1.3.7)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump axios from 0.19.2 to 0.21.1

Bumps [axios](https://github.com/axios/axios) from 0.19.2 to 0.21.1.
- [Release notes](https://github.com/axios/axios/releases)
- [Changelog](https://github.com/axios/axios/blob/v0.21.1/CHANGELOG.md)
- [Commits](axios/axios@v0.19.2...v0.21.1)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump dot-prop from 4.2.0 to 4.2.1

Bumps [dot-prop](https://github.com/sindresorhus/dot-prop) from 4.2.0 to 4.2.1.
- [Release notes](https://github.com/sindresorhus/dot-prop/releases)
- [Commits](sindresorhus/dot-prop@v4.2.0...v4.2.1)

Signed-off-by: dependabot[bot] <support@github.com>

* update changelog and bump the version

* Bump acorn from 6.1.1 to 6.4.2

Bumps [acorn](https://github.com/acornjs/acorn) from 6.1.1 to 6.4.2.
- [Release notes](https://github.com/acornjs/acorn/releases)
- [Commits](acornjs/acorn@6.1.1...6.4.2)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump https-proxy-agent from 2.2.1 to 2.2.4

Bumps [https-proxy-agent](https://github.com/TooTallNate/node-https-proxy-agent) from 2.2.1 to 2.2.4.
- [Release notes](https://github.com/TooTallNate/node-https-proxy-agent/releases)
- [Commits](TooTallNate/proxy-agents@2.2.1...2.2.4)

Signed-off-by: dependabot[bot] <support@github.com>

* few dependency fixes

* remove size task

* remove more size

* pin yargs-parser

* Add callback to flush binding

* throws error when message is over 32kb

* Fixing conflicts

* releasing a major version

* using axios instance

* calling post method as its more testable

* testing if we can use the custom axios instance

* Release a patch version to fix the issue with axios instance

* upgrade uuid dep to support bundler tree shaking

See https://github.com/uuidjs/uuid/blob/master/README_js.md#deep-requires-now-deprecated

From the above document, the motivation for upgrading:

As of uuid@7.x this library now provides ECMAScript modules builds,
which allow packagers like Webpack and Rollup to do "tree-shaking" to
remove dead code.

* Bump y18n from 3.2.1 to 3.2.2

Bumps [y18n](https://github.com/yargs/y18n) from 3.2.1 to 3.2.2.
- [Release notes](https://github.com/yargs/y18n/releases)
- [Changelog](https://github.com/yargs/y18n/blob/master/CHANGELOG.md)
- [Commits](https://github.com/yargs/y18n/commits)

Signed-off-by: dependabot[bot] <support@github.com>

* flushing when payload max size has reached

* Bump handlebars from 4.7.6 to 4.7.7

Bumps [handlebars](https://github.com/wycats/handlebars.js) from 4.7.6 to 4.7.7.
- [Release notes](https://github.com/wycats/handlebars.js/releases)
- [Changelog](https://github.com/handlebars-lang/handlebars.js/blob/master/release-notes.md)
- [Commits](handlebars-lang/handlebars.js@v4.7.6...v4.7.7)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump lodash from 4.17.20 to 4.17.21

Bumps [lodash](https://github.com/lodash/lodash) from 4.17.20 to 4.17.21.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.20...4.17.21)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump hosted-git-info from 2.7.1 to 2.8.9

Bumps [hosted-git-info](https://github.com/npm/hosted-git-info) from 2.7.1 to 2.8.9.
- [Release notes](https://github.com/npm/hosted-git-info/releases)
- [Changelog](https://github.com/npm/hosted-git-info/blob/v2.8.9/CHANGELOG.md)
- [Commits](npm/hosted-git-info@v2.7.1...v2.8.9)

Signed-off-by: dependabot[bot] <support@github.com>

* bumping up payload size to 500kb

* lock

* returning promise from flush

* sample code to test more easily

* making queue big enough that it would reach queue size limit

* dropping support for node 8

* segmentio#284 Added options for axiosRetryConfig, disable axiosRetry if retryCount is 0

* Remove unnessesary `|| {}`

Co-authored-by: Patrick Bassut <patrickbassut@hotmail.com>

* prep for 5.0.0 release

* v5.0.0

* Bump tar from 4.4.10 to 4.4.15

Bumps [tar](https://github.com/npm/node-tar) from 4.4.10 to 4.4.15.
- [Release notes](https://github.com/npm/node-tar/releases)
- [Changelog](https://github.com/npm/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v4.4.10...v4.4.15)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump path-parse from 1.0.6 to 1.0.7

Bumps [path-parse](https://github.com/jbgutierrez/path-parse) from 1.0.6 to 1.0.7.
- [Release notes](https://github.com/jbgutierrez/path-parse/releases)
- [Commits](https://github.com/jbgutierrez/path-parse/commits/v1.0.7)

---
updated-dependencies:
- dependency-name: path-parse
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update test.js

Co-authored-by: Patrick Bassut <patrickbassut@hotmail.com>

* Bump jszip from 3.2.1 to 3.7.1

Bumps [jszip](https://github.com/Stuk/jszip) from 3.2.1 to 3.7.1.
- [Release notes](https://github.com/Stuk/jszip/releases)
- [Changelog](https://github.com/Stuk/jszip/blob/master/CHANGES.md)
- [Commits](Stuk/jszip@v3.2.1...v3.7.1)

---
updated-dependencies:
- dependency-name: jszip
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump tar from 4.4.15 to 4.4.19

Bumps [tar](https://github.com/npm/node-tar) from 4.4.15 to 4.4.19.
- [Release notes](https://github.com/npm/node-tar/releases)
- [Changelog](https://github.com/npm/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v4.4.15...v4.4.19)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* segmentio#287 Flush now returns promises correctly with the data that callback would have gotten. Removed pify from devDependencies

* Bumped axios version to patch ReDos vulnerability

* update history + bump package.json

* let np takes care of version

* v5.1.0

* update axios retry

* version bump with np

* v5.1.1

* pinning axios-retry to 3.2.0

* v5.1.2

* flush when approaching the limit instead of surpassing the limit

* v5.2.0

* v6.0.0

* callback called twice fixed

* enqueue's flushes await

* undo wrong commit

* donde function updated

* Test fixed

* Update code snippet to match analyics-code API

Following the [documentation example here](https://segment.com/docs/connections/sources/catalog/libraries/server/node/#track) as well as the official `analytics-node` API signature, this code snippet example needs to be updated. The current example is from `analytics.js`, Segment's clientside library.

* Bump ajv from 6.10.0 to 6.12.6

Bumps [ajv](https://github.com/ajv-validator/ajv) from 6.10.0 to 6.12.6.
- [Release notes](https://github.com/ajv-validator/ajv/releases)
- [Commits](ajv-validator/ajv@v6.10.0...v6.12.6)

---
updated-dependencies:
- dependency-name: ajv
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump follow-redirects from 1.14.3 to 1.14.8

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.14.3 to 1.14.8.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.14.3...v1.14.8)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump trim-off-newlines from 1.0.1 to 1.0.3

Bumps [trim-off-newlines](https://github.com/stevemao/trim-off-newlines) from 1.0.1 to 1.0.3.
- [Release notes](https://github.com/stevemao/trim-off-newlines/releases)
- [Commits](stevemao/trim-off-newlines@v1.0.1...v1.0.3)

---
updated-dependencies:
- dependency-name: trim-off-newlines
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump minimist from 1.2.5 to 1.2.6

Bumps [minimist](https://github.com/substack/minimist) from 1.2.5 to 1.2.6.
- [Release notes](https://github.com/substack/minimist/releases)
- [Commits](https://github.com/substack/minimist/compare/1.2.5...1.2.6)

---
updated-dependencies:
- dependency-name: minimist
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump node-fetch from 2.6.1 to 2.6.7

Bumps [node-fetch](https://github.com/node-fetch/node-fetch) from 2.6.1 to 2.6.7.
- [Release notes](https://github.com/node-fetch/node-fetch/releases)
- [Commits](node-fetch/node-fetch@v2.6.1...v2.6.7)

---
updated-dependencies:
- dependency-name: node-fetch
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update library axios to 0.27.2

* Add sentat note to readme

Resolves Issue segmentio#338

* add an errorHandler property to the class initializer such that we can return a value using this error handler in case of axios exception instead of throwing. unit test included.

* Update History.md

* Update History.md

* Update History.md file with new changes

* Update History.md file to 6.1.0

* Update History.md

* v6.1.0

* Ensure callback is called when envoking errorHanlder

A property, errorHanlder, was recently added which will be called
instead of throwing an error in the .flush() method. This is important
because errors in .flush() could, previously, only be handled via
process.on('uncaughtException', err => { ... }).

However, this property is currently unusable as, when the flush method
invokes this property, it fails to call the callbacks of the events
being flushed.

This commit makes sure the callbacks are called.

* v6.2.0

* flush: ensure previous flush completion

* allow recovering from failed flushs

* standard --fix

* fix error handling

* first check for reasons not to flush and then for pending flushes

* feat: added gzip

* test: debugging test cases

* test: fix issues with gzip and 404 responses of dataplane batch endpoint

* chore: turned bull into optional dependency, queue prefix logic updated

* chore: removed unnecessary files

* chore: added dataplaneurl support in optional config

* chore: updated bull package dependency to latest

* ci: added github actions

* chore: eslintcache ignored

* ci: added nvmrc

* ci: updated test workflow

* chore: added example apps, test cases updated

* test: test cases updated

* ci: enabled sonar

* chore: type declaration added for screen API

* chore: added .env for example apps, version bumped

* chore: minor changes

* chore: security vulnerabilities adressed

* chore: cli app updated

* ci: sonar project properties update

* chore: changelog updated

* chore: readme update

* chore: readme update

* chore: readme update

* chore: readme update

* chore: cleanup, changelog update

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Christopher Pardy <chris@rategravity.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Albertinin Mourato <ams11@cin.ufpe.br>
Co-authored-by: Emanuele Libralato <10ko@users.noreply.github.com>
Co-authored-by: Kris Dages <krisdages@git.whiteboxsoftware.net>
Co-authored-by: Pooya Jaferian <pooya.j@gmail.com>
Co-authored-by: ankur <ankurchandraa@gmail.com>
Co-authored-by: David Reichert <dreichert@gmail.com>
Co-authored-by: albertmourato <albertinin.mourato@liferay.com>
Co-authored-by: Shane L. Duvall <shane@northtwofive.com>
Co-authored-by: Patrick Bassut <patrickbassut@hotmail.com>
Co-authored-by: Garrett Wu <wugarrett@gmail.com>
Co-authored-by: Dave <dhaden@atlassian.com>
Co-authored-by: Dahaden <dahaden@gmail.com>
Co-authored-by: Shane L. Duvall <sduvall@mark4tech.com>
Co-authored-by: Felipe Najson <felipe@team.northtwofive.com>
Co-authored-by: Felipe Najson <89416739+felipe-najson-ntf@users.noreply.github.com>
Co-authored-by: Benjamin Hoffman <6520022+benjaminhoffman@users.noreply.github.com>
Co-authored-by: “Edson <edson@alcatrazstudios.com>
Co-authored-by: Tam CARRE <kokone@takat.su>
Co-authored-by: Tim Haley <thaley@atlassian.com>
Co-authored-by: Horacio Peña <horape@gmail.com>
Co-authored-by: Michael Grosse Huelsewiesche <mihuelsewiesche@twilio.com>
Co-authored-by: George Bardis <gbardis@rudderstack.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
2 participants