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

Move much of the logging out of the driver #448

Merged
merged 14 commits into from
Aug 22, 2022
Merged

Move much of the logging out of the driver #448

merged 14 commits into from
Aug 22, 2022

Conversation

MartinquaXD
Copy link
Contributor

Useful refactoring for step 8 of #337

Settlement submission has a lot of subtleties and currently involves a huge amount of logging and metrics. Ideally the new driver would do that exactly as the current one and to not introduce bugs by poorly copying bits and pieces I refactored the current logging logic into the DriverLogger. I also made submit_settlement() a free function which only needs a SolutionSubmitter and a DriverLogger to handle the submission, logging and metrics.
That allows me to simply re-use that component in the new driver resulting in the identical behavior.

The refactor moves a bunch of code so to help with the review I made small commits which all compiled without any clippy warnings. So I would suggest reviewing those commits individually and in order.

To be perfectly honest this refactor feels a bit shoehorned but I think it's a reasonable step to untangle the current driver and hopefully move to greener pastures in the new driver.

Test Plan

CI

@MartinquaXD MartinquaXD requested a review from a team as a code owner August 11, 2022 14:39
Copy link
Contributor

@vkgnosis vkgnosis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good cleanup, definitely while the driver code is in flux. I don't know if we want to follow this pattern of moving all log functions out forever but it is helpful now.

Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels a bit shoe-horned, but very useful for the refactor.

@MartinquaXD MartinquaXD changed the title Move much of the logging logging out of the driver Move much of the logging out of the driver Aug 15, 2022
@MartinquaXD MartinquaXD merged commit 6d56663 into main Aug 22, 2022
@MartinquaXD MartinquaXD deleted the driver-logger branch August 22, 2022 06:31
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants