[PROPOSAL] Correctly label the orderbook orders metric #2917

m-lord-renkse · 2024-08-21T14:15:19Z

Description

We need to properly adjust the orderbook orders metric. The reason is that we got a false alarm alert, because as it is now, everything is a limit order, so we got a period of time in which we got orders (real limit ones) and no trades (because those orders were out of market).

This could be solved in many ways, as discussed with @MartinquaXD , one way could be to only report "market" orders. But after giving it a deep thought, I think this is the best way, because we don't lose any type of order, and we report all of them labelled correctly.

Changes

Since all the orders are limit orders, I changed the OrderClass of the orders metric to reflect real limit orders or user orders (order not outside the market price). This way we keep all the orders tracked. I meant this class to be exclusively of the metric system, as we don't intent to store user order (aka market order) in the domain. But it is handy to have it in the metrics.
Use strum to serialize properly the enums
Classify the metric's OrderClass accordingly to the quote price if present.

How to test

Checking / adjusting the metrics in grafana in staging

crates/orderbook/src/orderbook.rs

MartinquaXD · 2024-08-21T19:45:51Z

crates/orderbook/src/orderbook.rs

+#[derive(Display)]
+#[strum(serialize_all = "snake_case")]
+enum OrderClass {
+    User,


I think user was probably a misnomer to begin with. Let's call it market instead and adjust the grafana alerts accordingly.

If we rename this, why don't we call it Market and OutOfMarket or somethinglike this? Technically a market order is also a limit order.

In fact, it could maybe simply become a boolean inMarket?

Since we have everywhere "market" and "limit" still, I would suggest to stick with that, otherwise we add inconsistencies. We could clean up all the "market" and "limit" code, and then unify it properly (we also have the "liquidity" there...)

crates/orderbook/src/orderbook.rs

fleupold

I think the approach is good, but maybe we can make the enum more clear or even reduce it to a simple boolean?

fleupold · 2024-08-22T07:53:43Z

crates/orderbook/src/orderbook.rs

+#[derive(Display)]
+#[strum(serialize_all = "snake_case")]
+enum OrderClass {
+    User,


If we rename this, why don't we call it Market and OutOfMarket or somethinglike this? Technically a market order is also a limit order.

In fact, it could maybe simply become a boolean inMarket?

fleupold · 2024-08-22T07:56:34Z

crates/orderbook/src/orderbook.rs

@@ -42,24 +47,18 @@ struct Metrics {
    orders: prometheus::IntCounterVec,
 }

+#[derive(Display)]
+#[strum(serialize_all = "snake_case")]
 enum OrderOperation {
    Created,


Looks like Created should take an Option<Quote> (since quotes are not relevant for cancelled operations). This would also make on_order_operation a bit cleaner (since right now there is an implicit assumption that quote is always None for Cancellation events)

I will try to retrieve the quote for the cancelation events, so we can correctly label them.

Co-authored-by: Martin Beckmann <martin.beckmann@protonmail.com>

m-lord-renkse · 2024-08-23T12:01:38Z

crates/orderbook/src/database/orders.rs

+                    .zip(order_with_quote.quote_sell_token_price)
+                    .zip(order_with_quote.solver)
+                    .map(
+                        |(


Weird formatting :/

m-lord-renkse · 2024-08-23T12:02:40Z

crates/database/src/orders.rs

@@ -567,6 +579,26 @@ pub async fn single_full_order(
    sqlx::query_as(QUERY).bind(uid).fetch_optional(ex).await
 }

+pub async fn single_full_order_with_quote(


Create a new query so we don't need to do two roundtrip to get an order with its quote data.

m-lord-renkse · 2024-08-23T12:04:08Z

crates/orderbook/src/database/orders.rs

 }

 pub struct SolvableOrders {
    pub orders: Vec<Order>,
    pub latest_settlement_block: u64,
 }

+pub struct OrderWithQuote {


Created a new object which contains an order with the quote information (the quote information corresponding to the one stored in order_quotes table, which is the only info we need, not the full quote).

github-actions · 2024-08-31T00:07:29Z

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

crates/orderbook/src/database/orders.rs

MartinquaXD · 2024-09-09T10:19:57Z

crates/orderbook/src/database/orders.rs

+    }
+}
+
+impl Deref for OrderWithQuote {


Let's not try to get fancy here. Deref is mostly "reserved" for smart pointer types and things like drop guards.

I will change it for simplicity, but that actually is not true. Deref is for whatever use you want to use it, as long as it suits you. And it is definitely used for structs in which you have a main object, and then extra appended data, then you want to deref to the main struct instead of indexing it all the time. There are even crates created on top of Deref for cleaner code (see delegate crate).

😮‍💨 now it is super ugly with the order.order...

I think I am going to revert it, that's why deref was created...

Initially the docs specifically mentioned to only use if for smart pointers.

since then the phrasing was relaxed to users of that type should not get surprised by deref which arguable is not the case here because we almost never use Deref (mostly a point for code consistency)

now it is super ugly with the order.order
Sure, but it also doesn't look like you tried hard to avoid it looking ugly...

let OrderWithQuote{order, quote} = weird_new_struct;

Ultimately not a hill I'm willing to die an but IMO requiring this "hack" rather indicates that the chosen abstraction doesn't work great.
For example you could also consider making the quote part of the FullOrder which probably is useful in more than just this specific case.

@MartinquaXD yup, I am aware of that PR that removed the comment, I remember it back in the day when it came out. It was a "ok guys, use it as you want" kind of thing 😄 and then many crates like derive_deref, delegate, deref_into, shrinkwraprs, ambassador crates came up (and for sure there are more, but those are the ones I remember, some of them already existed from before like deref_into IIRC). So, that's why I don't agree it is for smart pointers only, it used to be, but now it is quite accepted for many other usages.

Sure, but it also doesn't look like you tried hard to avoid it looking ugly...
😬 I wanted to make a point, that with just adding the deref implementation, we need to avoid order.order or like the code you propose. It is extra lines/overhead.

I agree with you that adding a deref in a codebase it wasn't used before (it is used before but little) may create confusion (that's why I reverted in the first place), but I don't think it adds inconsistency. If we cannot add new crates/patterns because it is the first one, then we will never add anything 🤔

Ultimately not a hill I'm willing to die an but IMO requiring this "hack" rather indicates that the chosen abstraction doesn't work great.

Yup, I agree with you here to be honest. I am not convinced of the struct myself either. But since it was used "just" for the metrics, and it is a pattern I already saw in the codebase, I was like "ok". But I am not super happy about it.

For example you could also consider making the quote part of the FullOrder which probably is useful in more than just this specific case.

That'd be great, if you tell me it is worth doing it. I will definitely do it! my only concern with that is that now for each single full order we also fetch the quote (same roundtrip at least).

MartinquaXD · 2024-09-09T10:20:56Z

crates/orderbook/src/database/orders.rs

@@ -323,6 +354,51 @@ impl OrderStoring for Postgres {
        order.map(full_order_into_model_order).transpose()
    }

+    async fn single_order_with_quote(&self, uid: &OrderUid) -> Result<Option<OrderWithQuote>> {


Could probably be part of an existing DB test which tests quote fetching in general.

Really good idea, adding them!

Added a test at OrderStoring level and database level 👌

sunce86

Lg

m-lord-renkse commented Aug 21, 2024

View reviewed changes

crates/orderbook/src/orderbook.rs Outdated Show resolved Hide resolved

Correctly label the orderbook orders metric

41adcd3

m-lord-renkse force-pushed the improve-grafana-orderbook-order-metric branch from 9616717 to 41adcd3 Compare August 21, 2024 14:20

m-lord-renkse marked this pull request as ready for review August 21, 2024 14:22

m-lord-renkse requested a review from a team as a code owner August 21, 2024 14:22

MartinquaXD reviewed Aug 21, 2024

View reviewed changes

fleupold reviewed Aug 22, 2024

View reviewed changes

m-lord-renkse and others added 3 commits August 22, 2024 11:47

Update crates/orderbook/src/orderbook.rs

e686e8e

Co-authored-by: Martin Beckmann <martin.beckmann@protonmail.com>

Update crates/orderbook/src/orderbook.rs

e88e34f

Co-authored-by: Martin Beckmann <martin.beckmann@protonmail.com>

Merge branch 'main' into improve-grafana-orderbook-order-metric

21a667b

m-lord-renkse marked this pull request as draft August 23, 2024 11:39

m-lord-renkse force-pushed the improve-grafana-orderbook-order-metric branch from 514c2f0 to bf58f16 Compare August 23, 2024 11:41

Rework comments

bed0d84

m-lord-renkse force-pushed the improve-grafana-orderbook-order-metric branch from bf58f16 to bed0d84 Compare August 23, 2024 12:00

m-lord-renkse commented Aug 23, 2024

View reviewed changes

m-lord-renkse and others added 2 commits August 23, 2024 14:31

Merge branch 'main' into improve-grafana-orderbook-order-metric

028a1f8

fix

b393f2f

m-lord-renkse marked this pull request as ready for review August 23, 2024 12:54

github-actions bot added the stale label Aug 31, 2024

Merge branch 'main' into improve-grafana-orderbook-order-metric

b2ca305

m-lord-renkse requested review from MartinquaXD and fleupold September 2, 2024 07:53

Merge branch 'main' into improve-grafana-orderbook-order-metric

e1b4929

github-actions bot removed the stale label Sep 3, 2024

Merge branch 'main' into improve-grafana-orderbook-order-metric

2adfe39

squadgazzz reviewed Sep 4, 2024

View reviewed changes

crates/orderbook/src/database/orders.rs Outdated Show resolved Hide resolved

m-lord-renkse and others added 2 commits September 6, 2024 10:34

Merge branch 'main' into improve-grafana-orderbook-order-metric

9cb45a7

Rework comment

adee18a

m-lord-renkse requested a review from squadgazzz September 6, 2024 09:01

squadgazzz approved these changes Sep 6, 2024

View reviewed changes

MartinquaXD reviewed Sep 9, 2024

View reviewed changes

sunce86 approved these changes Sep 9, 2024

View reviewed changes

m-lord-renkse and others added 4 commits September 9, 2024 12:54

Remove deref

f8aeba6

Added test

b4c484d

Merge branch 'main' into improve-grafana-orderbook-order-metric

ed621c6

Adjust to latest changes

3ec80de

m-lord-renkse force-pushed the improve-grafana-orderbook-order-metric branch from abb7bcc to 3ec80de Compare September 12, 2024 14:38

Merge branch 'main' into improve-grafana-orderbook-order-metric

fc58810

m-lord-renkse mentioned this pull request Sep 12, 2024

chore: Include the quote in the full order #2981

Closed

m-lord-renkse merged commit 61240bc into main Sep 12, 2024
11 checks passed

m-lord-renkse deleted the improve-grafana-orderbook-order-metric branch September 12, 2024 15:24

github-actions bot locked and limited conversation to collaborators Sep 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PROPOSAL] Correctly label the orderbook orders metric #2917

[PROPOSAL] Correctly label the orderbook orders metric #2917

m-lord-renkse commented Aug 21, 2024

MartinquaXD Aug 21, 2024

fleupold Aug 22, 2024

m-lord-renkse Aug 22, 2024

fleupold left a comment

fleupold Aug 22, 2024

fleupold Aug 22, 2024

m-lord-renkse Aug 22, 2024

m-lord-renkse Aug 23, 2024

m-lord-renkse Aug 23, 2024

m-lord-renkse Aug 23, 2024

github-actions bot commented Aug 31, 2024

MartinquaXD Sep 9, 2024

m-lord-renkse Sep 9, 2024

m-lord-renkse Sep 9, 2024

m-lord-renkse Sep 9, 2024

MartinquaXD Sep 9, 2024 •

edited

Loading

m-lord-renkse Sep 9, 2024 •

edited

Loading

m-lord-renkse Sep 12, 2024

MartinquaXD Sep 9, 2024

m-lord-renkse Sep 9, 2024

m-lord-renkse Sep 9, 2024

sunce86 left a comment

[PROPOSAL] Correctly label the orderbook orders metric #2917

[PROPOSAL] Correctly label the orderbook orders metric #2917

Conversation

m-lord-renkse commented Aug 21, 2024

Description

Changes

How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinquaXD Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

m-lord-renkse Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunce86 left a comment

Choose a reason for hiding this comment

MartinquaXD Sep 9, 2024 •

edited

Loading

m-lord-renkse Sep 9, 2024 •

edited

Loading