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

Expose the current attempt number to the recoverFromFailure callback #1

Closed
edigaryev opened this issue Apr 12, 2024 · 4 comments
Closed

Comments

@edigaryev
Copy link

This would allow the swift-retry package users to avoid printing extraneous information if this is the last possible retry attempt.

Here's a real-world example: https://github.com/cirruslabs/tart/pull/788/files#diff-52d106b7150fe2a5d777ea88144bbf0a07f23d521a973fa1fd92133ab223fec3R196.

Here's a similar library for Golang that has this feature: https://pkg.go.dev/github.com/avast/retry-go/v4#OnRetryFunc.

P.S. thanks for making such a great package ❤️

@fumoboy007
Copy link
Owner

Hi @edigaryev! If I understood correctly, you are adding logging for the retries. If so, then have you considered passing a logger object to the retry function? The implementation already handles the situation you described.

For example, here are some example log messages when using an os.Logger object:

Attempt 0 failed with error of type `MyError`: `Some error description.`. Will wait 1.0 seconds before retrying.
Attempt 1 failed with error of type `MyError`: `Some error description.`. Will wait 2.0 seconds before retrying.
Attempt 2 failed with error of type `MyError`: `Some error description.`. No remaining attempts.

@edigaryev
Copy link
Author

If I understood correctly, you are adding logging for the retries

Exactly.

If so, then have you considered passing a logger object to the retry function?

I see that there are two types of loggers that the retry function accepts:

The appleLogger uses os.Logger which is a structure for which one cannot change the logging level. It seems that the only way to force it to log to the standard output/error and log at the debug log level (which swift-retry uses) is to run the program via XCode, which is not what our users normally do.

The logger uses Logging.Logger which is a protocol for which the Logging.LogHandler can also be re-defined to get the access to the metadata, however, swift-retry package does not seem to set the (1) number of attempts left nor (2) the error that was occurred in the metadata:

// Only log the error type rather than the full error in case the error has private user data.
// We can include the full error if and when the `Logging` API offers a distinction between
// public and private data.

Taking into account the issues above, it seems to me that passing the missing bits to the recoverFromFailure callback is better approach anyways, because it is more type-safe and solves the public vs private data problem above, since the user can decide whether to log the error in question on their own.

@fumoboy007
Copy link
Owner

(Sorry for the delay. I missed your reply.)

swift-retry package does not seem to set the (1) number of attempts left nor (2) the error that was occurred in the metadata:

I just released version 0.2.4, which logs the maximum allowed number of attempts and the full error. Thanks for the suggestions!

Taking into account the issues above, it seems to me that passing the missing bits to the recoverFromFailure callback is better approach anyways, because it is more type-safe and solves the public vs private data problem above, since the user can decide whether to log the error in question on their own.

Hopefully with the above changes, this shouldn’t be necessary!

@edigaryev
Copy link
Author

@fumoboy007 thank you!

I haven't tested it yet, but with the changes you've mentioned this should be possible, so closing the issue.

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

No branches or pull requests

2 participants