diff --git a/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/IExperimentConfiguration.cs b/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/IExperimentConfiguration.cs index ff328e440..49a4b3f48 100644 --- a/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/IExperimentConfiguration.cs +++ b/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/IExperimentConfiguration.cs @@ -28,4 +28,15 @@ public interface IExperimentConfiguration /// The detector. /// true if the detector is in the experiment group; otherwise, false. bool IsInExperimentGroup(IComponentDetector componentDetector); + + /// + /// Determines if the experiment should be recorded, given a list of all the detectors that ran and the number of + /// components that were detected. If any call to this method returns false, the experiment will not be + /// recorded. + /// + /// . + /// The component detector. + /// The number of components found by the . + /// true if the experiment should be recorded; otherwise, false. + bool ShouldRecord(IComponentDetector componentDetector, int numComponents); } diff --git a/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/NewNugetExperiment.cs b/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/NewNugetExperiment.cs index 65c57f006..a826b4081 100644 --- a/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/NewNugetExperiment.cs +++ b/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/NewNugetExperiment.cs @@ -18,4 +18,7 @@ public bool IsInControlGroup(IComponentDetector componentDetector) => /// public bool IsInExperimentGroup(IComponentDetector componentDetector) => componentDetector is NuGetProjectModelProjectCentricComponentDetector or NuGetPackagesConfigDetector; + + /// + public bool ShouldRecord(IComponentDetector componentDetector, int numComponents) => true; } diff --git a/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/NpmLockfile3Experiment.cs b/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/NpmLockfile3Experiment.cs index 82eaea030..8e2270009 100644 --- a/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/NpmLockfile3Experiment.cs +++ b/src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/NpmLockfile3Experiment.cs @@ -16,4 +16,8 @@ public class NpmLockfile3Experiment : IExperimentConfiguration /// public bool IsInExperimentGroup(IComponentDetector componentDetector) => componentDetector is NpmLockfile3Detector; + + /// + public bool ShouldRecord(IComponentDetector componentDetector, int numComponents) => + componentDetector is not NpmComponentDetector || numComponents == 0; } diff --git a/src/Microsoft.ComponentDetection.Orchestrator/Experiments/ExperimentService.cs b/src/Microsoft.ComponentDetection.Orchestrator/Experiments/ExperimentService.cs index a9ab3f27c..93f79c2fa 100644 --- a/src/Microsoft.ComponentDetection.Orchestrator/Experiments/ExperimentService.cs +++ b/src/Microsoft.ComponentDetection.Orchestrator/Experiments/ExperimentService.cs @@ -35,6 +35,8 @@ public ExperimentService( /// public void RecordDetectorRun(IComponentDetector detector, IEnumerable components) { + this.FilterExperiments(detector, components.Count()); + foreach (var (config, experimentResults) in this.experiments) { if (config.IsInControlGroup(detector)) @@ -59,21 +61,42 @@ public void RecordDetectorRun(IComponentDetector detector, IEnumerable + this.experiments.RemoveAll(experiment => + { + var shouldRemove = !experiment.Config.ShouldRecord(detector, count); + + if (shouldRemove) + { + this.logger.LogDebug("Removing {Experiment} from active experiments", experiment.Config.Name); + } + + return shouldRemove; + }); + /// public async Task FinishAsync() { foreach (var (config, experiment) in this.experiments) { - var oldComponents = experiment.ControlGroupComponents; - var newComponents = experiment.ExperimentGroupComponents; + var controlComponents = experiment.ControlGroupComponents; + var experimentComponents = experiment.ExperimentGroupComponents; this.logger.LogInformation( - "Experiment {Experiment} finished and has {Count} components in the control group and {Count} components in the experiment group.", + "Experiment {Experiment} finished with {ControlCount} components in the control group and {ExperimentCount} components in the experiment group", config.Name, - oldComponents.Count, - newComponents.Count); + controlComponents.Count, + experimentComponents.Count); + + // If there are no components recorded in the experiment, skip processing experiments. We still want to + // process empty diffs as this means the experiment was successful. + if (!experimentComponents.Any() && !controlComponents.Any()) + { + this.logger.LogWarning("Experiment {Experiment} has no components in either group, skipping processing", config.Name); + continue; + } - var diff = new ExperimentDiff(experiment.ControlGroupComponents, experiment.ExperimentGroupComponents); + var diff = new ExperimentDiff(controlComponents, experimentComponents); foreach (var processor in this.experimentProcessors) { diff --git a/test/Microsoft.ComponentDetection.Orchestrator.Tests/Experiments/ExperimentServiceTests.cs b/test/Microsoft.ComponentDetection.Orchestrator.Tests/Experiments/ExperimentServiceTests.cs index 93500432b..1d4a5174d 100644 --- a/test/Microsoft.ComponentDetection.Orchestrator.Tests/Experiments/ExperimentServiceTests.cs +++ b/test/Microsoft.ComponentDetection.Orchestrator.Tests/Experiments/ExperimentServiceTests.cs @@ -28,6 +28,10 @@ public ExperimentServiceTests() this.experimentProcessorMock = new Mock(); this.loggerMock = new Mock>(); this.detectorMock = new Mock(); + + this.experimentConfigMock.Setup(x => x.IsInControlGroup(this.detectorMock.Object)).Returns(true); + this.experimentConfigMock.Setup(x => x.IsInExperimentGroup(this.detectorMock.Object)).Returns(true); + this.experimentConfigMock.Setup(x => x.ShouldRecord(this.detectorMock.Object, It.IsAny())).Returns(true); } [TestMethod] @@ -35,9 +39,6 @@ public void RecordDetectorRun_AddsComponentsToControlAndExperimentGroup() { var components = ExperimentTestUtils.CreateRandomComponents(); - this.experimentConfigMock.Setup(x => x.IsInControlGroup(this.detectorMock.Object)).Returns(true); - this.experimentConfigMock.Setup(x => x.IsInExperimentGroup(this.detectorMock.Object)).Returns(true); - var service = new ExperimentService( new[] { this.experimentConfigMock.Object }, Enumerable.Empty(), @@ -50,12 +51,38 @@ public void RecordDetectorRun_AddsComponentsToControlAndExperimentGroup() } [TestMethod] - public async Task FinishAsync_ProcessesExperimentsAsync() + public async Task RecordDetectorRun_FiltersExperimentsAsync() { + var filterConfigMock = new Mock(); + filterConfigMock + .Setup(x => x.ShouldRecord(It.IsAny(), It.IsAny())) + .Returns(false); + filterConfigMock.Setup(x => x.IsInControlGroup(this.detectorMock.Object)).Returns(true); + filterConfigMock.Setup(x => x.IsInExperimentGroup(this.detectorMock.Object)).Returns(true); + var components = ExperimentTestUtils.CreateRandomComponents(); - this.experimentConfigMock.Setup(x => x.IsInControlGroup(this.detectorMock.Object)).Returns(true); - this.experimentConfigMock.Setup(x => x.IsInExperimentGroup(this.detectorMock.Object)).Returns(true); + var service = new ExperimentService( + new[] { this.experimentConfigMock.Object, filterConfigMock.Object }, + new[] { this.experimentProcessorMock.Object }, + this.loggerMock.Object); + + service.RecordDetectorRun(this.detectorMock.Object, components); + await service.FinishAsync(); + + filterConfigMock.Verify(x => x.ShouldRecord(this.detectorMock.Object, components.Count), Times.Once()); + this.experimentProcessorMock.Verify( + x => x.ProcessExperimentAsync(filterConfigMock.Object, It.IsAny()), + Times.Never()); + this.experimentProcessorMock.Verify( + x => x.ProcessExperimentAsync(this.experimentConfigMock.Object, It.IsAny()), + Times.Once()); + } + + [TestMethod] + public async Task FinishAsync_ProcessesExperimentsAsync() + { + var components = ExperimentTestUtils.CreateRandomComponents(); var service = new ExperimentService( new[] { this.experimentConfigMock.Object }, @@ -80,9 +107,6 @@ public async Task FinishAsync_SwallowsExceptionsAsync() var components = ExperimentTestUtils.CreateRandomComponents(); - this.experimentConfigMock.Setup(x => x.IsInControlGroup(this.detectorMock.Object)).Returns(true); - this.experimentConfigMock.Setup(x => x.IsInExperimentGroup(this.detectorMock.Object)).Returns(true); - var service = new ExperimentService( new[] { this.experimentConfigMock.Object }, new[] { this.experimentProcessorMock.Object }, @@ -92,4 +116,18 @@ public async Task FinishAsync_SwallowsExceptionsAsync() var act = async () => await service.FinishAsync(); await act.Should().NotThrowAsync(); } + + [TestMethod] + public async Task FinishAsync_SkipsEmptyExperimentsAsync() + { + var service = new ExperimentService( + new[] { this.experimentConfigMock.Object }, + new[] { this.experimentProcessorMock.Object }, + this.loggerMock.Object); + await service.FinishAsync(); + + this.experimentProcessorMock.Verify( + x => x.ProcessExperimentAsync(It.IsAny(), It.IsAny()), + Times.Never()); + } }