-
Notifications
You must be signed in to change notification settings - Fork 122
autoloop: add reasons to explain no action #332
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
f6d87f0 to
ea6524a
Compare
bhandras
approved these changes
Feb 5, 2021
Member
bhandras
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice improvement! LGTM 🥇
guggero
approved these changes
Feb 5, 2021
Contributor
guggero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK, LGTM 💯
This is extremely useful! Both the returned reason in the RPC as well as the log messages.
ea6524a to
32b4580
Compare
In an effort to surface more information about why autoloop is not executing, we add an error when suggest swaps is called with no rules. In other cases we can surface a reason enum with each rule that is set, but in the case where we have no rules, there are no results to accompany with reasons.
In practice, this restriction has proven to be too strict. Autoloop will now only hold off on a swap for a channel if a manual swap is specifically using that channel.
This commit switches up our eligible channels logic to rather return a struct containing information about our current swap traffic. This change is made in preparation for returning reasons indicating why we did not perform a swap for a specific channel - when we only return eligible swaps, we lose the information about why all the excluded channels weren't used.
32b4580 to
cf50ffd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One UX issue that people have run into using autoloop is that they don't know why no swaps are being executed, and need to go digging in debug logs to get an explanation. This PR surfaces a variety of reasons for not executing a swap to help better explain our logic, and updates our docs to provide recommendations for updating config to address these reasons.
This PR also relaxes our requirement that no "unrestricted" swaps are in flight, which was causing problems in practice where autoloop can't coexist with manually initiated swaps.
Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixes