Add --diagnostics: a per-request diagnostic stream for all output formats

[FrankRay78]

Summary

Add a --diagnostics switch that writes low-level detail about how a speed test ran to a stream separate from the result, in every output format. The result on stdout is unchanged whether or not the switch is present, so a user can capture diagnostics alongside piped CSV or JSON without corrupting it.

The goal is that a single run with --diagnostics produces everything needed to raise a good bug report — in particular, why individual requests failed, which the code currently discards.

This is the second of two issues that replace #256. It depends on the switch surface settled by #267.

Motivation

Diagnosing #245 — upload silently reporting 0 bps against HTTPS-migrated Ookla servers — took four released versions, a dozen screenshots and a hand-patched debug build to find something one line of diagnostic output would have named immediately.

The information exists and is thrown away. In OoklaSpeedtest.cs:340-356, every per-request exception — transport error, TLS failure, timeout, non-success HTTP status — is caught, converted into an increment of failedCount, and discarded. #222 added those counts, which is what made the symptom visible at all; this issue is about making the cause visible too.

The switch this replaces was no help. --verbosity Debug affected only the default console writer, so --csv and --json — the modes intended for scripting and for pasting into a bug report — had no diagnostics at all, and what it did add was a little extra prose about the result rather than information about how the test ran.

NetPace writes nothing to stderr today; everything goes through a single console. Introducing a second output stream is the substantive part of the Console-side work, and the test host does not yet capture one.

Users & jobs

A user hitting a network problem they cannot explain, who wants to attach something useful to a bug report. They need one run, with one extra switch, to produce a complete picture — not a back-and-forth of screenshots.

A maintainer triaging that report. They need to see which server was used and why it was chosen, which requests failed and for what reason, and enough environment detail to reproduce, without asking for a second run.

A script author already piping --csv or --json into another tool, who wants to keep a diagnostic trail on the side without touching their parsing.

Capability

A separate diagnostic stream

When --diagnostics is present, NetPace writes detail about the run to a stream distinct from the one carrying the result, so the two can be captured and redirected independently:

netpace --json --diagnostics > result.json 2> diagnostics.log

The result stream is byte-identical with and without the switch, in every output format. Nothing about the speed test result changes; the switch only adds a second stream.

When the switch is absent, nothing is written to the diagnostic stream.

What the diagnostics contain

Enough to reconstruct the run without asking the user for anything else:

• When the test ran, and how NetPace was invoked.
• What NetPace was running as and on — version, runtime, operating system.
• Which server was used, and how it came to be chosen: auto-selected by latency, taken first from the list, or specified by the user.
• Which of the latency, download and upload tests ran.
• For each request: which request it was, when it happened, what it targeted, whether it succeeded or the reason it failed, how many bytes moved, and how long it took.

Every request gets its own record — none are dropped or summarised. A run issues between 2 requests on the fastest profile and roughly 456 on the heaviest, so a complete log is at most a few hundred lines; trimming would save almost nothing and risks discarding the single failed request that explains the problem.

The failure reason is the point of the exercise. A run against a server that rejects every upload must produce diagnostics from which the rejection cause is identifiable — that is the #245 case, and it is the acceptance bar for this feature.

Interactive use

Running with --diagnostics at a terminal must not corrupt the live progress display. There is direct evidence from #245 that naive interleaving does exactly that: temporary instrumentation shredded the progress bar into Uploading ---- 13%ly closed by the remote host...

Out of scope

• Writing diagnostics to a file path given as an option value is not in this issue. Stream redirection already covers it.
• Diagnostics for anything other than a speed test run — server listing, help, version — are not in this issue.
• Changing what the counts in the normal result output show is not in this issue; Surface Transfer Failures #222 settled that.
• Credential redaction is not in this issue. No CLI option carries a secret today, and diagnostics record the invocation and per-request URLs rather than effective settings, so there is nothing to leak. See Open questions for the tripwire that would change that.
• Retiring --verbosity and adding --minimal are not in this issue; that is Retire --verbosity in favour of a --minimal output-format switch #267.

Acceptance criteria

