feat: Report MDBX commit latency metrics #5668

pakrentos · 2023-12-03T16:43:16Z

Implements an enhancement described in #5458

shekhirin

I like this a lot, really clean change! Just a few comments.

crates/storage/db/src/metrics.rs

shekhirin · 2023-12-05T13:44:26Z

crates/storage/db/src/implementation/mdbx/tx.rs

+            match this.inner.commit().map_err(|e| DatabaseError::Commit(e.into())) {
+                Ok((v, latency)) => (Ok(v), Some(latency)),
+                Err(e) => (Err(e), None),
+            }


this looks a bit weird, feels like it would be better to put the latency into Ok variant and make it non-optional. But then we'd need to replace the Ok variant in execute_with_close_transaction_metric. What do you think is better?

@shekhirin i'm not sure if it is ok to make it non-optional, since abort function does not provide any latencies.
We can however introduce a method to CommitLatency that requires self-invocation (thus, it must be called after passing an inner pointer to some FFI method). This change would make getter methods return Some(Duration) instead of None (so the getters' return type must also be altered). However, this could lead to less readable code due to the need for additional checks within each metric during record. To mitigate this, we could employ a useful macro.

Additionally, we could employ the proposed method to set an internal flag, checked during transaction metric recording. Yet, I'm concerned this might not be a best practice.

Each of these approaches would enable the initialization of a new CommitLatency struct within the abort function, allowing us to pass it as part of the Ok variant

ah, you're right about the abort method. Ok, current impl is fine with me!

…report_mbdx_metrics

shekhirin · 2023-12-13T17:18:47Z

crates/storage/db/src/implementation/mdbx/tx.rs

+            match this.inner.commit().map_err(|e| DatabaseError::Commit(e.into())) {
+                Ok((v, latency)) => (Ok(v), Some(latency)),
+                Err(e) => (Err(e), None),
+            }


ah, you're right about the abort method. Ok, current impl is fine with me!

shekhirin · 2023-12-14T10:21:20Z

LGTM, pending @mattsse

shekhirin · 2023-12-14T17:23:08Z

Verified that it works

ubuntu@reth3:~/reth$ curl -s localhost:9001 | grep '^reth_database_transaction_commit' | grep whole
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="commit",quantile="0"} 0.000015258
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="commit",quantile="0.5"} 0.000015256574364089644
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="commit",quantile="0.9"} 0.000015256574364089644
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="commit",quantile="0.95"} 0.000015256574364089644
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="commit",quantile="0.99"} 0.000015256574364089644
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="commit",quantile="0.999"} 0.000015256574364089644
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="commit",quantile="1"} 0.000015258
reth_database_transaction_commit_whole_duration_seconds_sum{mode="read-only",outcome="commit"} 0.00044248199999999985
reth_database_transaction_commit_whole_duration_seconds_count{mode="read-only",outcome="commit"} 29
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",quantile="0"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",quantile="0.5"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",quantile="0.9"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",quantile="0.95"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",quantile="0.99"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",quantile="0.999"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",quantile="1"} 0
reth_database_transaction_commit_whole_duration_seconds_sum{mode="read-only"} 0
reth_database_transaction_commit_whole_duration_seconds_count{mode="read-only"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="drop",quantile="0"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="drop",quantile="0.5"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="drop",quantile="0.9"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="drop",quantile="0.95"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="drop",quantile="0.99"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="drop",quantile="0.999"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-only",outcome="drop",quantile="1"} 0
reth_database_transaction_commit_whole_duration_seconds_sum{mode="read-only",outcome="drop"} 0
reth_database_transaction_commit_whole_duration_seconds_count{mode="read-only",outcome="drop"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",outcome="commit",quantile="0"} 0.05485251
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",outcome="commit",quantile="0.5"} 0.06518668725510515
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",outcome="commit",quantile="0.9"} 0.06667667989030134
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",outcome="commit",quantile="0.95"} 0.06667667989030134
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",outcome="commit",quantile="0.99"} 0.06667667989030134
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",outcome="commit",quantile="0.999"} 0.06667667989030134
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",outcome="commit",quantile="1"} 0.080699562
reth_database_transaction_commit_whole_duration_seconds_sum{mode="read-write",outcome="commit"} 1.2210061919999997
reth_database_transaction_commit_whole_duration_seconds_count{mode="read-write",outcome="commit"} 13
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",quantile="0"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",quantile="0.5"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",quantile="0.9"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",quantile="0.95"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",quantile="0.99"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",quantile="0.999"} 0
reth_database_transaction_commit_whole_duration_seconds{mode="read-write",quantile="1"} 0
reth_database_transaction_commit_whole_duration_seconds_sum{mode="read-write"} 0
reth_database_transaction_commit_whole_duration_seconds_count{mode="read-write"} 0

Add MDBX commit latencies to transaction metrics

1d099ce

pakrentos requested review from rakita and joshieDo as code owners December 3, 2023 16:43

onbjerg added C-enhancement New feature or request A-db Related to the database labels Dec 4, 2023

mattsse requested a review from shekhirin December 4, 2023 15:12

mattsse assigned shekhirin Dec 4, 2023

mattsse added the A-observability Related to tracing, metrics, logs and other observability tools label Dec 4, 2023

Merge branch 'main' into report_mbdx_metrics

c25bded

shekhirin reviewed Dec 5, 2023

View reviewed changes

Nikita Smirnov added 3 commits December 12, 2023 21:17

fix: update libmdbx-rs transaction tests

7f83809

fix: implement Duration calc for CommitLatency getters

d964805

Merge branch 'report_mbdx_metrics' of github.com:pakrentos/reth into …

0ecb2f9

…report_mbdx_metrics

shekhirin approved these changes Dec 14, 2023

View reviewed changes

shekhirin requested review from mattsse and rkrasiuk December 14, 2023 10:21

onbjerg approved these changes Dec 15, 2023

View reviewed changes

shekhirin added this pull request to the merge queue Dec 15, 2023

Merged via the queue into paradigmxyz:main with commit 667972c Dec 15, 2023
27 checks passed

pakrentos deleted the report_mbdx_metrics branch December 15, 2023 19:47

onbjerg mentioned this pull request Dec 19, 2023

Report MDBX commit latency metrics #5458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Report MDBX commit latency metrics #5668

feat: Report MDBX commit latency metrics #5668

pakrentos commented Dec 3, 2023

shekhirin left a comment

shekhirin Dec 5, 2023

pakrentos Dec 12, 2023

shekhirin Dec 13, 2023

shekhirin Dec 13, 2023

shekhirin commented Dec 14, 2023

shekhirin commented Dec 14, 2023

feat: Report MDBX commit latency metrics #5668

feat: Report MDBX commit latency metrics #5668

Conversation

pakrentos commented Dec 3, 2023

shekhirin left a comment

Choose a reason for hiding this comment

shekhirin Dec 5, 2023

Choose a reason for hiding this comment

pakrentos Dec 12, 2023

Choose a reason for hiding this comment

shekhirin Dec 13, 2023

Choose a reason for hiding this comment

shekhirin Dec 13, 2023

Choose a reason for hiding this comment

shekhirin commented Dec 14, 2023

shekhirin commented Dec 14, 2023