Standardized support for filtering metrics using verbosity levels, inclusion and exclusion regex.#636
Standardized support for filtering metrics using verbosity levels, inclusion and exclusion regex.#636RakeshwarK wants to merge 10 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces standardized metric filtering capabilities to the Virtual Client, allowing users to control which metrics are captured and logged using verbosity-based filtering, regex-based inclusion filtering, and exclusion filtering. The changes affect the Fio, DiskSpd, and Memtier workload parsers, along with core metric filtering infrastructure and comprehensive documentation.
Changes:
- Added comprehensive metric filtering documentation to the profiles guide with examples for verbosity, inclusion, and exclusion filtering
- Updated verbosity level definitions from the previous 0-2 scale to a new 1-5 scale (with only 1, 3, and 5 actively used in modified workloads)
- Enhanced MetricExtensions.FilterBy() to support verbosity filtering, exclusion filters (prefix with "-"), and regex-based name filtering
- Updated Fio, DiskSpd, and Memtier metric parsers to assign appropriate verbosity levels to metrics
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| website/docs/guides/0011-profiles.md | Added comprehensive documentation section on metric filtering including verbosity levels, regex patterns, exclusion filters, and combined filter examples |
| src/VirtualClient/VirtualClient.Contracts/Metric.cs | Updated XML documentation for Verbosity property to describe new 1-5 scale with detailed descriptions for each level |
| src/VirtualClient/VirtualClient.Contracts/Extensions/MetricExtensions.cs | Enhanced FilterBy() method to support verbosity filtering, exclusion filters, and inclusion filters; updated LogConsole() to use verbosity level 1 instead of 0 for critical metrics |
| src/VirtualClient/VirtualClient.Contracts.UnitTests/MetricExtensionsTests.cs | Added comprehensive test coverage for new filtering capabilities including verbosity levels, exclusion filters, and complex filter combinations |
| src/VirtualClient/VirtualClient.Actions/Memtier/MemtierMetricsParser.cs | Updated metric verbosity levels from 0-2 to 1/3/5 scale; added missing metricUnit to Hits/sec and Misses/sec metrics |
| src/VirtualClient/VirtualClient.Actions/FIO/FioMetricsParser.cs | Updated all FIO metrics to use new verbosity levels: 1 for critical metrics (bandwidth, IOPS, p50/p99), 3 for detailed percentiles (p70, p90, p99.9), and 5 for diagnostic metrics (histograms, stddev, byte/IO counts) |
| src/VirtualClient/VirtualClient.Actions/DiskSpd/DiskSpdMetricsParser.cs | Updated DiskSpd metrics to use new verbosity levels: 1 for critical metrics (bandwidth, IOPS, latency percentiles), 5 for diagnostic metrics (CPU usage, byte/IO counts) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/VirtualClient/VirtualClient.Contracts/Extensions/MetricExtensions.cs
Outdated
Show resolved
Hide resolved
src/VirtualClient/VirtualClient.Contracts/Extensions/MetricExtensions.cs
Show resolved
Hide resolved
src/VirtualClient/VirtualClient.Contracts/Extensions/MetricExtensions.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/VirtualClient/VirtualClient.Actions/ASPNET/BombardierMetricsParser.cs:51
- Lines 47-51 create metrics without specifying a verbosity level. According to the new standardization, these metrics will default to verbosity level 1. However, some of these percentile metrics (P50, P75, P90, P95, P99) should likely have explicit verbosity levels assigned for consistency with the rest of the codebase.
Based on the pattern in this file and others:
- P50 and P99 should be level 1 (critical percentiles)
- P75, P90, P95 should be level 2 (detailed percentiles)
This should be made explicit to match the verbosity assignment pattern seen elsewhere in the PR.
this.metrics.Add(new Metric("RequestPerSecond P50", root.Result.Rps.Percentiles.P50, MetricUnit.RequestsPerSec, MetricRelativity.HigherIsBetter));
this.metrics.Add(new Metric("RequestPerSecond P75", root.Result.Rps.Percentiles.P75, MetricUnit.RequestsPerSec, MetricRelativity.HigherIsBetter));
this.metrics.Add(new Metric("RequestPerSecond P90", root.Result.Rps.Percentiles.P90, MetricUnit.RequestsPerSec, MetricRelativity.HigherIsBetter));
this.metrics.Add(new Metric("RequestPerSecond P95", root.Result.Rps.Percentiles.P95, MetricUnit.RequestsPerSec, MetricRelativity.HigherIsBetter));
this.metrics.Add(new Metric("RequestPerSecond P99", root.Result.Rps.Percentiles.P99, MetricUnit.RequestsPerSec, MetricRelativity.HigherIsBetter));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/VirtualClient/VirtualClient.Contracts/Extensibility/MetricDataPoint.cs
Outdated
Show resolved
Hide resolved
src/VirtualClient/VirtualClient.Actions.UnitTests/3DMark/ThreeDMarkMetricsParserTests.cs
Outdated
Show resolved
Hide resolved
src/VirtualClient/VirtualClient.Contracts/Extensions/MetricExtensions.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Rakesh <153008248+RakeshwarK@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Rakesh <153008248+RakeshwarK@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Rakesh <153008248+RakeshwarK@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/VirtualClient/VirtualClient.Actions/ASPNET/BombardierMetricsParser.cs:51
- The RequestPerSecond percentile metrics (P50, P75, P90, P95, P99) don't specify verbosity levels and will default to verbosity: 1. For consistency with the Latency metrics above (lines 38-42), consider setting P75, P90, and P95 to verbosity: 2, keeping only P50 and P99 at the default verbosity: 1 for critical percentiles.
this.metrics.Add(new Metric("RequestPerSecond P50", root.Result.Rps.Percentiles.P50, MetricUnit.RequestsPerSec, MetricRelativity.HigherIsBetter));
this.metrics.Add(new Metric("RequestPerSecond P75", root.Result.Rps.Percentiles.P75, MetricUnit.RequestsPerSec, MetricRelativity.HigherIsBetter));
this.metrics.Add(new Metric("RequestPerSecond P90", root.Result.Rps.Percentiles.P90, MetricUnit.RequestsPerSec, MetricRelativity.HigherIsBetter));
this.metrics.Add(new Metric("RequestPerSecond P95", root.Result.Rps.Percentiles.P95, MetricUnit.RequestsPerSec, MetricRelativity.HigherIsBetter));
this.metrics.Add(new Metric("RequestPerSecond P99", root.Result.Rps.Percentiles.P99, MetricUnit.RequestsPerSec, MetricRelativity.HigherIsBetter));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// - 1 (Standard/Critical): Most important metrics | ||
| /// - 2 (Detailed): Additional detailed metrics | ||
| /// - 5 (Verbose): All diagnostic/internal metrics | ||
| /// Verbosity level 3 is reserved for future expansion and should not be used. |
There was a problem hiding this comment.
The summary should mention both levels 3 and 4 as reserved, not just level 3. Change "Verbosity level 3 is reserved for future expansion and should not be used." to "Verbosity levels 3 and 4 are reserved for future expansion and should not be used." to match the documentation in Metric.cs and MetricExtensions.cs.
| /// Verbosity level 3 is reserved for future expansion and should not be used. | |
| /// Verbosity levels 3 and 4 are reserved for future expansion and should not be used. |
| "Scenario": "RandomWrite_4k_BlockSize", | ||
| "PackageName": "fio", | ||
| "CommandLine": "--name=fio_test --size=10G --rw=randwrite --bs=4k", | ||
| "MetricFilters": "verbosity:3,-histogram*,bandwidth|iops|latency" |
There was a problem hiding this comment.
The example uses "verbosity:3" which includes reserved verbosity levels 3 and 4. Since currently only levels 1, 2, and 5 are actively used (as stated in the documentation above), consider using "verbosity:2" instead to make the example clearer and more aligned with actual usage patterns. Using "verbosity:3" would include the same metrics as "verbosity:2" in practice (since no metrics use level 3), but "verbosity:2" is more explicit about including only Standard and Detailed metrics.
| verbosity: 1, | ||
| description: "The latency for 50% of all requests was at or under this value.") | ||
| }, | ||
| // Level 3 - Detailed: P90 latency |
There was a problem hiding this comment.
The comment says "Level 3 - Detailed" but the actual verbosity value is set to 2 on line 367. The comment should say "Level 2 - Detailed" to match the actual verbosity level being used. The same issue exists on line 370 (P95 latency) and line 390 (P99.9 latency).
| this.AddMeasurement(metrics, job, $"read.clat_ns.percentile.['50.000000']", "read_completionlatency_p50", this.ConversionUnits, MetricRelativity.LowerIsBetter, this.ConversionFactor, verbosity: 1); | ||
| this.AddMeasurement(metrics, job, $"read.clat_ns.percentile.['99.000000']", "read_completionlatency_p99", this.ConversionUnits, MetricRelativity.LowerIsBetter, this.ConversionFactor, verbosity: 1); | ||
|
|
||
| // Level 3 - Detailed: Other percentiles |
There was a problem hiding this comment.
The comment says "Level 3 - Detailed" but the actual verbosity value is set to 2 on line 203. The comment should say "Level 2 - Detailed" to match the actual verbosity level being used. This same issue appears on lines 202, 265 in this file (write percentiles).
| // Level 3 - Detailed: Other percentiles | |
| // Level 2 - Detailed: Other percentiles |
| "Parameters": { | ||
| "Scenario": "Server", | ||
| "PackageName": "redis", | ||
| "CommandLine": "--protected-mode no", |
There was a problem hiding this comment.
The example Redis configuration disables protected-mode via the --protected-mode no flag, which can expose the Redis server to unauthenticated access if the host is reachable on the network. An attacker on the same network (or any network segment with access to this host) could connect to the Redis instance, read or modify data, or execute Redis commands with full privileges. Consider updating the example to keep protected-mode enabled by default or clearly document that disabling it is only safe in tightly controlled environments with additional network protections (e.g., binding to localhost or strict firewall rules).
| "CommandLine": "--protected-mode no", | |
| "CommandLine": "--bind 127.0.0.1", |
No description provided.