Skip to content

Fix the metrics parsing logic in script executor #725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
imadityaa merged 1 commit into from
Jun 3, 2026
Merged

Fix the metrics parsing logic in script executor #725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion VERSION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3.3.4
3.3.5
47 changes: 47 additions & 0 deletions src/VirtualClient/VirtualClient.Actions.UnitTests/ScriptExecutor/ScriptExecutorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>(path => path.Contains("test-metrics.json")),
It.IsAny<CancellationToken>()))
.ReturnsAsync(metricsJson)
.Callback(() =>
{
metricsFileParsed = true;
parseOrder = ++operationOrder;
});

// Capture the file move operation
this.mockFixture.File.Setup(fe => fe.Move(
It.Is<string>(path => path.Contains("test-metrics.json")),
It.IsAny<string>(),
It.IsAny<bool>()))
.Callback<string, string, bool>((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)
Expand Down
60 changes: 40 additions & 20 deletions src/VirtualClient/VirtualClient.Actions/ScriptExecutor/ScriptExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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<Metric> 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);
}
}
}
Expand All @@ -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)
{
Expand All @@ -291,25 +312,24 @@ 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);
}
}

// 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);
}
}
Expand Down
Loading