-
Notifications
You must be signed in to change notification settings - Fork 94
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
[r2r] update metrics related dep && refactoring #1312
Conversation
My refactoring to look for review is work in progress in |
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.
It would also be great if we moved mm_metrics
to a separate crate like mm2_err_handle
.
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.
Next review iteration :)
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.
I could not describe all the mistakes. I'll do it on the next review iteration.
Looking good except the tag exporter, I need a better solution as the solution I tried isn't working well https://github.com/KomodoPlatform/atomicDEX-API/pull/1312#discussion_r906242654 |
…into metrics-deps-update
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.
Thank you for the fixes! I hope the last my comments
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.
One important note
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.
The last review iteration
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.
Only one change please and one suggestion
mm2src/mm2_metrics/src/recorder.rs
Outdated
@@ -154,20 +153,16 @@ impl MmRecorder { | |||
} | |||
} | |||
|
|||
use std::collections::hash_map::Entry::{Occupied, Vacant}; |
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.
It's better to put imports at the beginning of the file
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.
done
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.
LGTM 🔥
@shamardy your turn to review the PR |
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.
Great Work! Only 2 non-blocker comments :)
914cfd0
…into metrics-deps-update
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.
Reapprove
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.
Re-approve
@sergeyboyko0791 As you both approved it with @shamardy, I guess it can be merged. |
@artemii235 I wanted to merge it, but found an issue with the CI tests on Mac. Restarted the CI and forgot to check... |
@sergeyboyko0791 If you see errors only on one platform and there are only unstable tests failures, you can merge the PR right away without restarting the CI. |
— Updating metrics related deps
— Refactoring mm_metrics