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.

Sign up for GitHub

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

Jump to bottom

feat(maker): Monitor and expose status of services connected #1180

Merged
merged 1 commit into from
Sep 13, 2023
Merged

feat(maker): Monitor and expose status of services connected #1180

merged 1 commit into from
Sep 13, 2023

Conversation

klochowicz
Copy link
Contributor

@klochowicz klochowicz commented Aug 29, 2023 •
edited
Loading

Monitor the following services:
- coordinator
- orderbook (we don't subscribe to orderbook feed yet)

in order to derive health status of the maker.

Health status is exposed via the HTTP API as well as reported as
dedicated prometheus metrics.

  • Subscribe to orderbook websocket stream to be able to detect if it is down
  • Subscribe to bitmex client stream to be able to spot outdated information

the TODO listed above will be wired only when bitmex and orderbook streams will be integrated into maker, and as such are not a dependency of this PR - we can wire them in the corresponding PRs or in a follow-up.

@klochowicz
Copy link
Contributor Author

I'm not sure whether we actually need the orderbook status (we don't seem to rely on the websocket stream at all so far? 🤔), and bitmex status can be added when we actually start hedging (it was not mentioned in the ticket, but I think it's an essential service for the maker to be monitored).

@klochowicz klochowicz mentioned this pull request Aug 29, 2023
Closed
3 tasks
Restioson
Restioson approved these changes Sep 5, 2023
View reviewed changes
crates/tests-e2e/src/maker.rs Outdated Show resolved Hide resolved
maker/src/health.rs Outdated Show resolved Hide resolved
bonomat
bonomat reviewed Sep 5, 2023
View reviewed changes
maker/src/metrics.rs Outdated Show resolved Hide resolved
bonomat
bonomat reviewed Sep 5, 2023
View reviewed changes
maker/src/routes.rs
@@ -52,6 +56,7 @@ pub fn router(
.route("/api/pay-invoice/:invoice", post(pay_invoice))
.route("/api/sync-on-chain", post(sync_on_chain))
.route("/metrics", get(get_metrics))
.route("/health", get(get_health))
Copy link
Contributor

Choose a reason for hiding this comment

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

For whom is this new endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's useful in development, testing, and manual checking if we want to quickly see the state.

bonomat
bonomat requested changes Sep 5, 2023
View reviewed changes
Copy link
Contributor

@bonomat bonomat left a comment

Choose a reason for hiding this comment

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

There are a few comments/todos in the code and I believe the usage of the metrics is not correct.
Let me know if my comment is not clear.

maker/src/health.rs Outdated Show resolved Hide resolved
maker/src/health.rs Outdated Show resolved Hide resolved
maker/src/metrics.rs Outdated Show resolved Hide resolved
@klochowicz
feat(maker): Monitor and expose status of connected services
43db279 
Monitor the following services:
    - coordinator
    - orderbook (as we don't subscribe to orderbook feed yet, so it's hardcoded to "online")
    - bitmex pricefeed

in order to derive health status of the maker.

Health status is exposed via the HTTP API as well as reported as
dedicated prometheus metrics.
@klochowicz klochowicz requested a review from bonomat September 13, 2023 03:33
@klochowicz
Copy link
Contributor Author

klochowicz commented Sep 13, 2023 •
edited
Loading

There are a few comments/todos in the code and I believe the usage of the metrics is not correct.
Let me know if my comment is not clear.

I believe I fixed the issue.
the only todo left in code is about us not using the orderbook websocket at all yet, so there's not much I can do about it. I've added bitmex stream monitoring.

bonomat
bonomat approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@bonomat bonomat left a comment

Choose a reason for hiding this comment

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

LGTM

@bonomat
Copy link
Contributor

bonomat commented Sep 13, 2023

There are a few comments/todos in the code and I believe the usage of the metrics is not correct.
Let me know if my comment is not clear.

I believe I fixed the issue. the only todo left in code is about us not using the orderbook websocket at all yet, so there's not much I can do about it. I've added bitmex stream monitoring.

Thanks for addressing.

@klochowicz klochowicz added this pull request to the merge queue Sep 13, 2023
Merged via the queue into main with commit 80f288b Sep 13, 2023
7 checks passed
@klochowicz klochowicz deleted the feat/maker-metrics branch September 13, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Restioson Restioson Restioson approved these changes

@bonomat bonomat bonomat approved these changes

@luckysori luckysori Awaiting requested review from luckysori

@holzeis holzeis Awaiting requested review from holzeis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@klochowicz @bonomat @Restioson