Offline storage: Flush() persists EventPersistence_DoNotStoreOnDisk records to disk when the RAM tier is active
Nobody has claimed this yet.
Assessment
- Difficulty
- 5/5
- Estimated time
- Over a week
- Newbie friendliness
- 35/100
Research direction
Start by reading OfflineStorageHandler::Flush(), StoreRecord(), and the EventPersistence_DoNotStoreOnDisk enum behavior described here. Confirm the intended contract with maintainers before changing anything. Done means either the disk-only semantics are documented or renamed, or the no-disk contract is enforced during flush with a regression test.
Written by the indexing model from the issue text.
Description
Summary
OfflineStorageHandler::Flush() drains the entire in-memory queue and persists it to the disk backend in one batch:
auto records = m_offlineStorageMemory->GetRecords(false, EventLatency_Unspecified);
size_t totalSaved = m_offlineStorageDisk->StoreRecords(records);
This batch includes records whose persistence == EventPersistence::EventPersistence_DoNotStoreOnDisk. As a result, when the RAM tier is active (the normal configuration), a DoNotStoreOnDisk record that entered the in-memory queue can be written to disk on a memory-pressure flush and survive a process restart.
By contrast, OfflineStorageHandler::StoreRecord() explicitly skips such records — but only in the disk-only fallback branch (no RAM tier / shutdown):
else // m_offlineStorageMemory == nullptr || m_shutdownStarted
{
if (m_offlineStorageDisk != nullptr)
{
if (record.persistence != EventPersistence::EventPersistence_DoNotStoreOnDisk)
{
return m_offlineStorageDisk->StoreRecord(record);
}
}
}
When the RAM tier is present, StoreRecord() inserts every record into memory unconditionally (no persistence check), so DoNotStoreOnDisk records ride the normal RAM→disk flush path.
Pre-existing
This behavior is not introduced by the batched-flush change in #1491 — the pre-#1491 Flush() on main already called m_offlineStorageDisk->StoreRecords(records) on the full drained batch with no DoNotStoreOnDisk filter. This issue tracks the semantics question separately so #1491 stays focused.
The open question
The DoNotStoreOnDisk filter existing only in the disk-only fallback branch suggests the current flush-to-disk behavior may be intentional (i.e. the flag means "skip only when forced straight to disk without a RAM tier"). But the enum name implies these records should never be written to disk under any configuration. The two readings disagree.
Options for maintainers
- Treat current behavior as intentional and document/rename the enum to make the "disk-only fallback path" semantics explicit, so the name no longer implies a global no-disk contract.
- Enforce a global no-disk contract: during flush, partition
DoNotStoreOnDiskrecords out of the disk batch and re-insert them into the RAM queue (rather than draining + persisting them). This keeps them in memory until upload or drop, at the cost of possible RAM accumulation if they are never uploaded, and would need a regression test.
Surfaced during the #1491 review (Copilot). Filing as a follow-up per maintainer direction.
- Dominant language
- C
- Stars
- 102
- Forks
- 66
- Avg merge
- 3d 10h
- Merged PRs (30d)
- 10
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from microsoft/cpp_client_telemetry
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
microsoft/cpp_client_telemetry#1333 ·
-
C API enhancement
Difficulty 2/5 1-3 hours Newbie friendliness 62/100
-
bug
microsoft/cpp_client_telemetry#1437 · 1 assignee ·
-
bug
Difficulty 4/5 3-5 days Newbie friendliness 25/100
microsoft/cpp_client_telemetry#1413 · 2 comments ·
-
bug
Difficulty 4/5 3-5 days Newbie friendliness 35/100
microsoft/cpp_client_telemetry#1391 ·
All issues in microsoft/cpp_client_telemetry
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
-
level/task module/gcp type/bug
Difficulty 2/5 1-3 hours Newbie friendliness 85/100
-
Difficulty 1/5 Under an hour Newbie friendliness 86/100
hapostgres/pg_auto_failover#1190 ·
-
docs
Difficulty 1/5 Under an hour Newbie friendliness 85/100
-
P3 sonic-vpp
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
sonic-net/sonic-buildimage#29662 ·