-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial metrics support #545
Conversation
At least Cc: #258 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No major complaints :)
Is there anything we can use to see what this produces externally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would hope that the RPC changes could be split off, done in a follow-up. I failed to see those were just really strangely diffed whitespace changes. Otherwise this looks good, would:
- consider changing adding in a metrics prefix right away since this will become part of the "stable api" and we don't want to go back and forth with it often
- would consider not initializing any recorders from
main
when monitoring addr is not specified, or at least commenting why it needs to be always added
I don't really know what would be a good way to handle the format!
away. Cheapest at runtime might be https://docs.rs/phf/latest/phf/ but hope you kept that BTreeMap bench :) But yeah, it's followup stuff.
f7b0834
to
3dc9ef7
Compare
Once labels are used to differentiate between counters for different method calls there is no need for |
Assuming monitoring is on port 9000 you can just: # TYPE rpc_method_calls_total counter
rpc_method_calls_total{method="starknet_getTransactionByBlockIdAndIndex"} 0
rpc_method_calls_total{method="starknet_getBlockTransactionCount"} 0
rpc_method_calls_total{method="starknet_getTransactionByHash"} 0
rpc_method_calls_total{method="starknet_getEvents"} 0
rpc_method_calls_total{method="starknet_getClassHashAt"} 0
rpc_method_calls_total{method="starknet_addDeployTransaction"} 0
rpc_method_calls_total{method="starknet_getStateUpdate"} 1
rpc_method_calls_total{method="starknet_blockNumber"} 0
rpc_method_calls_total{method="starknet_chainId"} 0
rpc_method_calls_total{method="starknet_blockHashAndNumber"} 0
rpc_method_calls_total{method="starknet_addInvokeTransaction"} 0
rpc_method_calls_total{method="starknet_getClassAt"} 0
rpc_method_calls_total{method="starknet_getBlockWithTxHashes"} 0
rpc_method_calls_total{method="starknet_syncing"} 0
rpc_method_calls_total{method="starknet_pendingTransactions"} 0
rpc_method_calls_total{method="starknet_call"} 0
rpc_method_calls_total{method="starknet_getStorageAt"} 0
rpc_method_calls_total{method="starknet_getClass"} 0
rpc_method_calls_total{method="starknet_getBlockWithTxs"} 0
rpc_method_calls_total{method="starknet_addDeclareTransaction"} 0
rpc_method_calls_total{method="starknet_getNonce"} 0
rpc_method_calls_total{method="starknet_estimateFee"} 0
rpc_method_calls_total{method="starknet_getTransactionReceipt"} 0
Or better download and run prometheus with a small addition to the config file (
|
b5cf45d
to
4fa1c15
Compare
Co-authored-by: Mirko von Leipzig <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Co-authored-by: Mirko von Leipzig <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Co-authored-by: Joonas Koivunen <joonas@equilibrium.co>
fc303b3
to
fb3946d
Compare
This PR is the second attempt at adding metrics to pathfinder (feature request issue, first attempt),
/metrics
route.RecorderGuard
should be used when installing a per-test recorder, which takes care of recorder removal and avoids races between tests (metrics
forces a global recorder instance per binary).format!()
to build the counter labels but this can be sped up ~5x if using aBTreeMap<&'static str, &'static str>
for method name vs counter name lookup instead (benchmark is disposable so not included in the PR). I haven't added this yet as I assumed that this extra heap allocation is negligible in terms of performance when taking the DB operations into account.