feat: add logging session CSV export to Daqifi.Core#167
Merged
Conversation
Adds Daqifi.Core.Logging.Export namespace with a storage-agnostic CSV export pipeline: ILoggingSessionSource abstracts the data source (hiding EF/SQLite), LoggingSessionCsvExporter contains the pure formatting logic ported from daqifi-desktop's OptimizedLoggingSessionExporter. Supports absolute/relative timestamps, configurable delimiter, all-samples and rolling-average modes, IProgress<int> reporting, and cancellation. 29 xUnit tests on net8.0 and net9.0. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review Summary by QodoAdd streaming CSV exporter for logging sessions to Daqifi.Core
WalkthroughsDescription• Adds storage-agnostic CSV export pipeline to Daqifi.Core - ILoggingSessionSource abstracts data source (EF/SQLite/in-memory) - LoggingSessionCsvExporter contains pure formatting logic • Supports absolute/relative timestamps, custom delimiters, rolling-average modes • Includes 29 comprehensive xUnit tests covering all export scenarios • No EF Core, WPF, or Windows dependencies in new code Diagramflowchart LR
Source["ILoggingSessionSource<br/>(Data abstraction)"] -- "StreamSamples" --> Exporter["LoggingSessionCsvExporter<br/>(Pure formatting logic)"]
Options["CsvExportOptions<br/>(Delimiter, Time mode, Averaging)"] --> Exporter
Exporter -- "Writes CSV" --> Writer["TextWriter<br/>(CSV output)"]
Exporter -- "Reports progress" --> Progress["IProgress<int><br/>(0-100)"]
File Changes1. src/Daqifi.Core/Logging/Export/ILoggingSessionSource.cs
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+68
to
+76
| long? firstTicks = null; | ||
| long? currentTick = null; | ||
| var bucket = new List<SampleRow>(); | ||
| var processed = 0; | ||
|
|
||
| await foreach (var sample in source.StreamSamples(cancellationToken)) | ||
| { | ||
| firstTicks ??= sample.TimestampTicks; | ||
|
|
There was a problem hiding this comment.
1. Relative time ignores sessionstart 📎 Requirement gap ≡ Correctness
When UseRelativeTime is enabled, the exporter computes offsets from the first sample tick instead of ILoggingSessionSource.SessionStart, so exports can be incorrect when session start differs from the first sample timestamp.
Agent Prompt
## Issue description
`UseRelativeTime` offsets are computed from the first sample tick instead of the session start (`ILoggingSessionSource.SessionStart`). This can produce incorrect relative timestamps when there is a gap between session start and the first sample.
## Issue Context
The compliance requirement for relative timestamps is "seconds elapsed since session start".
## Fix Focus Areas
- src/Daqifi.Core/Logging/Export/LoggingSessionCsvExporter.cs[68-76]
- src/Daqifi.Core/Logging/Export/LoggingSessionCsvExporter.cs[137-160]
- src/Daqifi.Core/Logging/Export/ILoggingSessionSource.cs[9-13]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Disagree, with action — SessionStart has been removed from the interface in commit 6b9e333. The exporter intentionally derives relative time from the first observed tick in the stream because:
SessionStartwas never consulted by the export logic in the first place.- For storage-agnostic consumers (SD-card files, capture buffers, future CLI tools) there's often no meaningful "session start" distinct from the first sample, so requiring sources to populate it would have been ceremony.
The rename commit message captures the rationale: "the exporter uses the first observed tick from the stream for relative-time calculations and never consulted SessionStart."
If a future consumer ever needs offsets from a different anchor, they can normalize ticks before yielding them through ISampleSource.
Comment on lines
+16
to
+30
| private static async Task<(string[] lines, string header)> ExportToLinesAsync( | ||
| ILoggingSessionSource source, | ||
| CsvExportOptions options, | ||
| IProgress<int>? progress = null, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| var sw = new StringWriter(); | ||
| var exporter = new LoggingSessionCsvExporter(); | ||
| await exporter.ExportAsync(source, sw, options, progress, cancellationToken); | ||
| var content = sw.ToString(); | ||
| var lines = content.Split('\n', StringSplitOptions.RemoveEmptyEntries) | ||
| .Select(l => l.TrimEnd('\r')) | ||
| .ToArray(); | ||
| return (lines, lines.Length > 0 ? lines[0] : string.Empty); | ||
| } |
There was a problem hiding this comment.
2. No golden-file csv tests 📎 Requirement gap ☼ Reliability
The added tests assert individual behaviors but do not provide byte-identical golden-file coverage across the required options matrix, so exact output compatibility with the desktop exporter is not proven.
Agent Prompt
## Issue description
Required golden-file tests (byte-identical CSV output across the options matrix) are missing; current tests validate pieces of output rather than full-file byte equality.
## Issue Context
The goal is to prove backwards-compatible CSV output matching the desktop exporter for the same inputs (absolute vs relative time, averaged vs all-samples, mixed-channel sessions, timestamp gaps).
## Fix Focus Areas
- src/Daqifi.Core.Tests/Logging/Export/LoggingSessionCsvExporterTests.cs[16-464]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Defer / partially disagree — the test plan committed to in this PR is "29 xUnit tests covering the options matrix" (now 32 after my recent fix-pass), not byte-identical golden-file comparison. The unit tests cover the matrix Qodo cites: absolute vs relative time, averaged vs all-samples, mixed-channel sessions with gaps, invalid ticks (now in both time modes), custom delimiter, duplicate channel at same tick, progress, cancellation.
Adding desktop-byte-equivalence tests would also cement coupling to a specific desktop snapshot, which works against the design intent of this PR — making the exporter consumable by SD-card tooling, headless CLIs, and other contexts where bit-for-bit desktop parity isn't a goal. If a desktop-side regression test is wanted, the natural place is desktop's own test suite, asserting a frozen golden against CsvExporter output.
Happy to revisit if there's a concrete scenario where desktop's CSV output and core's must agree to the byte, but I don't think speculating on that scenario justifies the test infra now.
Comment on lines
+142
to
+183
| await foreach (var sample in source.StreamSamples(cancellationToken)) | ||
| { | ||
| firstTicks ??= sample.TimestampTicks; | ||
| lastTick = sample.TimestampTicks; | ||
|
|
||
| if (totals.ContainsKey(sample.ChannelKey)) | ||
| { | ||
| totals[sample.ChannelKey] += sample.Value; | ||
| counts[sample.ChannelKey]++; | ||
| } | ||
|
|
||
| windowCount++; | ||
| processed++; | ||
|
|
||
| if (windowCount >= window) | ||
| { | ||
| sb.Clear(); | ||
| sb.Append(FormatTimestamp(lastTick, firstTicks.Value, options.UseRelativeTime)); | ||
|
|
||
| foreach (var key in channelKeys) | ||
| { | ||
| sb.Append(options.Delimiter); | ||
| if (counts[key] > 0) | ||
| sb.Append((totals[key] / counts[key]).ToString("G")); | ||
| } | ||
|
|
||
| sb.AppendLine(); | ||
| await writer.WriteAsync(sb.ToString()); | ||
|
|
||
| foreach (var key in channelKeys) | ||
| { | ||
| totals[key] = 0.0; | ||
| counts[key] = 0; | ||
| } | ||
| windowCount = 0; | ||
|
|
||
| ReportProgress(progress, processed, totalSamples); | ||
| } | ||
| } | ||
|
|
||
| progress?.Report(100); | ||
| } |
There was a problem hiding this comment.
4. Averaging drops trailing samples 🐞 Bug ≡ Correctness
ExportAveragedAsync never flushes a final partial window, so any trailing samples when sampleCount % AverageWindow != 0 are silently omitted from the CSV.
Agent Prompt
### Issue description
`ExportAveragedAsync` only writes output when it has accumulated a full `AverageWindow` samples, and it does not write the last partial window at end-of-stream. This causes silent data loss for trailing samples.
### Issue Context
If there are 5 samples and `AverageWindow=2`, only 4 samples are represented (2 rows) and the last sample is dropped.
### Fix Focus Areas
- src/Daqifi.Core/Logging/Export/LoggingSessionCsvExporter.cs[142-183]
### Suggested fix
After the `await foreach` loop:
- If `windowCount > 0`, write one final averaged row using the accumulated `totals`/`counts` and `lastTick`.
- Optionally `ReportProgress(...)` after flushing so progress reflects the last samples.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Fixed in d4cb91a — ExportAveragedAsync now flushes a final partial window after the await foreach exits when windowCount > 0, so trailing samples that don't fill AverageWindow are no longer dropped. New test Export_AverageWindow_FlushesPartialFinalWindow covers the 5-samples-with-window-2 case Qodo described (writes 3 rows: two full windows + one partial holding only the final sample).
- Make WriteTimestampRow async (consistent with averaged path, avoids blocking thread pool on file I/O when buffer fills) - Gate GetSampleCountAsync behind progress != null (avoids unnecessary DB round-trip when caller doesn't need progress reporting) - Guard against AverageWindow <= 0 with ArgumentOutOfRangeException - Use CultureInfo.InvariantCulture for all number formatting to prevent broken CSV output in non-US locales - Switch progress test collections to ConcurrentBag (thread-safe) - Add Theory test covering AverageWindow 0, -1, -100 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Drop the desktop-flavored "Session" naming from the export types. The CSV exporter is consumed by anything streaming timestamped channel samples (SD-card log files, live capture, EF-backed sessions, etc.), not just desktop's session model. Renames: ILoggingSessionSource -> ISampleSource LoggingSessionCsvExporter -> CsvExporter InMemoryLoggingSessionSource (test fake) -> InMemorySampleSource Also drops the unused SessionStart property from the interface; the exporter uses the first observed tick from the stream for relative-time calculations and never consulted SessionStart. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
Contributor
Author
|
/agentic_review |
|
Persistent review updated to latest commit 6b9e333 |
- Averaging now flushes a final partial window when the sample count isn't a multiple of AverageWindow; previously trailing samples were silently dropped (Qodo #2). - INVALID({ticks}) fallback now applies in both absolute and relative time modes; previously relative mode skipped the invalid-tick check entirely and emitted a numeric offset for out-of-range ticks (#7). - ExportAsync now calls cancellationToken.ThrowIfCancellationRequested() before writing the CSV header, preventing header-only output for a pre-cancelled token (#8). - ArgumentOutOfRangeException for invalid AverageWindow now reports paramName as "options.AverageWindow" instead of "options" (#9). - Test for "G" formatting now compares against InvariantCulture, matching the exporter's invariant output and avoiding failures on de-DE etc. (#5). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Qodo review response (commit d4cb91a)For the items in Qodo's persistent review that don't have inline comment threads:
Test suite: 856 passing on net9.0 + net10.0 (was 853 before; 3 new tests added). |
This was referenced Apr 25, 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
1 participant
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
Closes #166.
Moves the streaming CSV export pipeline out of
daqifi-desktopand intoDaqifi.Coreso any consumer (desktop, CLI, future UIs) can reuse it without pulling in EF Core or WPF.ILoggingSessionSource— async-enumerable abstraction over session data; hides storage (EF/SQLite) from the exporterChannelDescriptor— identifies a channel by device name, serial number, channel name, and typeSampleRow— flat record:(TimestampTicks, ChannelKey, Value)CsvExportOptions—Delimiter,UseRelativeTime,AverageWindowLoggingSessionCsvExporter.ExportAsync— pure logic ported fromOptimizedLoggingSessionExporter:DateTime.ToString("O")) vs. relative (F3seconds) timestampsINVALID({ticks}))IProgress<int>(0–100) andCancellationTokensupportTest plan
Daqifi.Core.Tests/Logging/Export/on net8.0 and net9.0Microsoft.EntityFrameworkCore.*,System.Windows.*, orMicrosoft.Win32.*references in new code🤖 Generated with Claude Code