Skip to content

Commit 088ed71

Browse files
mslukebomslukebo
authored
Plugin options system improvements (#408)
* Add logging to plugin options system classes * Do not bubble exceptions when OptionChanged handlers throw * Customize de/serialize message * Validate loader/saver arguments * Fix close stream on read name * Create serialization options in one place * Docstrings
1 parent f9771e8 commit 088ed71

File tree

15 files changed

+241
-58
lines changed

15 files changed

+241
-58
lines changed

‎src/Microsoft.Performance.SDK.Runtime.Tests/Options/PluginOptionsRegistryTests.cs‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public class PluginOptionsRegistryTests
2626
public void ApplyDto_Boolean_UpdatesValue()
2727
{
2828
var originalValue = false;
29-
var sut = new PluginOptionsRegistry();
29+
var sut = CreateSut();
3030
var option = TestPluginOption.BooleanOption(originalValue);
3131

3232
sut.RegisterFrom(new TestPluginOptionsRegistryProvider(option));
@@ -44,7 +44,7 @@ public void ApplyDto_Boolean_UpdatesValue()
4444
public void ApplyDto_Field_UpdatesValue()
4545
{
4646
var originalValue = "original value";
47-
var sut = new PluginOptionsRegistry();
47+
var sut = CreateSut();
4848
var option = TestPluginOption.FieldOption(originalValue);
4949

5050
sut.RegisterFrom(new TestPluginOptionsRegistryProvider(option));
@@ -62,7 +62,7 @@ public void ApplyDto_Field_UpdatesValue()
6262
public void ApplyDto_FieldArray_UpdatesValue()
6363
{
6464
var originalValue = new[] { "original value 1", "original option 2" };
65-
var sut = new PluginOptionsRegistry();
65+
var sut = CreateSut();
6666
var option = TestPluginOption.FieldArrayOption(originalValue);
6767

6868
sut.RegisterFrom(new TestPluginOptionsRegistryProvider(option));
@@ -81,7 +81,7 @@ public void ApplyDto_FieldArray_UpdatesValue()
8181
[TestMethod]
8282
public void ApplyingDto_WithOldDefaultValue_RestoresNewDefaultValue()
8383
{
84-
var sut = new PluginOptionsRegistry();
84+
var sut = CreateSut();
8585

8686
const string expected = "New Default Value";
8787

@@ -102,7 +102,7 @@ public void ApplyingDto_WithOldDefaultValue_RestoresNewDefaultValue()
102102
[TestMethod]
103103
public void ApplyingDto_WithNonExistentOption_HasNoEffect()
104104
{
105-
var sut = new PluginOptionsRegistry();
105+
var sut = CreateSut();
106106
var option = TestPluginOption.BooleanOption(false);
107107
sut.RegisterFrom(new TestPluginOptionsRegistryProvider(option));
108108

@@ -120,7 +120,7 @@ public void ApplyingDto_WithNonExistentOption_HasNoEffect()
120120
[TestMethod]
121121
public void ApplyingDto_WithSerializedNonDefaultValue_WhileUsingDefault_SetsCurrentValue_ToSerializedValue()
122122
{
123-
var sut = new PluginOptionsRegistry();
123+
var sut = CreateSut();
124124
var option = TestPluginOption.FieldOption("Default");
125125
Assert.IsTrue(option.IsUsingDefault);
126126

@@ -140,7 +140,7 @@ public void ApplyingDto_WithSerializedNonDefaultValue_WhileUsingDefault_SetsCurr
140140
[TestMethod]
141141
public void ApplyingDto_WithSerializedNonDefaultValue_WhileUsingNonDefaultValue_RestoresValue()
142142
{
143-
var sut = new PluginOptionsRegistry();
143+
var sut = CreateSut();
144144
var option = TestPluginOption.FieldOption("Default");
145145
option.CurrentValue = "Non default";
146146
Assert.IsFalse(option.IsUsingDefault);
@@ -153,4 +153,9 @@ public void ApplyingDto_WithSerializedNonDefaultValue_WhileUsingNonDefaultValue_
153153
Assert.AreEqual(expected, option.CurrentValue);
154154
Assert.IsFalse(option.IsUsingDefault);
155155
}
156-
}
156+
157+
private PluginOptionsRegistry CreateSut()
158+
{
159+
return new PluginOptionsRegistry(Logger.Null);
160+
}
161+
}

‎src/Microsoft.Performance.SDK.Runtime.Tests/Options/PluginOptionsSystemTests.cs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
using Microsoft.Performance.SDK.Runtime.Options;
99
using Microsoft.Performance.SDK.Tests.Options;
1010
using Microsoft.Performance.Testing;
11+
using Microsoft.Performance.Testing.SDK;
1112
using Microsoft.VisualStudio.TestTools.UnitTesting;
1213

1314
namespace Microsoft.Performance.SDK.Runtime.Tests.Options;
@@ -63,7 +64,7 @@ public void OptionReturnedBySystem_DoNotChangeOriginalInstance()
6364

6465
private PluginOptionsSystem CreateSut(params IProcessingSource[] processingSources)
6566
{
66-
var sut = PluginOptionsSystem.CreateUnsaved();
67+
var sut = PluginOptionsSystem.CreateUnsaved((_) => new NullLogger());
6768
sut.RegisterOptionsFrom(processingSources);
6869
return sut;
6970
}
@@ -74,8 +75,8 @@ private sealed class StubProcessingSource
7475
public StubProcessingSource(params PluginOption[] options)
7576
{
7677
this.pluginOptionsToReturn = new List<PluginOption>(options);
77-
7878
}
79+
7980
protected override ICustomDataProcessor CreateProcessorCore(
8081
IEnumerable<IDataSource> dataSources,
8182
IProcessorEnvironment processorEnvironment,
@@ -92,4 +93,4 @@ protected override bool IsDataSourceSupportedCore(IDataSource dataSource)
9293
private readonly List<PluginOption> pluginOptionsToReturn;
9394
public override IEnumerable<PluginOption> PluginOptions => this.pluginOptionsToReturn ?? Enumerable.Empty<PluginOption>();
9495
}
95-
}
96+
}

‎src/Microsoft.Performance.SDK.Runtime.Tests/Options/StreamPluginOptionsLoaderTests.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ public async Task LoadedDto_ContainsCorrectValues()
5656
/// if <see cref="StreamPluginOptionsLoader"/> is configured to do so.
5757
/// </summary>
5858
[TestMethod]
59-
public async Task CloseStreamOnWrite_ClosesStream()
59+
public async Task CloseStreamOnRead_ClosesStream()
6060
{
6161
using MemoryStream stream = new MemoryStream();
6262
var sut = new TestStreamPluginOptionsLoader(stream, true);
@@ -74,7 +74,7 @@ public async Task CloseStreamOnWrite_ClosesStream()
7474
/// if <see cref="StreamPluginOptionsLoader"/> is not configured to do so.
7575
/// </summary>
7676
[TestMethod]
77-
public async Task DoNotCloseStreamOnWrite_DoesNotCloseStream()
77+
public async Task DoNotCloseStreamOnRead_DoesNotCloseStream()
7878
{
7979
using MemoryStream stream = new MemoryStream();
8080
var sut = new TestStreamPluginOptionsLoader(stream, false);
@@ -92,8 +92,8 @@ private sealed class TestStreamPluginOptionsLoader
9292
{
9393
private readonly Stream stream;
9494

95-
public TestStreamPluginOptionsLoader(MemoryStream stream, bool closeStreamOnWrite)
96-
: base(closeStreamOnWrite)
95+
public TestStreamPluginOptionsLoader(MemoryStream stream, bool closeStreamOnRead)
96+
: base(closeStreamOnRead, Logger.Null)
9797
{
9898
this.stream = stream;
9999
}
@@ -103,4 +103,4 @@ protected override Stream GetStream()
103103
return this.stream;
104104
}
105105
}
106-
}
106+
}

‎src/Microsoft.Performance.SDK.Runtime.Tests/Options/StreamPluginOptionsSaverTests.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ private sealed class TestPluginOptionsSaver
8888
private readonly Stream stream;
8989

9090
public TestPluginOptionsSaver(MemoryStream stream, bool closeStreamOnWrite)
91-
: base(closeStreamOnWrite)
91+
: base(closeStreamOnWrite, Logger.Null)
9292
{
9393
this.stream = stream;
9494
}
@@ -98,4 +98,4 @@ protected override Stream GetStream()
9898
return this.stream;
9999
}
100100
}
101-
}
101+
}

