feat(zipkin-transport-http): configurable retry

[petermetz]

Defaults to an exponential backoff with retrying always and forever.
All options are configurable through the constructor of the
HttpLogger class.

Goal: Do not lose traces if there was an intermittent network
connectivity issue in a browser environment for example.

Signed-off-by: Peter Somogyvari peter.somogyvari@accenture.com

[jcchavezs]

Hi @petermetz thanks for this great addition. In terms of the code it looks good to me tho we had a discussion about a similar issue and we went for this path (suggested by @adriancole):

My main request is if there isn't a library that could be used that has resilience mechanics built-in, then users pass that library. this seems like commodity HTTP retry stuff which makes it feel odd we should host and maintain that.

Hence my question would be, does it make sense to allow people to plug https://github.com/jonbern/fetch-retry instead of writing our own?

[petermetz]

@jcchavezs Makes a lot of sense, thank you for the feedback! I swapped out the home grown retry logic with the library from your comment.
The code looks much simpler and yet, it achieves the same thing. Let me know what you think.

[petermetz]

@jcchavezs Also made it fully pluggable here: 027b5f8

[jcchavezs]

@petermetz thanks a lot for this great improvement. The only things that bothers me a lot is that HttpLogger accepts ad-hoc options for fetch-retry, I don't think it is its responsibility and also couples us with fetch-retry, also I think it becomes cumbersome as it gives the feeling that you could just pass the retry arguments and it will work with regular fetch. Hence I opened a PR in fetch-retry that allows to pass those options in the builder which will allows us to still accepting a fetch object: jonbern/fetch-retry#39.

[petermetz]

@jcchavezs Got it, makes sense. Let's see how that PR plays out then, I can keep this open in the meantime and update this one accordingly based on what happens to jonbern/fetch-retry#39

[jcchavezs]

@petermetz I think now we can use https://github.com/jonbern/fetch-retry/releases/tag/3.1.0. this will make the code much simpler and I think it is a good idea to add some documentation around the option of plugin a fetch client.

…

On Mon, 17 Feb 2020, 03:19 Peter Somogyvari, ***@***.***> wrote: @jcchavezs <https://github.com/jcchavezs> Got it, makes sense. Let's see how that PR plays out then, I can keep this open in the meantime and update this one accordingly based on what happens to jonbern/fetch-retry#39 <jonbern/fetch-retry#39> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#485?email_source=notifications&email_token=AAXOYAWJKOGJO372Q5QOTRTRDHXZTA5CNFSM4KSWR7NKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL43K2A#issuecomment-586790248>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXOYAWMPDADYWJXLJ7OJK3RDHXZTANCNFSM4KSWR7NA> .

[petermetz]

@jcchavezs Upgraded to 3.1.0 fetch-retry; simplified the constructor parameters as mentioned above and added an extra test + documentation with code examples on how to fully customize the fetch retry implementation:

• a6909d6
• 0122eba
• 102fdd4

[jcchavezs]

Thanks for the great work @petermetz. I really appreciate your patience and enthusiasm.

I changed the PR to use the default fetch by default and added fetch-retry as an example. The main reason for this is that we should aim to provide the simpler/standard in the library but allow users to consciously extend the functionality of this library. Hence I kept the retry usage as an example.

[jcchavezs]

Hi openzipkin! A new version of the package zipkin-transport-http (0.19.3-alpha.1) was published at 2020-02-24T11:02:46.339Z from 34.66.200.49. The shasum of this package was edf8df0c24dfc23decc6567c65a3314d6599016d. If you have questions or security concerns, you can reply to this message or email support@npmjs.com. npm loves you.