From 4fe804fa90f5946b86e7899c3bc6e492c7547e2f Mon Sep 17 00:00:00 2001 From: Aditya Abhishek Date: Wed, 3 Jun 2026 12:34:48 +0530 Subject: [PATCH] fix the metrics parsing logic --- VERSION | 2 +- .../ScriptExecutor/ScriptExecutorTests.cs | 47 +++++++++++++++ .../ScriptExecutor/ScriptExecutor.cs | 60 ++++++++++++------- 3 files changed, 88 insertions(+), 21 deletions(-) diff --git a/VERSION b/VERSION index 2c6109e5bb..0163af7e86 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.3.4 \ No newline at end of file +3.3.5 \ No newline at end of file diff --git a/src/VirtualClient/VirtualClient.Actions.UnitTests/ScriptExecutor/ScriptExecutorTests.cs b/src/VirtualClient/VirtualClient.Actions.UnitTests/ScriptExecutor/ScriptExecutorTests.cs index 920763b9c9..9a64117514 100644 --- a/src/VirtualClient/VirtualClient.Actions.UnitTests/ScriptExecutor/ScriptExecutorTests.cs +++ b/src/VirtualClient/VirtualClient.Actions.UnitTests/ScriptExecutor/ScriptExecutorTests.cs @@ -380,6 +380,53 @@ public void ScriptExecutorMovesTheLogFilesToCorrectDirectory_Unix(PlatformID pla } } + [Test] + [TestCase(PlatformID.Win32NT)] + [TestCase(PlatformID.Unix)] + public async Task ScriptExecutorMovesMetricsFileAfterParsingNotDuring(PlatformID platform) + { + this.SetupTest(platform); + + string metricsJson = @"{""metric1"": 100}"; + bool metricsFileParsed = false; + bool metricsFileMoved = false; + int operationOrder = 0; + int parseOrder = 0; + int moveOrder = 0; + + // Setup file system to return valid metrics content + this.mockFixture.FileSystem.Setup(fe => fe.File.ReadAllTextAsync( + It.Is(path => path.Contains("test-metrics.json")), + It.IsAny())) + .ReturnsAsync(metricsJson) + .Callback(() => + { + metricsFileParsed = true; + parseOrder = ++operationOrder; + }); + + // Capture the file move operation + this.mockFixture.File.Setup(fe => fe.Move( + It.Is(path => path.Contains("test-metrics.json")), + It.IsAny(), + It.IsAny())) + .Callback((source, dest, overwrite) => + { + metricsFileMoved = true; + moveOrder = ++operationOrder; + }); + + using (TestScriptExecutor executor = new TestScriptExecutor(this.mockFixture)) + { + await executor.InitializeAsync(EventContext.None, CancellationToken.None); + await executor.ExecuteAsync(EventContext.None, CancellationToken.None); + + Assert.IsTrue(metricsFileParsed, "Metrics file should have been parsed"); + Assert.IsTrue(metricsFileMoved, "Metrics file should have been moved"); + Assert.Less(parseOrder, moveOrder, "Metrics file should be parsed BEFORE being moved to avoid race conditions"); + } + } + private class TestScriptExecutor : ScriptExecutor { public TestScriptExecutor(MockFixture fixture) diff --git a/src/VirtualClient/VirtualClient.Actions/ScriptExecutor/ScriptExecutor.cs b/src/VirtualClient/VirtualClient.Actions/ScriptExecutor/ScriptExecutor.cs index 3263777485..734615bf95 100644 --- a/src/VirtualClient/VirtualClient.Actions/ScriptExecutor/ScriptExecutor.cs +++ b/src/VirtualClient/VirtualClient.Actions/ScriptExecutor/ScriptExecutor.cs @@ -12,6 +12,7 @@ namespace VirtualClient.Actions using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; + using Microsoft.Extensions.Logging; using MimeMapping; using Polly; using VirtualClient.Common; @@ -137,7 +138,6 @@ protected override async Task InitializeAsync(EventContext telemetryContext, Can { await this.EvaluateParametersAsync(cancellationToken); - string scriptFileLocation = string.Empty; if (PlatformSpecifics.IsFullyQualifiedPath(this.ScriptPath)) { this.ExecutablePath = this.ScriptPath; @@ -171,13 +171,14 @@ protected override async Task InitializeAsync(EventContext telemetryContext, Can ErrorReason.WorkloadDependencyMissing); } + this.ExecutableDirectory = this.fileSystem.Path.GetDirectoryName(this.ExecutablePath); + this.MetricsFilePath = this.Combine(this.ExecutableDirectory, ScriptExecutor.MetricsFileName); if (this.fileSystem.File.Exists(this.MetricsFilePath)) { this.fileSystem.File.Delete(this.MetricsFilePath); } - this.ExecutableDirectory = this.fileSystem.Path.GetDirectoryName(this.ExecutablePath); this.ToolName = string.IsNullOrWhiteSpace(this.ToolName) ? $"{this.fileSystem.Path.GetFileNameWithoutExtension(this.ExecutablePath)}" : this.ToolName; await this.systemManagement.MakeFileExecutableAsync(this.ExecutablePath, this.Platform, cancellationToken); } @@ -229,36 +230,56 @@ protected async Task CaptureMetricsAsync(IProcessProxy process, EventContext tel this.MetadataContract.Apply(telemetryContext); bool metricsFileFound = false; + bool metricsFileParsed = false; + int metricsCount = 0; try { + telemetryContext.AddContext("expectedMetricsFilePath", this.MetricsFilePath); + if (this.fileSystem.File.Exists(this.MetricsFilePath)) { metricsFileFound = true; - telemetryContext.AddContext("metricsFilePath", this.MetricsFilePath); string results = await this.fileSystem.File.ReadAllTextAsync(this.MetricsFilePath); if (!string.IsNullOrWhiteSpace(results)) { JsonMetricsParser parser = new JsonMetricsParser(results, this.Logger, telemetryContext); IList workloadMetrics = parser.Parse(); + metricsCount = workloadMetrics?.Count ?? 0; + metricsFileParsed = metricsCount > 0; - this.Logger.LogMetrics( - this.ToolName, - (this.MetricScenario ?? this.Scenario) ?? "Script", - process.StartTime, - process.ExitTime, - workloadMetrics, - null, - process.FullCommand(), - this.Tags, - telemetryContext); + if (metricsFileParsed) + { + this.Logger.LogMetrics( + this.ToolName, + (this.MetricScenario ?? this.Scenario) ?? "Script", + process.StartTime, + process.ExitTime, + workloadMetrics, + null, + process.FullCommand(), + this.Tags, + telemetryContext); + } + else + { + // IMPROVED: Log warning when metrics file is empty or contains no valid metrics + this.Logger.LogMessage( + $"{this.TypeName}.MetricsFileEmpty", + LogLevel.Warning, + telemetryContext.Clone() + .AddContext("metricsFilePath", this.MetricsFilePath) + .AddContext("reason", "Metrics file exists but contains no valid metrics")); + } } } } finally { telemetryContext.AddContext(nameof(metricsFileFound), metricsFileFound); + telemetryContext.AddContext(nameof(metricsFileParsed), metricsFileParsed); + telemetryContext.AddContext(nameof(metricsCount), metricsCount); } } } @@ -273,7 +294,7 @@ protected async Task CaptureLogsAsync(CancellationToken cancellationToken) // e.g. // /logs/anytool/executecustomscript1 // /logs/anytool/executecustomscript2 - string destinitionLogsDir = this.Combine(this.GetLogDirectory(this.ToolName), DateTime.UtcNow.ToString("yyyy-MM-dd_hh-mm-ss")); + string destinationLogsDir = this.Combine(this.GetLogDirectory(this.ToolName), DateTime.UtcNow.ToString("yyyy-MM-dd_hh-mm-ss")); if (this.LogPaths?.Any() == true) { @@ -291,7 +312,7 @@ protected async Task CaptureLogsAsync(CancellationToken cancellationToken) { foreach (string logFilePath in this.fileSystem.Directory.GetFiles(fullLogPath, "*", SearchOption.AllDirectories)) { - var logs = await this.MoveLogsAsync(logFilePath, destinitionLogsDir, cancellationToken, sourceRootDirectory: fullLogPath); + var logs = await this.MoveLogsAsync(logFilePath, destinationLogsDir, cancellationToken, sourceRootDirectory: fullLogPath); await this.RequestLogUploadsAsync(logs); } } @@ -299,17 +320,16 @@ protected async Task CaptureLogsAsync(CancellationToken cancellationToken) // Check for Matching FileNames foreach (string logFilePath in this.fileSystem.Directory.GetFiles(this.ExecutableDirectory, logPath, SearchOption.AllDirectories)) { - var logs = await this.MoveLogsAsync(logFilePath, destinitionLogsDir, cancellationToken); + var logs = await this.MoveLogsAsync(logFilePath, destinationLogsDir, cancellationToken); await this.RequestLogUploadsAsync(logs); } } } - // Move test-metrics.json file if that exists - string metricsFilePath = this.Combine(this.ExecutableDirectory, "test-metrics.json"); - if (this.fileSystem.File.Exists(metricsFilePath)) + // Move test-metrics.json file if that exists. + if (this.fileSystem.File.Exists(this.MetricsFilePath)) { - var logs = await this.MoveLogsAsync(metricsFilePath, destinitionLogsDir, cancellationToken); + var logs = await this.MoveLogsAsync(this.MetricsFilePath, destinationLogsDir, cancellationToken); await this.RequestLogUploadsAsync(logs); } }