[EASY] Add metrics for logging native price requests #3006

m-lord-renkse · 2024-09-19T12:47:08Z

Description

Add more metrics for native price requests. So we can properly assess how many requests are sent for each estimator.

m-lord-renkse · 2024-09-19T12:47:47Z

crates/shared/src/price_estimation/trade_finder.rs

@@ -104,16 +112,6 @@ impl Inner {
    }
 }

-impl Clone for TradeEstimator {


I guess this was done because it required in the pass a specific Clone implementation. But this currently does what the #[derive(Clone)] should do.

MartinquaXD

Already nitted on a few implementation details when I remembered that we already have InstrumentedPriceEstimator which already abstracted that logic.
Probably just needs a few adjustments to work with native price estimators as well.

crates/shared/src/price_estimation.rs

MartinquaXD · 2024-09-23T09:03:31Z

crates/shared/src/price_estimation.rs

+#[derive(prometheus_metric_storage::MetricStorage)]
+pub(in crate::price_estimation) struct Metrics {
+    /// number of requests done by each estimator
+    #[metric(labels("requests"))]


Suggested change

#[metric(labels("requests"))]

#[metric(labels("estimator"))]

I am unifying it with the same naming in InstrumentedPriceEstimator to be consistent.

crates/shared/src/price_estimation.rs

MartinquaXD · 2024-09-23T09:05:46Z

crates/shared/src/price_estimation/buffered.rs

@@ -91,6 +92,7 @@ where
        token: H160,
    ) -> futures::future::BoxFuture<'_, NativePriceEstimateResult> {
        async move {
+            Metrics::inc_estimator("buffered");


This would become problematic as soon as there are multiple different APIs that support buffering.

You are definitely right, let's add it to CoinGecko

Btw if you use the InstrumentedPriceEstimator wrapper thingy you don't have to add anything to coingecko. You just wrap it in that struct and it starts instrumenting it.

Yup, that's what I am doing, but I am trying to do it as nicer as possible, because InstrumentedPriceEstimator is hard-coded for PriceEstimating

m-lord-renkse · 2024-09-24T08:25:57Z

InstrumentedPriceEstimator

Sounds good, let's give it a try.

MartinquaXD

Nice!
Just a few nits

MartinquaXD · 2024-09-24T12:46:50Z

crates/shared/src/price_estimation/instrumented.rs

-pub struct InstrumentedPriceEstimator {
-    inner: Box<dyn PriceEstimating>,
+pub struct InstrumentedPriceEstimator<T> {
+    inner: Arc<T>,


Was there a reason to turn this into an Arc? Doesn't look necessary to me.

Good question! The reason I did it is because during the development I wanted to fully remove Box<> and to have just the type T, I put Arc<> temporarily to sanitize some errors and I forgot to delete it during the clean up. Good catch!

The type should be:

inner: T

We do not need any wrapper at all!

MartinquaXD · 2024-09-24T12:48:47Z

crates/shared/src/price_estimation/instrumented.rs

+            // Count as a successful request if the answer is ok (no error) or if the error
+            // is No Liquidity
+            let success =
+                estimate.is_ok() || matches!(&estimate, Err(PriceEstimationError::NoLiquidity));


I think counting NoLiquidity as a success makes sense but then we should also change that in the regular estimator implementation to not have a drift for no reason here.

Good point! I added a common function to evaluate the result! 👌

crates/shared/src/price_estimation/instrumented.rs

MartinquaXD · 2024-09-24T12:53:43Z

crates/shared/src/price_estimation/factory.rs

@@ -155,7 +155,7 @@ impl<'a> PriceEstimatorFactory<'a> {
            .as_ref()
            .and_then(|trade_verifier| estimator.verified(trade_verifier));

-        let fast = instrument(estimator, name);
+        let fast = instrument::<T>(estimator, name);


is ::<T> actually needed here? I thought the compiler would be smart enough to deduce T on it's own.

Good catch! 👀 it was needed for a middle version I did, but now it is not needed anymore 🙌

squadgazzz · 2024-09-24T19:24:51Z

crates/shared/src/price_estimation/factory.rs

+                ))
+            }
+            NativePriceEstimatorSource::OneInchSpotPriceApi => {
+                let name = "OneInchSpotPriceApi".to_string();


UpperCamelCase should be avoided since driver.name uses flatcase. We should use a similar format for other names as well. If not flatcase everywhere, maybe snake_case.

Yeah, I agree with you, but this is something systematic within the natice prices, we should change that in another PR alongside the configuration, because this happens also in the configuration of the services :/

squadgazzz · 2024-09-24T19:31:06Z

crates/shared/src/price_estimation/instrumented.rs

+            self.metrics
+                .native_price_estimation_times
+                .with_label_values(&[self.name.as_str()])
+                .observe(start.elapsed().as_secs_f64());
+            let result = self.estimate_result(estimate.as_ref());
+            self.metrics
+                .native_price_estimates
+                .with_label_values(&[self.name.as_str(), result])
+                .inc();


Do we really need 2 metrics? Can we use only the native_price_estimation_times with 2 labels and use the _count metric instead of declaring one more with increment?

@squadgazzz in the end, because of this comment: #3006 (comment)
it was decided to directly reuse the current metrics 👌

squadgazzz

LG

m-lord-renkse commented Sep 19, 2024

View reviewed changes

m-lord-renkse marked this pull request as ready for review September 19, 2024 12:49

m-lord-renkse requested a review from a team as a code owner September 19, 2024 12:49

Add metrics for logging native price requests

28b38c9

m-lord-renkse force-pushed the add-metrics-for-native-price-requests branch from 9311449 to 28b38c9 Compare September 19, 2024 13:04

MartinquaXD reviewed Sep 23, 2024

View reviewed changes

Rework comments

2fb58e4

m-lord-renkse force-pushed the add-metrics-for-native-price-requests branch from 3771174 to 2fb58e4 Compare September 24, 2024 11:58

m-lord-renkse requested a review from MartinquaXD September 24, 2024 12:00

MartinquaXD approved these changes Sep 24, 2024

View reviewed changes

m-lord-renkse added 2 commits September 24, 2024 15:43

nits cleanup

62838b7

Rework comments

4e3ecee

m-lord-renkse force-pushed the add-metrics-for-native-price-requests branch from b0a06d1 to 4e3ecee Compare September 24, 2024 14:07

squadgazzz reviewed Sep 24, 2024

View reviewed changes

Reuse metrics

3f137fc

m-lord-renkse requested a review from squadgazzz September 26, 2024 10:34

squadgazzz approved these changes Sep 26, 2024

View reviewed changes

m-lord-renkse force-pushed the add-metrics-for-native-price-requests branch from a243cdb to 3f137fc Compare September 26, 2024 10:44

Merge branch 'main' into add-metrics-for-native-price-requests

0f40ce7

m-lord-renkse enabled auto-merge (squash) September 26, 2024 10:47

m-lord-renkse merged commit 1b6ed89 into main Sep 26, 2024
11 checks passed

m-lord-renkse deleted the add-metrics-for-native-price-requests branch September 26, 2024 10:51

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EASY] Add metrics for logging native price requests #3006

[EASY] Add metrics for logging native price requests #3006

m-lord-renkse commented Sep 19, 2024

m-lord-renkse Sep 19, 2024

MartinquaXD left a comment

MartinquaXD Sep 23, 2024

m-lord-renkse Sep 24, 2024

MartinquaXD Sep 23, 2024

m-lord-renkse Sep 24, 2024

MartinquaXD Sep 24, 2024

m-lord-renkse Sep 24, 2024

m-lord-renkse commented Sep 24, 2024

MartinquaXD left a comment

MartinquaXD Sep 24, 2024

m-lord-renkse Sep 24, 2024

MartinquaXD Sep 24, 2024

m-lord-renkse Sep 24, 2024

MartinquaXD Sep 24, 2024

m-lord-renkse Sep 24, 2024

squadgazzz Sep 24, 2024 •

edited

Loading

m-lord-renkse Sep 25, 2024

squadgazzz Sep 24, 2024 •

edited

Loading

m-lord-renkse Sep 26, 2024

squadgazzz left a comment

[EASY] Add metrics for logging native price requests #3006

[EASY] Add metrics for logging native price requests #3006

Conversation

m-lord-renkse commented Sep 19, 2024

Description

Choose a reason for hiding this comment

MartinquaXD 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

m-lord-renkse commented Sep 24, 2024

MartinquaXD 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

squadgazzz Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz left a comment

Choose a reason for hiding this comment

squadgazzz Sep 24, 2024 •

edited

Loading

squadgazzz Sep 24, 2024 •

edited

Loading