Skip to content

Add structured result export for sqlstresscmd - #225

Merged
ErikEJ merged 7 commits into
masterfrom
copilot/feature-result-export
Oct 9, 2026
Merged

ErikEJ merged 7 commits into
masterfrom
copilot/feature-result-export

Conversation

Copilot AI commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

The CLI result export path was only partially implemented: -r/--results existed but was effectively CSV-only, still carried stale TODO metadata, and emitted culture-sensitive values that were awkward for downstream tooling. This change formalizes the export contract so result files can be archived, consumed by automation, and parsed reliably.

  • Result export model

    • Added a shared RunResult record for structured run summaries with ISO-8601 timestamps and invariant numeric formatting.
    • JSON output is now produced via the existing serializer, while CSV keeps its append-friendly behavior.
  • CLI behavior

    • -r/--results accepts CSV or JSON destinations.
    • -f/--format can override format detection when the file extension is ambiguous.
    • Default behavior remains extension-based, so normal CSV workflows continue to work.
  • Documentation and usability

    • Removed the stale TODO from the CLI option definition.
    • Updated the CLI README and help text to document the supported export flows.

Example:

sqlstresscmd -s sample.json -t 1 -r results.json -f json
sqlstresscmd -s sample.json -t 1 -r results.csv

This keeps the existing behavior for trend CSV files while enabling a stable, machine-readable JSON export for baselining and CI workflows.

Copilot AI linked an issue Oct 9, 2026 that may be closed by this pull request
Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>
Copilot AI changed the title [WIP] Implement result export feature for CLI Add structured result export for sqlstresscmd Oct 9, 2026
Copilot AI requested a review from ErikEJ October 9, 2026 12:20
@ErikEJ
ErikEJ marked this pull request as ready for review October 9, 2026 12:32
@ErikEJ
ErikEJ requested a balanced review from Copilot October 9, 2026 12:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Exit statuses remain incorrect for several failure paths, and invalid format overrides are silently ignored.

3 open findings
What changed in this PR

Adds structured CSV/JSON result exports for CLI automation and archiving.

Changes:

  • Adds the shared RunResult model and JSON serialization.
  • Adds format selection and invariant CSV timestamps.
  • Documents result export usage and introduces CLI exit codes.
File Description
src/​SqlQueryStressCLI/​sqlstresscmd.csproj Links the result model into the CLI.
src/​SqlQueryStressCLI/​README.md Documents CSV and JSON exports.
src/​SqlQueryStressCLI/​Program.cs Adds process exit codes.
src/​SqlQueryStressCLI/​LoadRunner.cs Implements format selection and JSON export.
src/​SqlQueryStressCLI/​CommandLineOptions.cs Adds the format option.
src/​SQLQueryStress/​RunResult.cs Defines the serialized result contract.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/SqlQueryStressCLI/LoadRunner.cs
Comment thread src/SqlQueryStressCLI/Program.cs
Comment thread src/SqlQueryStressCLI/Program.cs Outdated
Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unrecognized or missing file extensions currently produce mislabeled CSV output instead of requiring a valid format.

0 open findings

3 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reject unsupported extensions instead of defaulting to CSV

src/​SqlQueryStressCLI/​LoadRunner.cs:315

Unknown or missing extensions currently fall through to CSV, so paths such as results.jsn or results.xml succeed while containing CSV despite format detection being extension-based. Normalize and return the actual extension here so the existing default branch rejects unsupported/ambiguous names; callers can use -f when the filename has no recognized extension.

🧠 Review effort: Balanced

@ErikEJ

ErikEJ commented Oct 9, 2026

Copy link
Copy Markdown
Owner

@copilot Unknown or missing extensions currently fall through to CSV, so paths such as results.jsn or results.xml succeed while containing CSV despite format detection being extension-based. Normalize and return the actual extension here so the existing default branch rejects unsupported/ambiguous names; callers can use -f when the filename has no recognized extension.

Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>

Copilot AI commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Unknown or missing extensions currently fall through to CSV, so paths such as results.jsn or results.xml succeed while containing CSV despite format detection being extension-based. Normalize...

Changed extension-based detection to return the normalized extension, so unsupported and missing extensions are rejected unless -f overrides the format. Committed as 8783e18.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Result export can race with outstanding progress callbacks, producing incomplete data and an incorrect success status.

1 open finding

🧠 Review effort: Balanced

Comment thread src/SqlQueryStressCLI/LoadRunner.cs
Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Completion-event disposal has an unsupported concurrency race with the signaling callback.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Avoid disposing ManualResetEventSlim while Set may still be running

src/​SqlQueryStressCLI/​LoadRunner.cs:142

Wait() may return as soon as Set() marks the event signaled, before the completion thread has returned from Set(). Disposing ManualResetEventSlim concurrently with Set() is unsupported and can race the completion callback. Keep this one-shot event alive for the runner's lifetime (or replace it with a task-based completion primitive) instead of disposing it here.

🧠 Review effort: Balanced

@ErikEJ

ErikEJ commented Oct 9, 2026

Copy link
Copy Markdown
Owner

@copilot Wait() may return as soon as Set() marks the event signaled, before the completion thread has returned from Set(). Disposing ManualResetEventSlim concurrently with Set() is unsupported and can race the completion callback. Keep this one-shot event alive for the runner's lifetime (or replace it with a task-based completion primitive) instead of disposing it here.

Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>

Copilot AI commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Wait() may return as soon as Set() marks the event signaled, before the completion thread has returned from Set(). Disposing ManualResetEventSlim concurrently with Set() is unsupported and ca...

Removed the post-Wait() disposal so the one-shot event remains alive for the runner’s lifetime and cannot race Set(). Addressed in 3a7aca9. CLI Release build passed; review and CodeQL found no issues.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unsupported formats are rejected only after the SQL workload has already executed.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Medium severity Validate output format before executing SQL workload

src/​SqlQueryStressCLI/​LoadRunner.cs:88

Unsupported output formats are validated only after the SQL workload completes. A typo such as -f jsno can therefore execute a long-running or mutating query before the CLI reports that the command is invalid. Resolve and reject the format at the start of Run, before testing the connection or starting the worker.

🧠 Review effort: Balanced

@ErikEJ

ErikEJ commented Oct 9, 2026

Copy link
Copy Markdown
Owner

@copilot
Unsupported output formats are validated only after the SQL workload completes. A typo such as -f jsno can therefore execute a long-running or mutating query before the CLI reports that the command is invalid. Resolve and reject the format at the start of Run, before testing the connection or starting the worker.

Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>

Copilot AI commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

@copilot
Unsupported output formats are validated only after the SQL workload completes. A typo such as -f jsno can therefore execute a long-running or mutating query before the CLI reports that t...

Unsupported result formats are now rejected at the start of Run(), before connection testing or workload execution. Committed as HEAD.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The export formats, result aggregation, failure propagation, documentation, and unaffected GUI path are consistent and complete.

0 open findings

🧠 Review effort: Balanced

@ErikEJ
ErikEJ merged commit a0e5a45 into master Oct 9, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI - Result export

3 participants