feat(core): add minTimestamp and maxTimestamp columns to sys.telemetry_wal#6779
Merged
bluestreak01 merged 22 commits intomasterfrom Feb 28, 2026
Merged
Conversation
…y_wal Add minTimestamp and maxTimestamp columns to the sys.telemetry_wal table to record the data timestamp range for each WAL transaction event. For DATA and MAT_VIEW_DATA events, these columns contain the actual min/max timestamps from WalTxnDetails. For STRUCTURE_CHANGE and SQL events, they are set to Long.MIN_VALUE (NULL) since those events don't carry data timestamps. The schema migration is handled by adding a getExpectedColumnCount() method to the TelemetryType interface. When the existing table's column count doesn't match the expected count, the table is dropped and recreated. This is safe because telemetry_wal has a 1-week TTL and is non-critical data. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR enhances WAL telemetry by introducing min/max timestamp bounds to event tracking, integrating telemetry collection through WalWriter initialization, adding a new WAL_TXN_COMMITTED event type, expanding the telemetry_wal table schema with timestamp columns, and adjusting CairoEngine and ServerMain initialization to conditionally wire telemetry components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…y_wal Add minTimestamp and maxTimestamp columns to the sys.telemetry_wal table to record the data timestamp range for each WAL transaction event. For DATA and MAT_VIEW_DATA events, these columns contain the actual min/max timestamps from WalTxnDetails. For STRUCTURE_CHANGE and SQL events, they are set to Long.MIN_VALUE (NULL) since those events don't carry data timestamps. Enable WAL telemetry by default (regardless of the main telemetry setting) with a 1-week TTL. This reduces the dependency on logs to investigate data writing shape. Downgrade WalWriter commit log message from info to debug level, unless the commit has a replace range. Replace range commits remain at info level since they are less common and more important to track. The schema migration is handled by adding a getExpectedColumnCount() method to the TelemetryType interface. When the existing table's column count doesn't match the expected count, the table is dropped and recreated. This is safe because telemetry_wal has a 1-week TTL and is non-critical data. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…mmit-ts-to-wal-telemetry
Member
|
@CodeRabbit review critically please |
|
I'll provide line-cited findings with concrete follow-ups where issues are identified. 🧠 Learnings used✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/test/java/io/questdb/test/TelemetryTest.java (1)
401-419:⚠️ Potential issue | 🟡 MinorUse
assertQueryNoLeakCheck()for these SHOW CREATE TABLE assertions.This is a query result check and should follow the test helper guideline.
As per coding guidelines, Use assertQueryNoLeakCheck() to assert the results of queries in tests.♻️ Suggested change
- assertSql(header + ") timestamp(created) PARTITION BY MONTH" + end, showCreateTable); + assertQueryNoLeakCheck(header + ") timestamp(created) PARTITION BY MONTH" + end, showCreateTable); try (TelemetryJob ignore = new TelemetryJob(engine)) { - assertSql(header.replace("\tlatency FLOAT\n", "\tlatency FLOAT,\n") + + assertQueryNoLeakCheck(header.replace("\tlatency FLOAT\n", "\tlatency FLOAT,\n") + "\tminTimestamp TIMESTAMP,\n" + "\tmaxTimestamp TIMESTAMP\n" + ") timestamp(created) PARTITION BY DAY TTL 1 WEEK" + end, showCreateTable); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/io/questdb/test/TelemetryTest.java` around lines 401 - 419, The test uses assertSql(...) to validate the "SHOW CREATE TABLE" result inside TelemetryTest (using variables showCreateTable, header, end and within the TelemetryJob block); replace those assertSql calls with assertQueryNoLeakCheck(...) to follow the test helper guideline for query result checks—i.e., call assertQueryNoLeakCheck(expectedString, showCreateTable) for both the pre- and in-telemetry assertions (including the one inside the try (TelemetryJob ignore = new TelemetryJob(engine)) block) so the SHOW CREATE TABLE validations use the proper leak-checked test helper.core/src/main/java/io/questdb/cairo/wal/WalWriter.java (1)
117-136:⚠️ Potential issue | 🟡 MinorRe-sort the private final fields alphabetically (new fields are out of order).
This keeps the member block compliant with the required ordering.
As per coding guidelines, Java class members must be grouped by kind (static vs. instance) and visibility, and sorted alphabetically. When adding new methods or fields, insert them in the correct alphabetical position among existing members of the same kind.♻️ Example reordering
- private final MetadataValidatorService metaValidatorSvc = new MetadataValidatorService(); - private final MetadataService metaWriterSvc = new MetadataWriterService(); - private final WalWriterMetadata metadata; - private final Metrics metrics; - private final Telemetry<TelemetryWalTask> telemetryWal; - private final ObjList<Runnable> nullSetters; - private final RecentWriteTracker recentWriteTracker; - private final RowImpl row = new RowImpl(); - private final LongList rowValueIsNotNull = new LongList(); - private final BoolList symbolMapNullFlags = new BoolList(); - private final BoolList symbolMapNullFlagsChanged = new BoolList(); - private final ObjList<SymbolMapReader> symbolMapReaders = new ObjList<>(); - private final ObjList<DirectCharSequenceIntHashMap> symbolMaps = new ObjList<>(); - private final boolean walTelemetryEnabled; - private final TimestampDriver timestampDriver; - private final int timestampIndex; - private final ObjList<Utf8StringIntHashMap> utf8SymbolMaps = new ObjList<>(); - private final Uuid uuid = new Uuid(); + private final WalWriterMetadata metadata; + private final MetadataValidatorService metaValidatorSvc = new MetadataValidatorService(); + private final MetadataService metaWriterSvc = new MetadataWriterService(); + private final Metrics metrics; + private final ObjList<Runnable> nullSetters; + private final RecentWriteTracker recentWriteTracker; + private final RowImpl row = new RowImpl(); + private final LongList rowValueIsNotNull = new LongList(); + private final BoolList symbolMapNullFlags = new BoolList(); + private final BoolList symbolMapNullFlagsChanged = new BoolList(); + private final ObjList<SymbolMapReader> symbolMapReaders = new ObjList<>(); + private final ObjList<DirectCharSequenceIntHashMap> symbolMaps = new ObjList<>(); + private final Telemetry<TelemetryWalTask> telemetryWal; + private final TimestampDriver timestampDriver; + private final int timestampIndex; + private final ObjList<Utf8StringIntHashMap> utf8SymbolMaps = new ObjList<>(); + private final Uuid uuid = new Uuid(); + private final boolean walTelemetryEnabled;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/io/questdb/cairo/wal/WalWriter.java` around lines 117 - 136, The private final instance fields in class WalWriter are not alphabetically ordered; reorder the private final fields (e.g., AlterOperation alterOp, AtomicIntList initialSymbolCounts, BoolList symbolMapNullFlags, BoolList symbolMapNullFlagsChanged, DdlListener ddlListener, DirectCharSequenceIntHashMap entries in symbolMaps, LongList rowValueIsNotNull, MetadataService metaWriterSvc, MetadataValidatorService metaValidatorSvc, Metrics metrics, ObjList<MemoryMA> columns, ObjList<Runnable> nullSetters, ObjList<SymbolMapReader> symbolMapReaders, ObjList<DirectCharSequenceIntHashMap> symbolMaps, RecentWriteTracker recentWriteTracker, RowImpl row, Telemetry<TelemetryWalTask> telemetryWal, TimestampDriver timestampDriver, WalWriterMetadata metadata, boolean walTelemetryEnabled, IntList localSymbolIds) into alphabetical order among other private instance fields while keeping grouping by kind and visibility intact; locate these fields in WalWriter and move each declaration so the block is sorted alphabetically without changing modifiers or initializers.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/io/questdb/cairo/wal/WalWriter.java`:
- Around line 893-910: Empty WAL commits leave txnMinTimestamp as Long.MAX_VALUE
and txnMaxTimestamp as -1 which serializes to invalid timestamps; before calling
TelemetryWalTask.store (and before any logging that uses
txnMinTimestamp/txnMaxTimestamp) in WalWriter, normalize these sentinel values
to Numbers.LONG_NULL (e.g., if txnMinTimestamp == Long.MAX_VALUE set to
Numbers.LONG_NULL; if txnMaxTimestamp < 0 set to Numbers.LONG_NULL) so telemetry
and logs record NULLs instead of bogus values.
- Around line 117-136: The private final instance fields in class WalWriter are
not alphabetically ordered; reorder the private final fields (e.g.,
AlterOperation alterOp, AtomicIntList initialSymbolCounts, BoolList
symbolMapNullFlags, BoolList symbolMapNullFlagsChanged, DdlListener ddlListener,
DirectCharSequenceIntHashMap entries in symbolMaps, LongList rowValueIsNotNull,
MetadataService metaWriterSvc, MetadataValidatorService metaValidatorSvc,
Metrics metrics, ObjList<MemoryMA> columns, ObjList<Runnable> nullSetters,
ObjList<SymbolMapReader> symbolMapReaders, ObjList<DirectCharSequenceIntHashMap>
symbolMaps, RecentWriteTracker recentWriteTracker, RowImpl row,
Telemetry<TelemetryWalTask> telemetryWal, TimestampDriver timestampDriver,
WalWriterMetadata metadata, boolean walTelemetryEnabled, IntList localSymbolIds)
into alphabetical order among other private instance fields while keeping
grouping by kind and visibility intact; locate these fields in WalWriter and
move each declaration so the block is sorted alphabetically without changing
modifiers or initializers.
In `@core/src/main/java/io/questdb/TelemetryEvent.java`:
- Around line 62-65: Move the static short constant WAL_TXN_COMMITTED so the
WAL_* constants in the TelemetryEvent class are alphabetically ordered;
specifically, reorder the block containing WAL_APPLY_RESUME, WAL_APPLY_SUSPEND,
WAL_TXN_COMMITTED, and WAL_TXN_APPLY_START so WAL_TXN_COMMITTED appears in its
correct alphabetical position among the other WAL_ constants. Ensure the
constants remain grouped as static fields with the same visibility and update
only the ordering (no renames or value changes).
In `@core/src/test/java/io/questdb/test/cairo/wal/WalTelemetryTest.java`:
- Around line 85-91: Add coverage in WalTelemetryTest to verify non-DATA WAL
events render NULL/empty timestamps: in the WalTelemetryTest class, create and
emit a STRUCTURE_CHANGE or SQL/DDL event (e.g., an ALTER TABLE) that uses
Long.MIN_VALUE for minTimestamp/maxTimestamp, then assert the produced telemetry
rows for that event (identify by event type or walId/tableId used in existing
tests) show empty/null timestamp cells for minTimestamp and maxTimestamp rather
than populated values; update assertions alongside the existing DATA-event
checks for events 103/105/109 to include these non-DATA event rows.
In `@core/src/test/java/io/questdb/test/TelemetryTest.java`:
- Around line 401-419: The test uses assertSql(...) to validate the "SHOW CREATE
TABLE" result inside TelemetryTest (using variables showCreateTable, header, end
and within the TelemetryJob block); replace those assertSql calls with
assertQueryNoLeakCheck(...) to follow the test helper guideline for query result
checks—i.e., call assertQueryNoLeakCheck(expectedString, showCreateTable) for
both the pre- and in-telemetry assertions (including the one inside the try
(TelemetryJob ignore = new TelemetryJob(engine)) block) so the SHOW CREATE TABLE
validations use the proper leak-checked test helper.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/io/questdb/test/cairo/wal/WalTelemetryTest.java`: - Around line 85-91: Add coverage in WalTelemetryTest to verify non-DATA WAL events render NULL/empty timestamps: in the WalTelemetryTest class, create and emit a STRUCTURE_CHANGE or SQL/DDL event (e.g., an ALTER TABLE) that uses Long.MIN_VALUE for minTimestamp/maxTimestamp, then assert the produced telemetry rows for that event (identify by event type or walId/tableId used in existing tests) show empty/null timestamp cells for minTimestamp and maxTimestamp rather than populated values; update assertions alongside the existing DATA-event checks for events 103/105/109 to include these non-DATA event rows.core/src/test/java/io/questdb/test/cairo/wal/WalTelemetryTest.java (1)
85-91: Test covers DATA events but not the NULL-timestamp path for non-data events.The expected output correctly verifies that
minTimestampandmaxTimestampare populated for DATA events (103, 105, 109). However, there's no coverage for STRUCTURE_CHANGE or SQL events whereLong.MIN_VALUEis passed and should render asNULL(or empty timestamp). Consider adding a test that includes anALTER TABLEor similar DDL to verify those rows show NULL/empty timestamps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/io/questdb/test/cairo/wal/WalTelemetryTest.java` around lines 85 - 91, Add coverage in WalTelemetryTest to verify non-DATA WAL events render NULL/empty timestamps: in the WalTelemetryTest class, create and emit a STRUCTURE_CHANGE or SQL/DDL event (e.g., an ALTER TABLE) that uses Long.MIN_VALUE for minTimestamp/maxTimestamp, then assert the produced telemetry rows for that event (identify by event type or walId/tableId used in existing tests) show empty/null timestamp cells for minTimestamp and maxTimestamp rather than populated values; update assertions alongside the existing DATA-event checks for events 103/105/109 to include these non-DATA event rows.
Member
Code Review1. Alphabetical ordering violation in
|
- Normalize txnMinTimestamp/txnMaxTimestamp to Numbers.LONG_NULL for empty commits before emitting telemetry (avoids bogus Long.MAX_VALUE/-1 values) - Sort WAL_TXN_COMMITTED constant alphabetically in TelemetryEvent - Sort telemetryWal and walTelemetryEnabled fields alphabetically in WalWriter - Replace assertSql with assertQueryNoLeakCheck in TelemetryTest Co-Authored-By: Claude Opus 4.6 <[email protected]>
Collaborator
Author
|
Addressed the review comments in 51e06a2: Fixed (inline comments):
Fixed (outside diff range comments):
Skipped (nitpick):
|
- Fix "telementry" typo and run-on sentence in WalWriter commit comment - Use Numbers.LONG_NULL instead of Long.MIN_VALUE for null timestamp sentinel in ApplyWal2TableJob to make intent explicit - Add comment explaining destructive schema migration in Telemetry.java - Fix tab style inconsistency in WalTelemetryTest (use literal tabs consistently within the same method) - Remove unrelated //noinspection resource from ServerMain.java Co-Authored-By: Claude Opus 4.6 <[email protected]>
ideoma
added
WAL
Core
Collaborator
Author
|
Addressed all items from #6779 (comment) in c5fc08a:
|
Replace the sentinel-based checks (Long.MAX_VALUE, < 0) with a simple txnRowCount > 0 guard. The previous < 0 check would suppress legitimate negative (pre-epoch) timestamps. Add a test for empty replace range commits to verify NULL timestamps are emitted when row count is 0. Co-Authored-By: Claude Opus 4.6 <[email protected]>
bluestreak01
previously approved these changes
Feb 24, 2026
bluestreak01
approved these changes
Feb 28, 2026
Contributor
[PR Coverage check]😍 pass : 33 / 33 (100.00%) file detail
|
maciulis
pushed a commit
to maciulis/questdb
that referenced
this pull request
Mar 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
minTimestampandmaxTimestampTIMESTAMP columns tosys.telemetry_walto capture the data timestamp range per WAL transaction eventTest plan
WalTelemetryTest#testTelemetryWalto verify new columns appear with correct min/max timestamps for DATA events and NULL for non-data eventsmvn clean package -DskipTests -pl coresys.telemetry_waland confirmminTimestamp/maxTimestampare populated for data events🤖 Generated with Claude Code