Skip to content

Commit b327939

Browse files
Copilotnchapagain001
Copilot
and
nchapagain001
authored
Refactor copilot-instructions.md into focused instruction files with PR review guidelines (#695)
* Add .github/copilot-instructions.md with comprehensive codebase conventions and patterns Co-authored-by: nchapagain001 <165215502+nchapagain001@users.noreply.github.com>
1 parent 3b0174b commit b327939

8 files changed

Lines changed: 624 additions & 0 deletions

‎.github/copilot-instructions.md‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
# Copilot Instructions for VirtualClient
2+
3+
## Project Overview
4+
5+
VirtualClient is a cross-platform benchmarking framework by Microsoft. It runs profile-driven performance
6+
benchmarks, stress tests, and qualification workloads on systems (particularly Azure VMs), collecting
7+
structured metrics. Supports Windows and Linux on x64 and ARM64.
8+
9+
## Tech Stack
10+
11+
- **Runtime**: .NET 9 (`global.json`)
12+
- **Platforms**: `linux-x64`, `linux-arm64`, `win-x64`, `win-arm64`
13+
- **Serialization**: `Newtonsoft.Json`, `YamlDotNet`
14+
- **DI**: `Microsoft.Extensions.DependencyInjection`
15+
- **Logging**: `Serilog.Extensions.Logging` (NOT `Serilog`)
16+
- **Testing**: NUnit, Moq, AutoFixture
17+
- **Code quality**: StyleCop.Analyzers, AsyncFixer
18+
- **Package versions**: Centrally managed via `Directory.Packages.props`
19+
20+
## Repository Structure
21+
22+
```
23+
src/VirtualClient/
24+
├── VirtualClient.Main/ # CLI entry point, profiles/
25+
├── VirtualClient.Contracts/ # Base classes (VirtualClientComponent), Metric, Parser/
26+
├── VirtualClient.Core/ # ProfileExecutor, PackageManager, SystemManagement
27+
├── VirtualClient.Common/ # IProcessProxy, ConcurrentBuffer, Telemetry/EventContext
28+
├── VirtualClient.Api/ # REST API for client/server coordination
29+
├── VirtualClient.Actions/ # ~40+ workload executors (OpenSSL/, FIO/, DiskSpd/, ...)
30+
├── VirtualClient.Dependencies/ # Prerequisite installers
31+
├── VirtualClient.Monitors/ # Background monitors
32+
├── VirtualClient.TestFramework/ # MockFixture, InMemoryProcess, test doubles
33+
└── VirtualClient.*.UnitTests/ # Unit test projects
34+
```
35+
36+
## Architecture Patterns
37+
38+
### Component Model
39+
40+
All workloads, dependencies, monitors inherit `VirtualClientComponent`.
41+
Constructor: `(IServiceCollection, IDictionary<string, IConvertible>)`.
42+
Lifecycle: `IsSupported` → `InitializeAsync` → `Validate` → `ExecuteAsync` → `CleanupAsync`.
43+
44+
### Process Execution
45+
46+
External binaries run through `IProcessProxy` (wraps `System.Diagnostics.Process`).
47+
Output captured via `ConcurrentBuffer`. Tests use `InMemoryProcess`.
48+
49+
## Build and Test
50+
51+
```bash
52+
# Build
53+
./build.sh # or: dotnet build src/VirtualClient/VirtualClient.sln -c Debug
54+
55+
# Test
56+
./build-test.sh # or: dotnet test <project>.csproj -c Debug --filter "Category=Unit"
57+
58+
# Publish
59+
dotnet publish src/VirtualClient/VirtualClient.Main/VirtualClient.Main.csproj -r linux-x64 -c Release --self-contained
60+
```
61+
62+
Version is read from the `VERSION` file. Override with `VCBuildVersion` env var.

‎.github/instructions/client-server-workloads.instructions.md‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
---
2+
applyTo: "VirtualClient.Actions/**/*.cs"
3+
description: "Pattern for developing multi-VM client/server/reverseProxy workloads"
4+
---
5+
6+
# Client/Server Workload Development
7+
8+
For network and database workloads, VirtualClient supports multi-role execution where separate
9+
instances run as client and server, coordinating via the built-in REST API.
10+
11+
## Key Components
12+
13+
- `EnvironmentLayout` — topology of instances (`ClientInstance` with Name, Role, IPAddress)
14+
- `IApiClientManager` — creates API clients for inter-VM communication
15+
- `ClientRole.Client` / `ClientRole.Server` / `ClientRole.ReverseProxy` — role constants
16+
- `this.SetServerOnline(bool)` — extension method to signal server readiness
17+
- `serverApiClient.PollForHeartbeatAsync(timeout, ct)` / `PollForServerOnlineAsync(timeout, ct)`
18+
19+
## Base Executor Pattern
20+
21+
See `VirtualClient.Actions/Examples/ClientServer/ExampleClientServerExecutor.cs` for the canonical
22+
implementation. The base class resolves dependencies and defines supported roles in the constructor:
23+
24+
```csharp
25+
[SupportedPlatforms("linux-x64,linux-arm64")]
26+
public class MyWorkloadExecutor : VirtualClientComponent
27+
{
28+
public MyWorkloadExecutor(IServiceCollection dependencies, IDictionary<string, IConvertible> parameters = null)
29+
: base(dependencies, parameters)
30+
{
31+
this.SystemManagement = dependencies.GetService<ISystemManagement>();
32+
this.ApiClientManager = dependencies.GetService<IApiClientManager>();
33+
this.FileSystem = this.SystemManagement.FileSystem;
34+
this.PackageManager = this.SystemManagement.PackageManager;
35+
this.ProcessManager = this.SystemManagement.ProcessManager;
36+
this.StateManager = this.SystemManagement.StateManager;
37+
38+
// Set the base class property — do NOT declare a new field
39+
this.SupportedRoles = new List<string> { ClientRole.Client, ClientRole.Server };
40+
}
41+
}
42+
```
43+
44+
## Client-Side Sync Flow
45+
46+
Clients poll the server before starting the workload (see `ExampleClientExecutor.cs`):
47+
48+
```csharp
49+
IApiClient serverApiClient = this.ApiClientManager.GetOrCreateApiClient(server.Name, server);
50+
await serverApiClient.PollForHeartbeatAsync(this.PollingTimeout, cancellationToken);
51+
await serverApiClient.PollForServerOnlineAsync(TimeSpan.FromSeconds(30), cancellationToken);
52+
// Server confirmed online — execute workload
53+
```
54+
55+
## Server-Side Signal Flow
56+
57+
Servers signal readiness after starting (see `ExampleServerExecutor.cs`):
58+
59+
```csharp
60+
this.SetServerOnline(true); // Signal to clients
61+
await webHostProcess.WaitForExitAsync(cancellationToken);
62+
// In finally block:
63+
this.SetServerOnline(false); // Always signal offline before exiting
64+
```
65+
66+
## Validation
67+
68+
Override `Validate()` to check layout and roles:
69+
```csharp
70+
protected override void Validate()
71+
{
72+
base.Validate();
73+
this.ThrowIfLayoutNotDefined();
74+
}
75+
```
76+
77+
## Profile Structure
78+
79+
```json
80+
{
81+
"Actions": [
82+
{ "Type": "MyServerExecutor", "Parameters": { "Role": "Server", "Port": 5000 } },
83+
{ "Type": "MyClientExecutor", "Parameters": { "Role": "Client", "ServerPort": "$.Parameters.Port" } }
84+
]
85+
}
86+
```
87+
88+
## Checklist
89+
90+
- [ ] Set `this.SupportedRoles` in constructor (use base class property, not a new field)
91+
- [ ] Resolve `IApiClientManager` and `ISystemManagement` from dependencies
92+
- [ ] Use `this.IsInRole(ClientRole.Client/Server)` to branch execution
93+
- [ ] Server calls `this.SetServerOnline(true/false)` for handshake
94+
- [ ] Client calls `PollForHeartbeatAsync` then `PollForServerOnlineAsync`
95+
- [ ] Use `Polly` retry policies for cross-VM communication resilience
96+
- [ ] Test both client and server code paths independently

‎.github/instructions/csharp.instructions.md‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
---
2+
applyTo: "**/*.cs"
3+
description: "C# coding standards and conventions for VirtualClient"
4+
---
5+
6+
# C# Coding Standards
7+
8+
## File Header
9+
10+
Every `.cs` file must start with:
11+
```csharp
12+
// Copyright (c) Microsoft Corporation.
13+
// Licensed under the MIT License.
14+
```
15+
16+
## Namespace and Using Style
17+
18+
- `using` statements go **inside** the namespace block (not at file top) — enforced by StyleCop SA1200
19+
- Ordering: `System.*` → `Microsoft.*` → `Newtonsoft.*` → `VirtualClient.*`
20+
- Namespace matches folder structure: `VirtualClient.Actions`, `VirtualClient.Contracts`, etc.
21+
22+
```csharp
23+
namespace VirtualClient.Actions
24+
{
25+
using System;
26+
using System.Collections.Generic;
27+
using System.Threading;
28+
using System.Threading.Tasks;
29+
using Microsoft.Extensions.DependencyInjection;
30+
using Microsoft.Extensions.Logging;
31+
using VirtualClient.Common;
32+
using VirtualClient.Contracts;
33+
}
34+
```
35+
36+
## Naming Conventions
37+
38+
- **Classes**: PascalCase, suffixed by role (`OpenSslExecutor`, `DiskSpdMetricsParser`)
39+
- **Properties**: PascalCase (`CommandLine`, `MetricScenario`)
40+
- **Private fields**: camelCase, no prefix (`private IFileSystem fileSystem;` — not `_fileSystem`)
41+
- **Member access**: Always use `this.` prefix (`this.fileSystem`, `this.Parameters`, `this.Logger`)
42+
- **Constants**: PascalCase (`private const string CoreMarkOutputFile1 = "run1.log";`)
43+
- **Parameters keys**: PascalCase, accessed case-insensitively via `StringComparer.OrdinalIgnoreCase`
44+
- **Async methods**: Suffix with `Async` (`ExecuteAsync`, `InitializeAsync`, `CleanupAsync`)
45+
46+
## Profile Parameter Properties
47+
48+
Properties reading from the `Parameters` dictionary use this pattern:
49+
50+
```csharp
51+
public string CommandArguments
52+
{
53+
get { return this.Parameters.GetValue<string>(nameof(this.CommandArguments)); }
54+
}
55+
56+
// With default value:
57+
public string CompilerName
58+
{
59+
get { return this.Parameters.GetValue<string>(nameof(this.CompilerName), string.Empty); }
60+
}
61+
```
62+
63+
## XML Documentation
64+
65+
All public members require XML doc comments with `<summary>`, `<param>`, `<returns>` tags:
66+
67+
```csharp
68+
/// <summary>
69+
/// Constructor
70+
/// </summary>
71+
/// <param name="dependencies">Provides required dependencies to the component.</param>
72+
/// <param name="parameters">Parameters defined in the profile or supplied on the command line.</param>
73+
```
74+
75+
## Code Quality Rules
76+
77+
- **StyleCop.Analyzers** enforces style (suppressed: SA1204 static element ordering)
78+
- **AsyncFixer** validates async patterns (suppressed: AZCA1002 async method naming)
79+
- NuGet versions must be in `Directory.Packages.props`, never in individual `.csproj` files
80+
81+
## Exception Handling
82+
83+
Use the project's exception hierarchy — never throw raw `Exception` or `InvalidOperationException`:
84+
85+
- `WorkloadException` — workload failures, validation errors (with `ErrorReason.InvalidProfileDefinition`)
86+
- `DependencyException` — dependency resolution failures
87+
- `ProcessException` — process execution failures
88+
- `MonitorException` — monitor failures
89+
- `WorkloadResultsException` — parsing failures

‎.github/instructions/metrics-parser.instructions.md‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
---
2+
applyTo: "**/*MetricsParser.cs"
3+
description: "Pattern for developing metric parsers with proper units and consistency"
4+
---
5+
6+
# MetricsParser Development Guide
7+
8+
## Class Structure
9+
10+
Parsers inherit from `MetricsParser` → `TextParser<IList<Metric>>`:
11+
12+
```csharp
13+
public class MyWorkloadMetricsParser : MetricsParser
14+
{
15+
private static readonly Regex ValuePattern = new Regex(
16+
@"(\d+\.?\d*)\s+(ops/sec)", RegexOptions.Compiled);
17+
18+
public MyWorkloadMetricsParser(string rawText)
19+
: base(rawText)
20+
{
21+
}
22+
23+
public override IList<Metric> Parse()
24+
{
25+
try
26+
{
27+
this.Preprocess();
28+
this.Sections = TextParsingExtensions.Sectionize(this.PreprocessedText, "SectionHeader");
29+
List<Metric> metrics = new List<Metric>();
30+
// Parse sections and extract metrics
31+
return metrics;
32+
}
33+
catch (Exception exc)
34+
{
35+
throw new WorkloadResultsException(
36+
"Failed to parse results.", exc, ErrorReason.WorkloadResultsParsingFailed);
37+
}
38+
}
39+
40+
protected override void Preprocess()
41+
{
42+
// Remove unwanted lines before parsing
43+
this.PreprocessedText = TextParsingExtensions.RemoveRows(this.RawText, @"^\s*$");
44+
}
45+
}
46+
```
47+
48+
## Standard Units (from `MetricUnit` constants)
49+
50+
Always use `MetricUnit.*` constants from `VirtualClient.Contracts/MetricUnit.cs` — never raw strings.
51+
Full list includes: `Bytes`, `Kilobytes`, `Megabytes`, `Gigabytes`, `Terabytes`, `Petabytes`,
52+
`BytesPerSecond`, `KilobytesPerSecond`, `MegabytesPerSecond`, `GigabytesPerSecond`,
53+
`KibibytesPerSecond`, `MebibytesPerSecond`, `GibibytesPerSecond`,
54+
`OperationsPerSec`, `RequestsPerSec`, `TransactionsPerSec`,
55+
`Nanoseconds`, `Microseconds`, `Milliseconds`, `Seconds`, `Minutes`,
56+
`Count`, `Operations`, `Watts`, `Amps`, `Celcius`, `Megahertz`, `BytesPerConnection`.
57+
58+
## MetricRelativity
59+
60+
Set relativity correctly on every metric:
61+
62+
- `MetricRelativity.HigherIsBetter` — throughput, bandwidth, operations/sec
63+
- `MetricRelativity.LowerIsBetter` — latency, time, error counts
64+
- `MetricRelativity.Undefined` — informational metrics (default)
65+
66+
```csharp
67+
new Metric("throughput", value, MetricUnit.OperationsPerSec, MetricRelativity.HigherIsBetter)
68+
new Metric("latency_p99", value, MetricUnit.Milliseconds, MetricRelativity.LowerIsBetter)
69+
```
70+
71+
## Regex Patterns
72+
73+
Define as `private static readonly Regex` with `RegexOptions.Compiled`:
74+
75+
```csharp
76+
private static readonly Regex ThroughputRegex = new Regex(
77+
@"Throughput:\s+(\d+\.?\d*)\s+ops/sec", RegexOptions.Compiled);
78+
```
79+
80+
## Error Handling
81+
82+
Wrap `Parse()` body in try-catch, throwing `WorkloadResultsException`:
83+
84+
```csharp
85+
catch (Exception exc)
86+
{
87+
throw new WorkloadResultsException(
88+
"Failed to parse MyWorkload results.", exc, ErrorReason.WorkloadResultsParsingFailed);
89+
}
90+
```
91+
92+
## Text Parsing Utilities
93+
94+
- `TextParsingExtensions.Sectionize(text, pattern)` — split text into named sections
95+
- `TextParsingExtensions.RemoveRows(text, pattern)` — strip unwanted lines in `Preprocess()`
96+
- `this.PreprocessedText` — normalized text for parsing (set in `Preprocess()`)
97+
- `this.Sections` — dictionary of parsed sections (set in `Parse()`)

‎.github/instructions/pr-review.instructions.md‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
---
2+
applyTo: "**/*.cs"
3+
description: "PR review rules: required fixes vs suggestions for C# code changes"
4+
---
5+
6+
# PR Review Guidelines
7+
8+
## Required Fixes (flag these — they break things)
9+
10+
1. **Component must inherit `VirtualClientComponent`.** Profile `"Type"` resolution casts via
11+
`Activator.CreateInstance` to `VirtualClientComponent`. Wrong base class → `InvalidCastException`.
12+
13+
2. **Constructor must be `(IServiceCollection, IDictionary<string, IConvertible>)`.** `ComponentFactory`
14+
uses reflection with this exact signature. Wrong constructor → `MissingMethodException`.
15+
16+
3. **Assembly must have `[assembly: VirtualClientComponentAssembly]`.** Without this attribute,
17+
`ComponentTypeCache` skips the assembly during type discovery → `TypeLoadException`.
18+
19+
4. **Profile `"Type"` must exactly match the C# class name.** `ComponentTypeCache.TryGetComponentType`
20+
matches on type name. Typo → `TypeLoadException` at profile load.
21+
22+
5. **Parser must extend `MetricsParser` and implement `Parse()`.** `Parse()` is abstract — missing
23+
override is a compile error. Wrong return type breaks `LogMetrics`.
24+
25+
6. **NuGet versions in `Directory.Packages.props` only.** Adding `Version=` in a `.csproj` causes
26+
build error `NU1008` due to central package management.
27+
28+
7. **`using` statements inside `namespace` block.** StyleCop SA1200 enforced repo-wide — top-level
29+
usings fail CI.
30+
31+
8. **Use project exception hierarchy.** Raw `Exception`/`InvalidOperationException` breaks error
32+
routing on `ErrorReason`. See exception list in csharp.instructions.md.
33+
34+
9. **Tests must have `[TestFixture]` and `[Category("Unit")]`.** Build scripts filter by category.
35+
Missing category → tests silently never run in CI.
36+
37+
10. **Copyright header on every `.cs` file.** StyleCop SA1633 requires the standard two-line header.
38+
39+
## Suggestions (flag these — coding conventions defined in csharp.instructions.md)
40+
41+
1. **Add `[SupportedPlatforms("...")]` to executor classes** — omitting means workload attempts
42+
to run on all platforms.
43+
44+
2. **`Validate()` should throw `WorkloadException` with `ErrorReason.InvalidProfileDefinition`.**
45+
46+
3. **Test classes should inherit `MockFixture`** — not create mocks from scratch.
47+
48+
4. **Parser tests should load real output from `Examples/`** — not inline strings.

0 commit comments

Comments
 (0)