update: fixed analytics #421
Conversation
This PR is not related to Signoz. |
crates/client/analytics/src/lib.rs
Outdated
| @@ -196,6 +196,24 @@ pub fn register_counter_metric_instrument<T: CounterType<T> + Display>( | |||
| T::register_counter(crate_meter, instrument_name, desc, unit) | |||
| } | |||
|
|
|||
| pub trait UpDownCounterType<T> { | |||
There was a problem hiding this comment.
would be great to document those functions
crates/client/analytics/src/lib.rs
Outdated
| @@ -196,6 +196,24 @@ pub fn register_counter_metric_instrument<T: CounterType<T> + Display>( | |||
| T::register_counter(crate_meter, instrument_name, desc, unit) | |||
| } | |||
|
|
|||
| pub trait UpDownCounterType<T> { | |||
| fn register_counter(meter: &Meter, name: String, description: String, unit: String) -> UpDownCounter<T>; | |||
There was a problem hiding this comment.
I would rename that register_up_down_counter to align with other declarations
crates/client/mempool/src/lib.rs
Outdated
| @@ -187,7 +187,7 @@ impl Mempool { | |||
| true, | |||
| )?; | |||
|
|
|||
| self.metrics.accepted_transaction_counter.add(1, &[]); | |||
| self.metrics.accepted_transaction_counter.add(1.0, &[]); | |||
There was a problem hiding this comment.
why do we need a decimal counter here? cant we stick with an integer?
There was a problem hiding this comment.
also why an upDownCounter if we never decrement? AM I missing something?
There was a problem hiding this comment.
Yeah, initially we were going store the flow of increase and decrease of transactions in the mempool, but now we deprecated that thought, removed the up_down_counter logic completely
| @@ -113,15 +113,15 @@ impl RpcMetrics { | |||
| tracing::trace!(target: "rpc_metrics", "[{transport_label}] on_response started_at={:?}", now); | |||
| tracing::trace!(target: "rpc_metrics::extra", "[{transport_label}] result={}", rp.as_result()); | |||
|
|
|||
| let micros = now.elapsed().as_micros(); | |||
| let millis = now.elapsed().as_millis(); | |||
| tracing::debug!( | |||
| target: "rpc_metrics", | |||
| "[{transport_label}] {} call took {} μs", | |||
There was a problem hiding this comment.
here you should remove μs seconds to mili
| tracing::debug!( | ||
| target: "rpc_metrics", | ||
| "[{transport_label}] {} call took {} μs", | ||
| "[{transport_label}] {} call took {} ms", |
There was a problem hiding this comment.
can you use Duration {:?} (Debug) impl instead? it'll add the s/ms/us suffix depending on the value - i think that's what we used to do before you switched the codebase to tracing
There was a problem hiding this comment.
Good catch !
Added
| @@ -113,15 +113,15 @@ impl RpcMetrics { | |||
| tracing::trace!(target: "rpc_metrics", "[{transport_label}] on_response started_at={:?}", now); | |||
| tracing::trace!(target: "rpc_metrics::extra", "[{transport_label}] result={}", rp.as_result()); | |||
|
|
|||
| let micros = now.elapsed().as_micros(); | |||
| let millis = now.elapsed().as_millis(); | |||
There was a problem hiding this comment.
my comment was about doing
let time = now.elapsed();
tracing::debug!(
target: "rpc_metrics",
"[{transport_label}] {} call took {:?}",
req.method_name(),
time,
);
self.calls_time.record(time.to_millis() as f64, &[KeyValue::new("method", req.method_name().to_string())]);instead, using the Debug impl of Duration, not the Debug impl of u64
There was a problem hiding this comment.
Interesting, but don't you think this increases possibility of the two readings going out of sync,
let's say due to xyz reasons a RPC call took 1 sec, the tracing might record it as 1 sec but the calls_time metric will record it as 1000 milliseconds.
Whereas in the previous implementation both would have recorded 1000 milliseconds
WDYT ?
This PR introduces some fixes on the instrumentation code.
millisecondsinstead ofmicroseconds