• For every output format, the result stream produced with --diagnostics is byte-identical to the result stream produced by the same invocation without it.
• With --diagnostics, the result stream and the diagnostic stream can be redirected to different destinations, and each contains only its own content.
• Without --diagnostics, no diagnostic content is produced on any stream.
• --diagnostics produces diagnostics in every output format, including the compact, CSV and JSON formats.
• Diagnostics from a run identify when the test ran, how NetPace was invoked, the NetPace version, the runtime and the operating system.
• Diagnostics identify the server used and which selection route chose it.
• Diagnostics identify which of the latency, download and upload tests ran.
• Diagnostics record each request individually, including whether it succeeded and — where it did not — a reason attributable to that request.
• Given a run in which every request of a test fails, the diagnostics allow the failure cause to be identified without re-running. (Regression bar for Upload Redirect 307 #245.)
• Running interactively with --diagnostics leaves the live progress display intact and readable.
• --help lists --diagnostics; README's --help block and USER_GUIDE describe it, including a redirection example.

Both streams are verified against committed snapshots per the testing conventions in CLAUDE.md. Timestamps must come from the existing IClock stub rather than wall-clock time, or the snapshots become non-deterministic.

---

Technical notes

Where it lives

• The substantive work is in NetPace.Core, not the Console. Per-request detail does not currently exist to be rendered — the catch block at OoklaSpeedtest.cs:340-356 increments a counter and discards the exception. Core has to surface these events before the Console can format them; the formatting is the easy half.
• The existing IProgress<SpeedTestProgress> channel is one candidate carrier, but that is a suggestion, not a decision — see Open questions.
• Console side: NetPace resolves a single IAnsiConsole from DI throughout Program.cs. A second, error-stream console has to be introduced and threaded to wherever diagnostics are emitted.

Constraints / assumptions

• Test seam is a prerequisite, not a detail. CommandLineTestHost registers one TestConsole and returns a single Output string. There is no way to capture or snapshot a second stream today. Until that is extended, none of this is testable, and Constitution Principle I is non-negotiable — so this is the first thing to build, before any production code.
• Do not write to the diagnostic stream inside the measured window. Console I/O per request risks perturbing the numbers being measured, and is the mechanism behind the shredded progress bar in Upload Redirect 307 #245. Buffer and flush after the progress display completes.
• Keep it trim-safe. NetPace targets AOT-trimmable builds; environment and version detail must be gathered without runtime reflection.
• A full run issues many requests. A per-request record for every one of them may make the diagnostic stream large — see Open questions.

Unknowns

• Surfacing per-request events will change the NetPace.Core public surface. The maintainer has approved that change in principle, so CLAUDE.md's "discuss public-API changes first" gate is satisfied for this issue. What remains is the shape of the change and its semver consequence for NuGet consumers, settled by the carrier decision below.

---

Open questions / future work

• Carrier for per-request events in Core — recommendation posted, awaiting ratification. The design comment recommends a second IProgress<RequestDiagnostic> on new overloads, chosen because three of the four output writers pass no progress reporter and --diagnostics must work in every format. Stays a MINOR bump. Ratify before implementing.
• Failure reason on the wire. Decided — RequestDiagnostic carries the reason as a string plus the exception type name, rather than the Exception itself, keeping exception objects off the public NuGet surface.
• Diagnostic record format. Decided — one key=value record per line. See the decision comment below.
• Volume. Decided — every request gets a record, none dropped or summarised. See Capability.
• Credential redaction — deferred, with a tripwire. Out of scope here because nothing NetPace records today can carry a secret. Revisit the moment either becomes true: a CLI option is added that takes a credential, or diagnostics start reporting effective settings or profile state (OoklaSpeedtestSettings.ProxyCredential is where a proxy password actually lives). Until then there is no secret in the output to redact.

Related

• Retire --verbosity in favour of a --minimal output-format switch #267 — the companion issue retiring --verbosity and adding --minimal. Settles the switch surface this feature attaches to.
• Rework --verbosity Debug into a stderr diagnostic channel for all output formats #256 — the original proposal both issues split out of, now retired.
• Upload Redirect 307 #245 — the bug whose diagnosis this would have made trivial; the regression bar for this feature.
• Surface Transfer Failures #222 — added the failure counts that made the symptom visible at all.
• CLAUDE.md — testing conventions, the snapshot review rule, and the public-API discussion requirement.
• .specify/memory/constitution.md — Principle I (TDD), Principle III (CLI Excellence), Principle IX (Behavioural Specification).

[FrankRay78]

Decision: diagnostic record format

One key=value record per line (logfmt style). This resolves the "diagnostic record format" open question in the issue body.

The format

Every line is one record. First field is always ts, second is always event; the remaining keys are fixed per event type and always appear in the same order.

ts=1980-01-01T10:05:00.000 event=run.start version=1.0.0 runtime=".NET 10.0.0" os="Linux 6.8.0-137-generic" arch=x64
ts=1980-01-01T10:05:00.000 event=run.invocation args="--json --diagnostics --server 12345"
ts=1980-01-01T10:05:05.000 event=server.selected id=12345 sponsor="Foo Telecom" location="London, GB" host=speedtest.foo.example:8080 selection=auto-latency latency_ms=12
ts=1980-01-01T10:05:10.000 event=test.start test=download
ts=1980-01-01T10:05:15.000 event=request test=download seq=1 url="https://speedtest.foo.example:8080/download?size=25000000" status=ok bytes=2500000 duration_ms=412
ts=1980-01-01T10:05:20.000 event=request test=upload seq=1 url="https://speedtest.foo.example:8080/upload" status=failed bytes=0 duration_ms=1203 reason="The SSL connection could not be established."
ts=1980-01-01T10:05:25.000 event=test.end test=upload requests=8 succeeded=0 failed=8 bytes=0 duration_ms=9604
ts=1980-01-01T10:05:30.000 event=run.end exit=0

Rules that make it a stable contract:

• Quoting — a value is bare unless it contains a space, =, or ", in which case it is wrapped in " with embedded " escaped as \".
• Always one line per record — newlines and carriage returns inside a value (exception messages are frequently multi-line) are escaped as \n / \r. Never emit a raw newline mid-record.
• No column alignment or padding. Single space between fields. Padded columns would make one long URL reflow the whole block in a .verified.txt diff, which is the same reason the codebase avoids aligned whitespace in source.
• Every line is self-describing. A grepped subset stays meaningful without its header — that is the point of repeating test= and ts= on each request record.
• Timestamps come from IClock, formatted yyyy-MM-ddTHH:mm:ss.fff, and are deliberately not affected by --datetime-format. That option shapes the result, and diagnostics must stay stable regardless of how a user has formatted their output. The example above uses the values ClockStub produces, so it doubles as a sketch of what a snapshot looks like.
• event is a closed vocabulary. Adding a new event type or a new key is an additive change; renaming or removing one is a breaking change to the diagnostic contract.

Why this over the alternatives

Versus JSON Lines. NDJSON is strictly better for machine parsing, and AOT-safety is not an argument against it — the codebase already has source-generated contexts in NetPace.Console/JsonResultContext.cs, so a new one per record type is routine. The reasons it still loses:

• The primary consumer is a human pasting into a GitHub issue. NDJSON is materially harder to skim than logfmt, and a run produces many request records.
• It would create a second, unrelated JSON contract sitting alongside --json. Two JSON shapes in one product invites "why doesn't the diagnostic JSON match the result JSON?" forever.
• logfmt recovers most of the machine-readability anyway: grep 'status=failed' is the entire triage workflow for the Upload Redirect 307 #245 class of bug, with no parser at all.

Versus human-readable prose. Not greppable in any stable way, and it invites drift — every new field becomes a new sentence to word, and the snapshot churns on rewording rather than on behaviour.

Consequence for the redaction rule

Writing this out sharpened the redaction question, and corrected an assumption inherited from #256.

#256 justified redaction by pointing at "full CLI switches" leaking a proxy password. That is aimed at the wrong surface: proxy credentials are not CLI switches at all — they live in OoklaSpeedtestSettings.ProxyCredential (src/NetPace.Core/Clients/Ookla/OoklaSpeedtestSettings.cs), reached through configuration and profiles.

So redaction has two surfaces, not one:

1. event=run.invocation — the recorded command line. Today this carries no credential-bearing option, so the risk here is latent rather than live.
2. Any record that reports effective settings or profile state — where the proxy credential actually lives.

The safest shape given that split: never emit a credential-bearing value at all, and where its presence is diagnostically useful, report only whether one was configured (proxy_credential=set / proxy_credential=none) rather than a masked placeholder for the value. "Was a proxy credential in play?" is the question a triager actually needs answered; the value itself never needs to leave the machine.

The remaining open question stands: whether the rule is a named set of fields or a value-shape heuristic. A named set is easier to reason about but silently misses any credential-carrying option added later — worth a deliberate call during implementation.

[FrankRay78]

Decisions: volume, redaction, and where the carrier question stands

Volume — decided: every request gets a record

No cap, no summarising. A run issues 2 requests on the fastest profile and roughly 456 on the heaviest (5 sizes × 40 iterations download, 16 × 16 upload), so the largest possible log is a few hundred lines. Trimming saves almost nothing and risks discarding the one failed request that explains the problem, which is the entire reason the feature exists.

Credential redaction — deferred

Dropped from this issue's scope, and the corresponding acceptance criterion removed. No CLI option takes a credential today, and diagnostics record the invocation and per-request URLs rather than effective settings — so there is currently no secret in the output to redact. Building a redaction mechanism now would be guarding against a case NetPace does not have.

This supersedes the "Consequence for the redaction rule" section of the format-decision comment above. The tripwire stated there still stands and is now recorded in Open questions: revisit if a CLI option is added that takes a credential, or if diagnostics ever start reporting effective settings or profile state, since OoklaSpeedtestSettings.ProxyCredential is where a proxy password actually lives.

Carrier for per-request events — still open

Not settled, and worth more thought before it is. Two things shape it.

A diagnostic record is assembled from both sides. Some of what a record needs is only known in the Console app — the command line as invoked, the NetPace version and environment, and how the server was chosen (ServerSelector is Console-side). The rest is only known in Core. So whatever the carrier turns out to be, the diagnostic writer is a Console-side assembler consuming a Core-side event stream; the carrier choice does not change that shape, only what the Core half looks like.

The existing progress channel is already per-request in cadence. Confirmed in GenericTestSpeedAsync — ReportProgress fires once per completed request, inside the lock, whether that request succeeded or failed. That is a genuine point in favour of reusing it, and it undercuts the "two unrelated ideas" objection: it is already an event-per-request stream that happens to be summarised as a percentage.

Three facts that complicate reusing it, all verified against the current code:

1. The payload is cumulative, not per-request. SpeedTestProgress carries PercentageComplete, total BytesProcessed and total ElapsedMilliseconds. It has no request identity, URL, per-request byte count or per-request duration. Carrying diagnostics here means adding those fields, not just an exception — and a record that mixes running totals with single-request facts is easy to misread.
2. Byte-cap-excluded requests report nothing. Progress is only reported when !cts.IsCancellationRequested, so once the byte budget trips, in-flight requests complete silently. Those are exactly the requests a diagnostic log would want to account for.
3. Three of the four output writers do not pass a progress reporter at all. MinimalConsoleWriter, CSVConsoleWriter and JsonConsoleWriter call the progress-less GetDownloadSpeedAsync(server, ct) overloads; only DefaultConsoleWriter supplies one. Since --diagnostics must work in every output format, riding on IProgress would force those three modes to construct a progress reporter purely to receive diagnostics — in modes that deliberately render no progress display.

Fact 3 is the sharpest of the three: it is the one that turns "reuse the existing channel" from the cheaper option into an option with its own plumbing cost.

For reference on the semver consequence: SpeedTestProgress is a sealed record with init-only properties, so adding properties to it is additive and existing NuGet consumers keep compiling. A separate channel stays additive too, provided it arrives as new overloads rather than by changing the signatures of the existing ISpeedTestService methods, which implementers outside this repo would have to follow.

[FrankRay78]

Recommended design: the diagnostic writer

Grounded in the current code. Everything below is a recommendation to ratify before implementing, except where marked decided.

1. It is not a fifth IConsoleWriter

IConsoleWriter selects one of four mutually-exclusive result renderers. Diagnostics are orthogonal — they must work alongside whichever one the user picked. Model it as a recorder that all four writers feed, not as a writer competing with them.

2. Do not emit diagnostics through IAnsiConsole

This is the constraint most likely to be discovered late, and it falls directly out of the one-record-per-line format decision.

IAnsiConsole wraps at terminal width — which is exactly why CommandLineTestHost sets .Width(int.MaxValue). A logfmt record carrying a full Ookla URL comfortably exceeds 80 columns, so in a real terminal Spectre would fold it and silently break the one-record-per-line contract that makes grep 'status=failed' the whole triage workflow.

Write diagnostics to a TextWriter instead — System.Console.Error in production, a StringWriter in tests. No width handling, no markup parsing, and no risk of a [ inside an exception message being interpreted as Spectre markup. FileConsole is precedent for a second output target, but it is the wrong precedent here.

3. The recorder

internal interface IDiagnosticRecorder
{
    void Record(string eventName, params (string Key, string? Value)[] fields);
    void Flush(TextWriter writer);
}

Two implementations, null-object pattern: NullDiagnosticRecorder (no-op, registered when --diagnostics is absent) and BufferingDiagnosticRecorder. This keeps if (diagnostics is not null) out of every call site and makes the switched-off case genuinely free.

Two properties it must have:

• Thread safety. Requests run up to 32-way parallel, so Record is called concurrently. ConcurrentQueue plus an Interlocked.Increment sequence number assigned at record time.
• Deterministic ordering. Sort by that sequence number, not by timestamp. ClockStub returns a constant, so timestamp ordering would be unstable in snapshots.

Timestamp formatting, logfmt quoting and newline escaping all live in the recorder, so the format contract has exactly one implementation to test and one place to change.

4. Carrier — recommendation

Add a second IProgress<T> on new overloads:

Task<SpeedTestResult> GetDownloadSpeedAsync(
    IServer server,
    IProgress<SpeedTestProgress>? progress,
    IProgress<RequestDiagnostic>? diagnostics,
    CancellationToken cancellationToken = default);

Why this rather than extending SpeedTestProgress:

• It lets the three progress-less writers (MinimalConsoleWriter, CSVConsoleWriter, JsonConsoleWriter) pass null for progress and still receive diagnostics. That is the deciding factor — --diagnostics has to work in every output format, and those three deliberately render no progress display.
• SpeedTestProgress keeps meaning progress; its cumulative fields are not muddled with single-request facts.
• It reuses a BCL abstraction already used in this codebase, and stays trim-safe.
• New overloads rather than changed signatures keeps this a MINOR bump for anyone implementing ISpeedTestService outside this repo.

Use the existing SyncProgress<T> pattern, not Progress<T> — Progress<T> posts through the synchronization context, which would reorder diagnostic records. It is currently a private nested class at NetPace.Console/ConsoleWriters/DefaultConsoleWriter.cs:144 and needs promoting to an internal top-level type.

Decided: RequestDiagnostic carries the failure reason as a string, plus the exception type name as a separate field. Passing the Exception itself would give the Console side more freedom, but it would put an exception object on the public NuGet surface for no benefit a triager can use.

5. Flush once, in SpeedTestCommand

Flush after writer.PerformSpeedTestAsync returns. One place, covers all four writers, and it is after the Spectre progress display has completed — which is what prevents the interleaving that shredded the progress bar in #245.

Put the flush in a finally: a run that throws an operational fault is precisely when the diagnostics matter most. Under --loop, flush per iteration so the buffer does not grow unbounded and the user sees records as the loop runs.

6. Two Console-side refactors needed first

• ServerSelector does not report which route it took. It has four branches — first-in-list, user-specified without latency, auto-by-latency, and user-specified with latency — and returns a bare LatencyTestResult that cannot distinguish them. It needs to return the route alongside the result, or the selection= field cannot be populated honestly.
• Invocation and environment are Program-side. args, the assembly version (already computed for --version in Program.RunAsync), and RuntimeInformation.FrameworkDescription / OSDescription / ProcessArchitecture — none currently used anywhere, all AOT-safe with no reflection. Record run.start and run.invocation in Program.RunAsync after parsing.

7. Test seam

Register the diagnostic TextWriter in DI. CommandLineTestHost supplies a StringWriter, and TestResult gains a DiagnosticOutput property alongside Output. Existing snapshots are untouched by this.

For the "result stream is byte-identical with and without --diagnostics" criterion, use a targeted assertion comparing two runs rather than a snapshot. That criterion is a single observable fact rather than a claim about output shape, which is where the constitution's testing standards say an assertion beats a snapshot.

8. Build order

1. Test seam (StringWriter in the host, DiagnosticOutput on TestResult).
2. Recorder and its logfmt formatting — unit-testable in isolation, no network, no Core changes.
3. Program-side run.start / run.invocation records.
4. ServerSelector route, and the server.selected record.
5. Core carrier and per-request records.

This front-loads everything that needs no NetPace.Core change, so the public-API commit lands only once the format is already pinned by passing tests.