Skip to content

[r2r] update metrics related dep && refactoring#1312

Merged
sergeyboyko0791 merged 49 commits intodevfrom
metrics-deps-update
Aug 10, 2022
Merged

[r2r] update metrics related dep && refactoring#1312
sergeyboyko0791 merged 49 commits intodevfrom
metrics-deps-update

Conversation

@borngraced
Copy link
Copy Markdown

@borngraced borngraced commented Jun 15, 2022

— Updating metrics related deps
— Refactoring mm_metrics

@borngraced borngraced changed the title update metrics related dep [WIP] update metrics related dep Jun 16, 2022
@borngraced borngraced marked this pull request as draft June 17, 2022 21:25
@borngraced borngraced marked this pull request as ready for review June 20, 2022 10:25
@borngraced
Copy link
Copy Markdown
Author

borngraced commented Jun 20, 2022

My refactoring to look for review is work in progress in adapt.rs file @sergeyboyko0791 @shamardy

@borngraced borngraced changed the title [WIP] update metrics related dep [WIP] update metrics related dep && refactoring Jun 20, 2022
Copy link
Copy Markdown

@sergeyboyko0791 sergeyboyko0791 left a 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.

Comment thread mm2src/common/Cargo.toml Outdated
Comment thread mm2src/common/Cargo.toml Outdated
Comment thread mm2src/common/mm_metrics/adapt.rs Outdated
Comment thread mm2src/common/mm_metrics/adapt.rs Outdated
Comment thread mm2src/common/mm_metrics/adapt.rs Outdated
Comment thread mm2src/common/mm_metrics/adapt.rs Outdated
Comment thread mm2src/common/mm_metrics/adapt.rs Outdated
Comment thread mm2src/common/mm_metrics/adapt.rs Outdated
Copy link
Copy Markdown

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

Next review iteration :)

Comment thread mm2src/mm2_metrics/Cargo.toml
Comment thread mm2src/mm2_metrics/src/adapt.rs Outdated
Comment thread mm2src/mm2_metrics/src/adapt.rs Outdated
Comment thread mm2src/mm2_metrics/src/adapt.rs Outdated
Comment thread mm2src/mm2_metrics/src/adapt.rs Outdated
Comment thread mm2src/mm2_metrics/src/adapt.rs Outdated
Comment thread mm2src/mm2_metrics/src/adapt.rs Outdated
Comment thread mm2src/mm2_metrics/src/adapt.rs Outdated
Comment thread mm2src/mm2_metrics/src/adapt.rs Outdated
Copy link
Copy Markdown

@sergeyboyko0791 sergeyboyko0791 left a 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.

Comment thread mm2src/coins/Cargo.toml Outdated
Comment thread mm2src/mm2_main/Cargo.toml Outdated
Comment thread mm2src/mm2_main/src/lp_ordermatch.rs
Comment thread mm2src/mm2_metrics/src/adapt.rs Outdated
Comment thread mm2src/mm2_metrics/src/adapt.rs Outdated
Comment thread mm2src/mm2_metrics/src/recorder.rs Outdated
Comment thread mm2src/mm2_metrics/src/recorder.rs Outdated
Comment thread mm2src/mm2_metrics/src/recorder.rs Outdated
Comment thread mm2src/mm2_metrics/src/recorder.rs Outdated
Comment thread mm2src/mm2_metrics/src/recorder.rs Outdated
@borngraced
Copy link
Copy Markdown
Author

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

@borngraced borngraced changed the title [WIP] update metrics related dep && refactoring [r2r] update metrics related dep && refactoring Jun 29, 2022
@borngraced borngraced linked an issue Jun 29, 2022 that may be closed by this pull request
Copy link
Copy Markdown

@sergeyboyko0791 sergeyboyko0791 left a 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

Comment thread mm2src/mm2_metrics/src/mm_metrics.rs Outdated
Comment thread mm2src/mm2_metrics/src/mm_metrics.rs Outdated
Comment thread mm2src/mm2_metrics/src/mm_metrics.rs Outdated
Copy link
Copy Markdown

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

One important note

Comment thread mm2src/mm2_metrics/src/mm_metrics.rs Outdated
Copy link
Copy Markdown

@sergeyboyko0791 sergeyboyko0791 left a 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

Comment thread mm2src/coins/Cargo.toml Outdated
Comment thread mm2src/mm2_main/src/lp_network.rs
Comment thread mm2src/mm2_metrics/Cargo.toml Outdated
Comment thread mm2src/mm2_metrics/src/lib.rs Outdated
Comment thread mm2src/mm2_metrics/src/mm_metrics.rs Outdated
Comment thread mm2src/mm2_metrics/src/mm_metrics.rs Outdated
Comment thread mm2src/mm2_metrics/src/mm_metrics.rs Outdated
Comment thread mm2src/mm2_metrics/src/recorder.rs Outdated
@borngraced borngraced changed the title [wip] update metrics related dep && refactoring [r2r] update metrics related dep && refactoring Aug 1, 2022
Copy link
Copy Markdown

@sergeyboyko0791 sergeyboyko0791 left a 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

Comment thread mm2src/mm2_metrics/src/recorder.rs Outdated
}
}

use std::collections::hash_map::Entry::{Occupied, Vacant};
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Comment thread mm2src/mm2_metrics/src/recorder.rs Outdated
sergeyboyko0791
sergeyboyko0791 previously approved these changes Aug 4, 2022
Copy link
Copy Markdown

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

@sergeyboyko0791
Copy link
Copy Markdown

@shamardy your turn to review the PR

shamardy
shamardy previously approved these changes Aug 4, 2022
Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a 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 :)

Comment thread mm2src/mm2_main/src/lp_network.rs Outdated
Comment thread mm2src/mm2_metrics/src/mm_metrics.rs Outdated
@borngraced borngraced dismissed stale reviews from shamardy and sergeyboyko0791 via 914cfd0 August 5, 2022 09:15
Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Reapprove

Copy link
Copy Markdown

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

Re-approve

@artemii235
Copy link
Copy Markdown

@sergeyboyko0791 As you both approved it with @shamardy, I guess it can be merged.

@sergeyboyko0791
Copy link
Copy Markdown

@artemii235 I wanted to merge it, but found an issue with the CI tests on Mac. Restarted the CI and forgot to check...
Will check now

@artemii235
Copy link
Copy Markdown

@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.

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

Successfully merging this pull request may close these issues.

Update metrics related deps.

4 participants