db/pebble: Extract Pebble's native metrics #1259
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1259 +/- ##
==========================================
+ Coverage 73.38% 74.07% +0.69%
==========================================
Files 101 81 -20
Lines 10320 8915 -1405
==========================================
- Hits 7573 6604 -969
+ Misses 2183 1815 -368
+ Partials 564 496 -68 ☔ View full report in Codecov by Sentry. |
|
I see that the overall metrics were refactored. Any changes you would want to see before I start adapting it to the new structure? I could also split this PR into smaller ones, each representing part of the |
thanks, that would be great. separate commits are good enough, no need for multiple PRs. |
Exca-DK
force-pushed
the
pebble-metrics
branch
from
9be4ebc
to
6ffe0f7
Compare
|
@omerfirmak Would mind giving it a try? Just |
- disk reporter - levels reporter - stalls reporter
Added additional reporters: - cache - flush - filter - memtable - keys - snapshot - table - wal
- Make counter vs gauges consistent - Increase meter interval
- small refactor - added unit test
Exca-DK
force-pushed
the
pebble-metrics
branch
from
c304a09
to
2e560bc
Compare
|
One thing to note: I added a hint about deriving some extra metrics from pebble events. Didn't go for that one in this PR as this PR is focused on just |
|
Sorry for the very late reply. I think most of the metrics pebble exposes are way too specific to make any sense for us. We definitely don't need metrics from all the levels. Let's do what geth does and only report a subset if you dont mind. |
|
Should be ready for review @omerfirmak preview of metrics in dashboard during initial sync: |
db/pebble/db_metrics.go
Outdated
| // Create storage for delta calculation. | ||
| var ( | ||
| compTimes [2]time.Duration | ||
| writeDelayCounts [2]uint64 | ||
| writeDelayTimes [2]time.Duration | ||
| compWrites [2]uint64 | ||
| compReads [2]uint64 | ||
| nWrites [2]uint64 | ||
| ) |
There was a problem hiding this comment.
I don't think you need to do this here, no? Delta calculation can be done on Grafana.
There was a problem hiding this comment.
I am not sure. The data provided by Pebble is the total cumulative so far, and that sounds to me like a case for Counter with delta instead of only ever-increasing gauge. For example, compWrites and compReads represent bytes written and read during compactions. I haven't personally seen a bytes-written metric represented as a gauge or, even worse, a counter that adds the total instead of the change. go-ethereum also reports the delta for that, but they use a slightly different collector called meter, which, from what I understand, accumulates data and resets itself after taking a snapshot.
I've been thinking about whether memComps, lvl0Comps, nonLvl0Comps, and seekComps shouldn't be represented as counters with a delta, too, for that reason. That would leave us with only three gauges in total: DiskSize, LevelFiles, and ManualMemAlloc. They represent the current value and can decrease or increase between snapshots.
There was a problem hiding this comment.
The data provided by Pebble is the total cumulative so far
That is entirely fine, prometheus counters are expected to monotonically increase. They shouldn't be decremented. Let's just export whatever pebble is reporting, I don't think we need to do delta calculation ourselves.
There was a problem hiding this comment.
Sure, I exported them as they are. Would mind sharing some insights as to why the total? I am also not sure if I shouldn't change the counters into gauges with that change.
|
I added changes that align with the behavior described in comment. |
|
Hey @joshklop, I noticed you added this to the milestone. Do you want me to resolve the conflicts since this PR has been idle for some time already? |
Hi @Exca-DK, that would be super helpful. Thanks for offering. I think @omerfirmak had some opinions about how we should move forward on this PR, so I'll re-request a review from him. |
Please do, we will try to get this merged soon. Sorry it took us so long:( |
closes #1243