‎src/Microsoft.Performance.SDK.Runtime/Options/PluginOptionsRegistry.cs‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
using System.Linq;
88
using System.Threading;
99
using Microsoft.Performance.SDK.Options;
10+
using Microsoft.Performance.SDK.Processing;
1011
using Microsoft.Performance.SDK.Runtime.Options.Serialization.DTO;
1112

1213
namespace Microsoft.Performance.SDK.Runtime.Options;
@@ -18,6 +19,18 @@ public sealed class PluginOptionsRegistry
1819
{
1920
private readonly object mutex = new();
2021
private readonly Dictionary<Guid, PluginOption> optionByGuid = new();
22+
private readonly ILogger logger;
23+
24+
/// <summary>
25+
/// Initializes a new instance of the <see cref="PluginOptionsRegistry"/> class.
26+
/// </summary>
27+
/// <param name="logger">
28+
/// The logger to use.
29+
/// </param>
30+
public PluginOptionsRegistry(ILogger logger)
31+
{
32+
this.logger = logger;
33+
}
2134

2235
/// <summary>
2336
/// Represents a class that can provide <see cref="PluginOption"/> instances to register to this class. Do not
@@ -59,6 +72,8 @@ internal void RegisterFrom(IProvider provider)
5972
{
6073
lock (this.mutex)
6174
{
75+
this.logger.Info($"Registering plugin options from {provider.GetType().FullName}.");
76+
6277
foreach (var option in provider.GetOptions())
6378
{
6479
this.optionByGuid[option.Guid] = option;
@@ -101,6 +116,8 @@ internal void UpdateFromDto(PluginOptionsDto dto)
101116
{
102117
lock (this.mutex)
103118
{
119+
this.logger.Verbose($"Updating {this.Options.Count} plugin options from DTO.");
120+
104121
ApplyBooleanDtos(dto.BooleanOptions);
105122
ApplyFieldDtos(dto.FieldOptions);
106123
ApplyFieldArrayDtos(dto.FieldArrayOptions);
@@ -111,21 +128,33 @@ private void ApplyFieldArrayDtos(IReadOnlyCollection<FieldArrayPluginOptionDto>
111128
{
112129
ApplyT<FieldArrayOption, FieldArrayPluginOptionDto>(
113130
dtoFieldArrayOptions,
114-
(option, dto) => { option.CurrentValue = dto.Value; });
131+
(option, dto) =>
132+
{
133+
option.CurrentValue = dto.Value;
134+
this.logger.Verbose($"Plugin option {option} was updated to the saved value [{string.Join(", ", dto.Value)}].");
135+
});
115136
}
116137

117138
private void ApplyFieldDtos(IReadOnlyCollection<FieldPluginOptionDto> dtoFieldOptions)
118139
{
119140
ApplyT<FieldOption, FieldPluginOptionDto>(
120141
dtoFieldOptions,
121-
(option, dto) => { option.CurrentValue = dto.Value; });
142+
(option, dto) =>
143+
{
144+
option.CurrentValue = dto.Value;
145+
this.logger.Verbose($"Plugin option {option} was updated to the saved value {dto.Value}.");
146+
});
122147
}
123148

124149
private void ApplyBooleanDtos(IReadOnlyCollection<BooleanPluginOptionDto> dtoBooleanOptions)
125150
{
126151
ApplyT<BooleanOption, BooleanPluginOptionDto>(
127152
dtoBooleanOptions,
128-
(option, dto) => { option.CurrentValue = dto.Value; });
153+
(option, dto) =>
154+
{
155+
option.CurrentValue = dto.Value;
156+
this.logger.Verbose($"Plugin option {option} was updated to the saved value {dto.Value}.");
157+
});
129158
}
130159

131160
private void ApplyT<T, TDTO>(
@@ -141,9 +170,19 @@ private void ApplyT<T, TDTO>(
141170
if (this.optionByGuid.TryGetValue(dto.Guid, out var option) && option is T asT)
142171
{
143172
if (dto.IsDefault)
173+
{
144174
asT.ApplyDefault();
175+
this.logger.Verbose($"Option {asT} was reset to default.");
176+
}
145177
else
178+
{
179+
this.logger.Verbose($"Updating option {asT} from a saved value.");
146180
applySaved(asT, dto);
181+
}
182+
}
183+
else
184+
{
185+
this.logger.Verbose($"No option with GUID {dto.Guid} was found in the registry.");
147186
}
148187
}
149188
}

‎src/Microsoft.Performance.SDK.Runtime/Options/PluginOptionsSystem.cs‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT License.
33

4+
using System;
45
using System.Collections.Generic;
56
using System.Linq;
67
using System.Threading.Tasks;
@@ -25,29 +26,37 @@ public sealed class PluginOptionsSystem
2526
/// <param name="filePath">
2627
/// The path to the file to which options will be saved and loaded.
2728
/// </param>
29+
/// <param name="loggerFactory">
30+
/// A factory for creating loggers.
31+
/// </param>
2832
/// <returns>
2933
/// A new instance of <see cref="PluginOptionsSystem"/> that persists options to a file.
3034
/// </returns>
31-
public static PluginOptionsSystem CreateForFile(string filePath)
35+
public static PluginOptionsSystem CreateForFile(
36+
string filePath,
37+
Func<Type, ILogger> loggerFactory)
3238
{
33-
var loader = new FilePluginOptionsLoader(filePath);
34-
var saver = new FilePluginOptionsSaver(filePath);
35-
var registry = new PluginOptionsRegistry();
39+
var loader = new FilePluginOptionsLoader(filePath, loggerFactory(typeof(FilePluginOptionsLoader)));
40+
var saver = new FilePluginOptionsSaver(filePath, loggerFactory(typeof(FilePluginOptionsSaver)));
41+
var registry = new PluginOptionsRegistry(loggerFactory(typeof(PluginOptionsRegistry)));
3642

3743
return new PluginOptionsSystem(loader, saver, registry);
3844
}
3945

4046
/// <summary>
4147
/// Creates a new instance of <see cref="PluginOptionsSystem"/> that does not persist options.
4248
/// </summary>
49+
/// <param name="loggerFactory">
50+
/// A factory for creating loggers.
51+
/// </param>
4352
/// <returns>
4453
/// A new instance of <see cref="PluginOptionsSystem"/> that does not persist options.
4554
/// </returns>
46-
public static PluginOptionsSystem CreateUnsaved()
55+
public static PluginOptionsSystem CreateUnsaved(Func<Type, ILogger> loggerFactory)
4756
{
4857
var loader = new NullPluginOptionsLoader();
4958
var saver = new NullPluginOptionsSaver();
50-
var registry = new PluginOptionsRegistry();
59+
var registry = new PluginOptionsRegistry(loggerFactory(typeof(PluginOptionsRegistry)));
5160

5261
return new PluginOptionsSystem(loader, saver, registry);
5362
}
@@ -144,4 +153,4 @@ public Task<bool> TrySave()
144153
var dto = optionsRegistryToDtoConverter.ConvertToDto(this.Registry);
145154
return this.Saver.TrySaveAsync(dto);
146155
}
147-
}
156+
}

‎src/Microsoft.Performance.SDK.Runtime/Options/Serialization/Loading/FilePluginOptionsLoader.cs‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT License.
33

4+
using System;
45
using System.IO;
6+
using Microsoft.Performance.SDK.Processing;
57
using Microsoft.Performance.SDK.Runtime.Options.Serialization.DTO;
68

79
namespace Microsoft.Performance.SDK.Runtime.Options.Serialization.Loading;
@@ -20,15 +22,37 @@ public sealed class FilePluginOptionsLoader
2022
/// <param name="filePath">
2123
/// The path to the file from which to load options.
2224
/// </param>
23-
public FilePluginOptionsLoader(string filePath)
24-
: base(true)
25+
/// <param name="logger">
26+
/// The logger to use.
27+
/// </param>
28+
/// <exception cref="ArgumentNullException">
29+
/// <paramref name="filePath"/> is or <paramref name="logger"/> <c>null</c>.
30+
/// </exception>
31+
/// <exception cref="ArgumentException">
32+
/// <paramref name="filePath"/> is empty or composed only of whitespace.
33+
/// </exception>
34+
public FilePluginOptionsLoader(string filePath, ILogger logger)
35+
: base(true, logger)
2536
{
37+
Guard.NotNull(filePath, nameof(filePath));
38+
Guard.NotNull(logger, nameof(logger));
39+
40+
if (string.IsNullOrWhiteSpace(filePath))
41+
{
42+
throw new ArgumentException($"The file path cannot be empty or whitespace.", nameof(filePath));
43+
}
44+
2645
this.filePath = filePath;
2746
}
2847

48+
private protected override string GetDeserializeErrorMessage(Exception exception)
49+
{
50+
return $"Failed to load plugin options from '{this.filePath}': {exception.Message}.";
51+
}
52+
2953
/// <inheritdoc />
3054
protected override Stream GetStream()
3155
{
3256
return File.Open(this.filePath, FileMode.Open, FileAccess.Read, FileShare.Read);
3357
}
34-
}
58+
}

0 commit comments

Comments
 (0)