refactor(rpc-server): Split error metrics to disambiguate the error reasons #98
Conversation
khorolets
requested review from
gmilescu,
telezhnaya,
kobayurii and
andrei-near
There was a problem hiding this comment.
this PR will break the existing ReadRPC dashboard
Which one?
There's a more elegant way to create all these counters (e.g. using arrays should make this PR ~5 times shorter, we anyway use magic constants)
But I'm fine with the current approach if it solves the problem
| DataMismatchReason::ReadRpcSuccessNearRpcSuccess => 0, | ||
| DataMismatchReason::ReadRpcSuccessNearRpcError => 1, | ||
| DataMismatchReason::ReadRpcErrorNearRpcSuccess => 2, | ||
| DataMismatchReason::ReadRpcErrorNearRpcError => 3, |
There was a problem hiding this comment.
Haven't you missed the last option here?
There was a problem hiding this comment.
No, number 4 is for other reason of error, it's not related to the DataMismatchReason
| crate::utils::capture_shadow_consistency_error!( | ||
| err, | ||
| error_meta, | ||
| "EXPERIMENTAL_TX_STATUS" |
There was a problem hiding this comment.
Minor: last arg could be enum to avoid typos
There was a problem hiding this comment.
You are right, though we can improve it later.
| // QUERY counters | ||
| lazy_static! { | ||
| // QUERY.view_account | ||
| pub(crate) static ref QUERY_VIEW_ACCOUNT_ERROR_0: IntCounter = try_create_int_counter( |
There was a problem hiding this comment.
don't we need to mark all this with the feature flag as well?
There was a problem hiding this comment.
I mean we could, though it's not that big of a trouble.
Co-authored-by: Olga Telezhnaya <olyatelezhnaya@gmail.com>
Could you elaborate on arrays, I believe my knowledge of Prometheus is too weak. If you feel inspired enough, could you fire a separate issue about improvements (arrays)? |
|
I meant arrays on Rust side |
Resolves #97
What this PR is about?
In this PR my goal was to disambiguate the error counters to reflect different (known) reasons for the errors. I achieve this by renaming the existing metrics and adding the code as a suffix.
Yeah, I am aware of the antipattern of "magic numbers", but I want to keep the metric names shorter. Here's the list of the available codes (also, I've added this info to the
rpc-serverREADME).So for the
blockmethod, the metric with error code 0 will look like thisBLOCK_ERROR_0.Things I want to draw your attention to
All the changes in this PR work only with the
shadow_data_consistencyfeature flag and don't affect the other sides of the project.FYI while renaming metrics, I've reached some limits of
lazy_static, so I had to group the static refs of the metrics to a separatelazy_static! {}"containers".To keep the code cleaner and follow the DRY principle, I introduced a macro
capture_shadow_consistency_error!that handles theShadowConsistencyError:This PR also contains a small change to the
docker-compose.yml, but that is just an improvement since we got rid of the ridiculous log targetis_data_consistencyin favor of theshadow_data_consistencysimilar to the feature name.And yeah, merging this PR will break the existing ReadRPC dashboard, I am going to redo it afterward.