-
Notifications
You must be signed in to change notification settings - Fork 181
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
Add more pebble database metrics collection #2187
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2187 +/- ##
==========================================
+ Coverage 78.56% 78.79% +0.22%
==========================================
Files 102 102
Lines 9205 9383 +178
==========================================
+ Hits 7232 7393 +161
- Misses 1342 1361 +19
+ Partials 631 629 -2 ☔ View full report in Codecov by Sentry. |
32f672e
to
156f0e1
Compare
156f0e1
to
c2b2a8d
Compare
|
||
// collectMetrics periodically retrieves internal pebble counters and reports them to | ||
// the metrics subsystem. | ||
func (d *DB) StartMetricsCollection(ctx context.Context, refresh time.Duration) { |
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.
Do you think it would be worth adding a test for this?
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 don't think we need to. It's not easy to add tests for them as we have to create an actual pebbleDB instead of relying on in-memory. If it goes wrong, it shouldn't affect the main application, and most likely we will detect the weird behaviors when we see it on Grafana.
nWrite += int64(levelMetrics.BytesCompacted) | ||
nWrite += int64(levelMetrics.BytesFlushed) |
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.
Do we not need other fields like BytesMoved or BytesIngested?
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.
You raised a good point :)
BytesMoved
represents the bytes moved during compactions. Since we're already counting BytesCompacted, we don't need to include BytesMoved to avoid double-counting.
BytesIngested
represents bytes added through ingestion (e.g., bulk loading). We should include this in our nWrite calculation as it represents additional data written to the database.
c2b2a8d
to
a915ecf
Compare
This pull request is stale because it has been open 35 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
Adds the following metrics:
db_compaction_time
- Total time spent in database compactiondb_compaction_read
- Total bytes read during compactiondb_compaction_write
- Total bytes written during compactiondb_write_delay_count
- Total write delay counts due to database compactiondb_write_delay_time
- Total time spent in write stallsdb_disk_size
- Tracks the size of all of the levels of the databasedb_disk_write
- Measures the effective amount of data written to diskdb_mem_comp
- Tracks the amount of memory allocated for compactiondb_level0_comp
- Tracks the number of level-zero compactionsdb_nonlevel0_comp
- Tracks the number of non level-zero compactionsdb_seek_comp
- Tracks the number of table compactions caused by read operationsdb_manual_mem_alloc
- Tracks the amount of non-managed memory currently allocated