RocksDB Statistics in Metrics #1169
Conversation
When we add the no-op metrics optimization this broke push-metrics.
# Conflicts: # metrics/src/test/java/tech/pegasys/pantheon/metrics/prometheus/PrometheusMetricsSystemTest.java
fix histogram name
# Conflicts: # services/kvstore/build.gradle # services/kvstore/src/main/java/tech/pegasys/pantheon/services/kvstore/RocksDbKeyValueStorage.java
| ((PrometheusMetricsSystem) metricsSystem) | ||
| .addCollector(ROCKSDB_STATS, histogramToCollector(histogram)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you wrap all of this new logic up in a utility class in the metrics package? That way we can reuse this for other RocksDB instances as needed. It looks like you could create a RocksDBStats class with methods getStatistics() and registerMetrics(MetricsSystem metrics, MetricsCategory category).
There was a problem hiding this comment.
If we keeps the statistics object in rocksdb and treat it like a part of the options then we can have RocksDBStats have just static methods.
|
|
||
| // Histograms - treated as prometheus summaries | ||
| static final HistogramType[] HISTOGRAMS = { | ||
| HistogramType.DB_GET, // COUNT : 4716170 SUM : 29514961 |
There was a problem hiding this comment.
What are all of these comments for?
There was a problem hiding this comment.
Previous measurements. I'm was trying to see if there were stats we should get rid of. But the performance latency impact of keeping all of them is almost unmeasurable.
| @@ -24,13 +24,14 @@ | |||
| PEERS("peers"), | |||
| PROCESS("process", false), | |||
| ROCKSDB("rocksdb"), | |||
| ROCKSDB_STATS("rocksdb", false), | |||
There was a problem hiding this comment.
Also - might be worth making these category names more accurate while you're in here. Perhaps: ROCKSDB_KVSTORE / ROCKSDB_KVSTORE_STATS?
There was a problem hiding this comment.
I went with KVSTORE_ROCKSDB in case we ever switch DBs it has good hierarchy: KVSTORE_MEMDB, KVSTORE_JDBC, etc.
| TickerType.NO_FILE_ERRORS, | ||
| // TickerType.STALL_L0_SLOWDOWN_MICROS, | ||
| // TickerType.STALL_MEMTABLE_COMPACTION_MICROS, | ||
| // TickerType.STALL_L0_NUM_FILES_MICROS, |
There was a problem hiding this comment.
I assume we want to keep these here for visibility of all of the options?
There was a problem hiding this comment.
yes, to make it clear their absence is intentional.
There was a problem hiding this comment.
(optional) Might be worth an explanatory comment :)
| @@ -0,0 +1,198 @@ | |||
| package tech.pegasys.pantheon.services.kvstore; | |||
There was a problem hiding this comment.
What do you think about moving this to: tech.pegasys.pantheon.metrics.rocksdb. We have a similar paradigm for vertx utilities in tech.pegasys.pantheon.metrics.vertx.
| }; | ||
|
|
||
| static void registerRocksDBMetrics( | ||
| final Statistics stats, final PrometheusMetricsSystem metricsSystem) { |
There was a problem hiding this comment.
If you move this to the metrics package and inject MetricsCategory, we can reuse this elsewhere. For example, in the RocksDbTaskQueue.
There was a problem hiding this comment.
That would give the metrics package a dependency on RocksDB, and then every package that uses metrics would drag that dependency along as deadweight.
There was a problem hiding this comment.
Yeah, thats a good point. And that's the case now with vertx. I guess the "right" thing to do would be to have independent vertxmetrics and rocksdbmetrics packages modules.
|
I will do the refactoring in a follow on PR because creating the module that will be needed has some long tendrils. |
Expose all the RocksDB statistics as metrics
Expose all the RocksDB statistics as metrics
PR description
Expose all the RocksDB statistics as metrics