perf: performance stats about pebble HOT 11 CLOSED

@hueypark, @ajkr moving the discussion here from #403.

See cockroachdb/cockroach#36196 (comment) and cockroachdb/cockroach#36196 (comment) for @ajkr's thoughts on interesting RocksDB stats and metrics.

• rocksdb.number.superversion* tracks operations on the superversion which can be useful for debugging high memory/space usage as the superversion pins memtables and ssts so holding them too long can cause problems. The Pebble name for this data structure is readState.
• rocksdb.number.iter.skip, rocksdb.number.db.seek.found, rocksdb.number.db.next.found, rocksdb.number.db.prev.found can be useful for debugging slow reads. Skips will be relatively high when there are lots of tombstones.
• rocksdb.no.file.opens and rocksdb.no.file.closes can be useful for debugging slow reads. Opening files in the read path is slower than getting them from table cache. We do track table cache metrics in Metrics.TableCache. It seems worthwhile to maintain metrics on file opens and closes.

Other useful metrics that are already exposed by Pebble in some fashion:

• rocksdb.stall.micros can be useful for debugging write stalls. We do not need to track a specific metric in Pebble as this can be tracked externally via the WriteStall{Begin,End} events.
• rocksdb.bloom.filter.prefix.checked and rocksdb.bloom.filter.prefix.useful to answer whether your prefix bloom filters are effective. These are exposed as Metrics.Filter.{Hits,Misses}.
• rocksdb.block.cache.miss and rocksdb.block.cache.hit to answer whether the memory we're allocating to block cache is worthwhile. These are exposed as Metrics.BlocksCache.{Hits,Misses}.

Looking at the above, it seems there are only a handful of additional metrics to track. @ajkr did I miss anything from your source comments?

from pebble.

Yes that is all for ticker stats. I went through the histograms and tried to remember which ones had been useful to me before.

• "rocksdb.bytes.per.multiget" was useful to find out MultiGet()s were mostly for a single key, which was slower than Get().
• "rocksdb.read.num.merge_operands" was useful to find out reads were slow due to too many merge operands for a key.
• "rocksdb.compression.times.nanos" and "rocksdb.decompression.times.nanos" were useful to experiment with compression changes.
• Stats like "rocksdb.read.block.get.micros" and "rocksdb.table.open.io.micros" were useful for ruling out slow I/O.

Although, that is just ones that were useful to me. They were all useful to somebody at some point, otherwise they wouldn't have been added.

from pebble.

Thanks for the reviewing the RocksDB stats and histograms, @ajkr.

They were all useful to somebody at some point, otherwise they wouldn't have been added.

Is this true? I know I've been metrics in past projects which I've never found useful. I think it is very difficult to avoid doing this. And even if a metric was useful in the past, the problem it revealed might have been fixed and never reoccurred. I'm not anti-metric, I just want to have some pressure against adding every metric we can think of. (I realize you're not suggesting that).

from pebble.

I guess it probably isn't generally true. For example it looks like the blobdb stats were added en masse and it seems very unlikely they were all necessary at the start.

Even if it were generally true, certainly not all of them turned out relevant over time. For example one I added was the number of times some tricky optimization path triggered, as we wanted to drop that optimization but couldn't bring ourselves to do it without knowing if anyone's benefiting from it.

OK, guess I am in agreement we should be limited. I'll propose the following first steps:

• Add the missing tickers mentioned in #49 (comment).
• Pick a very specific set of more granular metrics. One I like are the timing measurements for things like table open, block read latencies, (de)compression times. Those can help us debug slow queries. If we choose those, there is still a question of whether to expose them as histograms, or on a per-query basis similar to RocksDB's perf context.

from pebble.

If we choose those, there is still a question of whether to expose them as histograms, or on a per-query basis similar to RocksDB's perf context.

I think ability to see stats per query would be more useful in this case. If only a small fraction of queries experience high latency, then overall histograms or CPU profiles will not clearly show where the problem is. Being able to selectively look at stats specific to the slow queries seems more useful.

from pebble.

@petermattis @ajkr

