Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve callback error and modified the entire retry logic #34

Merged
merged 1 commit into from Feb 21, 2018

Conversation

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jan 23, 2018

@mchaudhary @mostlyjason In this PR, I have modified the entire error retry logic and have covered the below points:

  • Retrying on connection errors(Maximum 5 times).
  • Retrying on status code other than 200(Maximum 5 times).
  • Display a message on console when there is any connection or server error occurs and event retry.
  • Display an error message on console when any event failed after retrying 5 times.
  • Display a message on console if the event is sent successfully after retrying.
  • Handled the bad token error i.e. response code 403.
  • Changed coding style to functional programming.
  • Error retrying will be in a increasing time interval i.e. in multiple of 2.
  • Removed all hard coded values/numbers.
  • Testing

Also, I took a reference of retry logic from here: https://github.com/logzio/logzio-nodejs/blob/master/lib/logzio-nodejs.js#L248-L258.

I checked the library behavior after these changes in a continue logging environment for about 8 hours and did not see any callback error. The issue is related to loggly/winston-loggly-bulk#13.

Update: I also tested the library code in both bulk & input mode for 15 days in a continue event logging environment. My node application did not break with any error.

Please review the changes.

isValidToken = false;
return onError((new Error('Loggly Error (' + responseCode + '): ' + httpStatusCode.badToken.message)));
}
if (responseCode === httpStatusCode.success.code && bulk.attemptNumber >= eventRetried) {

This comment has been minimized.

@Shwetajain148

Shwetajain148 Jan 24, 2018
Author

@mchaudhary This condition checks if the event sent successfully after retrying "n" number of times with status code 200 then print a console message.

isBulk ? sendBulkLogs(mode) : sendInputLogs(mode);
}, sleepTimeMs);
}
if (mode.attemptNumber >= numberOfRetries) {

This comment has been minimized.

@Shwetajain148

Shwetajain148 Jan 24, 2018
Author

@mchaudhary This condition checks if the total number of retries has reached the maximum retrying limit then show the failed event message on console.

isValidToken = false;
return onError((new Error('Loggly Error (' + responseCode + '): ' + httpStatusCode.badToken.message)));
}
if (responseCode === httpStatusCode.success.code && input.attemptNumber >= eventRetried) {

This comment has been minimized.

@Shwetajain148

Shwetajain148 Jan 24, 2018
Author

@mchaudhary This condition checks if the event sent successfully after retrying "n" number of times with status code 200 then print a console message.

sendInputLogs(input);
}
}
function createBulk(msgs) {

This comment has been minimized.

@Shwetajain148

Shwetajain148 Jan 24, 2018
Author

@mchaudhary In this function, I am attaching some fields with the event like attempt number, eventId and time to retry in so that later the library can identify which event was failed on which error and in what time this event needs to retry.

@Shwetajain148 Shwetajain148 force-pushed the Shwetajain148:resolve-callback-error branch from fbae04d to b92b702 Jan 24, 2018
@eduard-piliposyan
Copy link

@eduard-piliposyan eduard-piliposyan commented Feb 9, 2018

Any ETA when this will be merged ? We have error Callback was already called. and will be very happy to see this issue fixed.

@wlindley
Copy link

@wlindley wlindley commented Feb 15, 2018

Same here, we'd love to see this merged!

@dmr-loggly
Copy link

@dmr-loggly dmr-loggly commented Feb 15, 2018

Hey everyone, we apologize for the delay in processing this PR. I'm working right now on making sure it's brought to the right people's attention to be fixed ASAP.

@dmr-loggly dmr-loggly self-assigned this Feb 15, 2018
@mchaudhary mchaudhary merged commit 95d89ed into loggly:master Feb 21, 2018
@matthewchng
Copy link

@matthewchng matthewchng commented Mar 5, 2018

@mchaudhary Will there be a patch version bump to release this change? The current version 2.2.1 was updated 3 months ago. Thanks.

node-loggly-bulk version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.