Wire external table reads into RocksDB metrics

[xingbowang]

Summary:
Wrap the filesystem passed to external table readers when statistics or file IO listeners are configured so reads performed by implementations such as Nimble update the same RocksDB metrics as regular SST reads. The wrapper records SST_READ_MICROS, activity-specific FILE_READ_* histograms, last/non-last-level and temperature read tickers, and forwards OnFileReadFinish/OnIOError to configured listeners so ZippyDB file-read counters observe external-table IO. MultiRead records the elapsed read histogram once and applies byte/listener accounting to each completed request.

Add a table test that makes a dummy external table reader read through ExternalTableOptions::fs and verifies both Statistics counters/histograms and file-read listener updates. Add an unreleased-history entry for the new external table read metrics coverage. This revision also applies the RocksDB make check-format clang-format output for external_table.cc.

Differential Revision: D108012449

[meta-codesync]

@xingbowang has exported this pull request. If you are a Meta employee, you can view the originating Diff in D108012449.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 363.3s.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit c16b8b2

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit c16b8b2

---

Summary

This PR adds metrics collection for external table reads by wrapping the FileSystem with instrumentation for statistics, histograms, and listener callbacks. The filesystem-wrapper approach is sound given that external tables bypass RandomAccessFileReader. No double-counting risk exists since the instrumented RandomAccessFileReader from table_cache.cc is discarded (file.reset()) before external table readers open their own files. The ExternalTableOptions::fs reference lifetime is also safe for construction-time-only usage.

High-severity findings (3):

• [external_table.cc] ExternalTableMetricsFileSystem stores listeners_ as a dangling-prone const& to the caller's vector; if the FS is kept alive past NewTableReader, a later NewRandomAccessFile call would dereference a dead vector.
• [external_table.cc] GetExternalTableFileReadHistogram duplicates GetFileReadHistograms from random_access_file_reader.cc verbatim; the shared logic should be factored into a common utility.
• [external_table.cc] Missing IOSTATS_ADD(bytes_read, size) in Read()/MultiRead(), breaking consistency with normal SST read accounting.

Full review (click to expand)

Findings

🔴 HIGH

H1. Listeners reference in ExternalTableMetricsFileSystem — external_table.cc

• Issue: ExternalTableMetricsFileSystem stores const std::vector<std::shared_ptr<EventListener>>& listeners_ as a const reference. While the ExternalTableMetricsRandomAccessFile constructor safely copies the listeners (filtering with ShouldBeNotifiedOnFileIO), the FS-level reference is unsafe if the FS object outlives the ImmutableOptions that owns the vector (e.g., if an external table reader copies the shared_ptr<FileSystem> and uses it after the DB options are destroyed).
• Root cause: Deviating from the established RandomAccessFileReader pattern which copies the listeners vector.
• Suggested fix: Copy the listeners vector in ExternalTableMetricsFileSystem's constructor, matching the RandomAccessFileReader pattern.

H2. Duplicated histogram mapping logic — external_table.cc:23-51

• Issue: GetExternalTableFileReadHistogram() is a verbatim copy of GetFileReadHistograms() in random_access_file_reader.cc. Two copies of the same switch logic must be maintained in sync.
• Root cause: The original function is file-local (inline in .cc), so it cannot be reused.
• Suggested fix: Extract into a shared header (e.g., monitoring/file_read_histograms.h) and use from both call sites.

H3. Missing IOSTATS_ADD(bytes_read, ...) — external_table.cc

• Issue: The normal SST read path calls IOSTATS_ADD(bytes_read, size) for per-thread IO statistics. The new wrapper omits this, causing external table reads to be invisible in iostats_context metrics.
• Suggested fix: Add IOSTATS_ADD(bytes_read, bytes) in RecordReadBytes() or RecordRead().

🟡 MEDIUM

M1. std::chrono::steady_clock instead of SystemClock

• Issue: Uses std::chrono::steady_clock::now() directly; all other RocksDB metrics use SystemClock. Non-mockable in tests and inconsistent.
• Suggested fix: Use SystemClock* from topts.ioptions.clock.

M2. Unconditional timing overhead

• Issue: Clock read on every Read()/MultiRead() even when statistics_ is null.
• Suggested fix: Guard with if (statistics_ != nullptr).

M3. Repeated ShouldNotifyListeners() calls

• Issue: Evaluated up to 3 times per Read(). Invariant within a call.
• Suggested fix: Cache in a local const bool.

M4. Test coverage gaps

• Issue: Missing coverage for MultiRead(), error/OnIOError path, activity-specific histograms, temperature tickers, is_last_level=true.

M5. Missing CreateDirIfMissing in test

• Issue: Test creates file in test::PerThreadDBPath(...) without ensuring directory exists.

🟢 LOW / NIT

L1. Read() records bytes_read on failure — benign 0-byte tick.

L2. ExternalTableMetricsFileSystem::Name() returns target name, hiding the wrapper.

Cross-Component Analysis

Concern	Verdict
Double-counting via table_cache.cc	Safe. Incoming RandomAccessFileReader is discarded (file.reset()).
ExternalTableOptions::fs dangling reference	Safe for construction-time use. Pre-existing struct design issue, not introduced by this PR.
is_last_level across contexts	Correct in all known calling contexts.
Only NewRandomAccessFile wrapped	Acceptable for external table read path.

Positive Observations

• Clean filesystem-wrapper approach for instrumenting external tables that bypass RandomAccessFileReader.
• ExternalTableMetricsRandomAccessFile correctly copies listeners during construction.
• MultiRead correctly records histogram once per batch, bytes/listeners per request.
• is_last_level formula handles all known calling contexts.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase