Add metrics to gas price service #2635
Conversation
| @@ -303,12 +309,61 @@ where | |||
| AtomicStorage::commit_transaction(storage_tx)?; | |||
| let new_algo = self.algorithm_updater.algorithm(); | |||
| tracing::debug!("Updating gas price: {}", &new_algo.calculate()); | |||
| self.shared_algo.update(new_algo); | |||
| self.shared_algo.update(new_algo).await; | |||
There was a problem hiding this comment.
I'm confused by the error:
pub async fn update(&mut self, new_algo: A) {
let mut write_lock = self.0.write();
*write_lock = new_algo;
}
| // Clear the buffer after committing changes | ||
| self.da_block_costs_buffer.clear(); | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn record_metrics( |
There was a problem hiding this comment.
we should conditionally export metrics, see description of #2310 for reference
| let (old_recorded_height, mut new_recorded_height) = match storage_tx | ||
| .get_recorded_height() | ||
| .map_err(|err| anyhow!(err))? | ||
| { | ||
| Some(_) => None, | ||
| Some(old) => (Some(old), None), | ||
| None => { | ||
| // Sets it on first run | ||
| self.initial_recorded_height.take() | ||
| let initial = self.initial_recorded_height.take(); | ||
| (initial, initial) | ||
| } | ||
| }; |
There was a problem hiding this comment.
Maybe (None, initial) would be clearer.
| let exec_gas_price_i64 = v1_metadata | ||
| .new_exec_gas_price() | ||
| .try_into() | ||
| .unwrap_or(i64::MAX); |
There was a problem hiding this comment.
Nit: I generally prefer explicit conversions for readability. I.e. i64::try_from().
I'm also uncertain if it makes sense to report i64::MAX if we're out of range. Wouldn't it be better to not report any metrics and log an error in this case?
|
Just a question: Are you sure that it is enough information for you to understand the all internal state of the algorithm? |
I think this is enough. I was missing the |
## Version 0.41.5 ### Changed - [2387](#2387): Update description `tx-max-depth` flag. - [2630](#2630): Removed some noisy `tracing::info!` logs - [2643](#2643): Before this fix when tip is zero, transactions that use 30M have the same priority as transactions with 1M gas. Now they are correctly ordered. ### Added - [2617](#2617): Add integration skeleton of parallel-executor. - [2553](#2553): Scaffold global merkle root storage crate. - [2598](#2598): Add initial test suite for global merkle root storage updates. - [2635](#2635): Add metrics to gas price service ### Fixed - [2632](#2632): Improved performance of certain async trait impls in the gas price service. - [2662](#2662): Fix balances query endpoint cost without indexation and behavior coins to spend with one parameter at zero.
Linked Issues/PRs
Description
A lot of the data that effects the gas price is somewhat opaque. Up until now, it's been hard to read without direct access to the Gas Price DB. This will give us visibility into:
which allows us to make charts like (plz ignore the gaps, this was just testing the visualizations):
Before requesting review