I know I've been metrics in past projects which I've never found useful.

I agree. In particular, the Go language provides a wonderful benchmark tool, so these metrics won't help the development process. But when I worked as an application developer, I solved the problem several times by sharing the database log with a professional database administrator. If a physically remote Pebble user complains of an unknown slowdown, we may ask the user to enable metrics and share the results.

Add the missing tickers mentioned in #49 (comment).

However, it is difficult to guess which logs will be usefull by future users of CockroachDB and Pebble, so I'm going to do what @ajkr suggests first. Would it be okay?

from pebble.

But when I worked as an application developer, I solved the problem several times by sharing the database log with a professional database administrator. If a physically remote Pebble user complains of an unknown slowdown, we may ask the user to enable metrics and share the results.

Yes, this is the motivation for such metrics. The problem with adding metrics haphazardly is that the metric collection itself can be a performance bottleneck.

However, it is difficult to guess which logs will be usefull by future users of CockroachDB and Pebble, so I'm going to do what @ajkr suggests first. Would it be okay?

Yes, though let's talk through the specifics of each one. For example, there might be something more specific than the rocksdb.number.superversion* metrics that can be done to track the overhead for readState references. And I'd like to better understand what the rocksdb.number.iter.skip and related metrics are tracking. The rocksdb.no.file.{opens,closes} seem straightforward and uncontentious.

from pebble.

Yes, though let's talk through the specifics of each one.

As you said, there is no reason to work with rocksdb.number.superversion*, rocksdb.number.iter.skip, rocksdb.no.file.{opens,closes} right away. It seems nice to extend pebble.Metrics later when better ideas come to mind or need arises.

By the way, I started this to fix issue cockroachdb/cockroach#42479. I think pebble.Metrics is suitable for solving the issue.

from pebble.

Apologies if there was confusion: I think we should add the equivalent of rocksdb.no.file.{opens,closes}. I'd add a new TableCacheMetrics struct that embeds a CacheMetrics along with the fields Opens and Closes:

type TableCacheMetrics struct {
  CacheMetrics
  Opens int64
  Closes int64
}

And then change tableCache.metrics() to return this new struct instead of CacheMetrics.

I think we should also add metrics around readState and extend Metrics.MemTable. Right now, Metrics.MemTable tracks the the memtables in DB.mu.mem.queue, ignoring the memtables that are pinned by readState references. It would be useful if we could figure out a low-overhead method of calculating the size and count of memtables that are pinned by a readState reference, but which have already been flushed (and thus removed from DB.mu.mem.queue). Similarly, it would be useful to track the size and count of sstables that have been compacted out of existence but not removed due to a readState reference. For the memtable tracking, I think this can be done by adding metrics for flushed but unreleased memtables. In DB.flush1, when a memtable is flushed we'd bump the count and size of flushed but unreleased memtables. And once the memtable is released we'd decrement these counts. The big challenge here is how to do this in a somewhat clean manner. memTable.readerUnref already has some logic for decrementing the mem accounting bytes when a memtable is released. Perhaps that could be extended. I haven't thought enough about how to do the tracking for sstables.

from pebble.

Thank you for the detailed explanation. I'll take a closer look at readState.

from pebble.

@hueypark I was looking at this more today for an unrelated reason. I think we should add counts and sizes of "zombie" memtables and sstables. The term zombie here is used to indicate that the memtables/sstables have been deleted, but still exist because they are referenced by an interator. Specifically, we'd add Metrics.MemTable.Zombie{Count,Size} and Metrics.Table.Zombie{Count,Size}.

Tracking the zombie memtables seems somewhat straightforward as described above. The zombie sstables seems a bit more difficult. Right now, there is a subtle dance that occurs in Version.Unref to keep track of obsolete files which are ready for deletion. I think what we could do is change BulkVersionEdit.Apply so that it adds to a map of zombie sstables that were deleted by the version edit. This map would be stored in versionSet.zombies. We'd remove sstables from this map when they are actually deleted on disk.

Cc @sumeerbhola to sanity check that the above makes sense.

from pebble.