amplitude / Amplitude-Node

Server-side Node.js SDK for Amplitude
MIT License
67 stars 20 forks source link

Feature: Refined Logic for 400 and 429 errors #28

Closed kelvin-lu closed 4 years ago

kelvin-lu commented 4 years ago

Refactor and remove events that should not be retried:

I really wanted to reuse logic between onEventsError and retryOnLoop, but the assumptions each function make (same/different user + device Ids) make it harder.

kelvin-lu commented 4 years ago

@ManThursday we've taken a stab at refactoring retryEventsOnLoop to break out a retryEventsOnce that, as the name says, sends one request and sends as a response flags/arrays that are actionable. Plus refactoring the tests to make more sense (thank you for that suggestion).

One thought I had is to restructure how events are being passed in and out of the retry buffer - right now, the retry function creates a slice of the main retry buffer each time I try to send the events and using eventCount to keep track of how many events I should try.

I thought I could refactor this to instead splice all the events at the beginning of retryEventsOnLoop and unshift them back in every time the payload gets cut. it might be simpler and reduces concerns of what happens if retryEventsOnLoop manages to get called on the same device + userID, but adds a new concern would be the performance of unshift (which may or may not be a real concern) - thoughts? I wouldn't try to tackle this in this PR, but wanted to throw out the idea