From aa28b16735610630ff2d4b174606b17ad4657e7c Mon Sep 17 00:00:00 2001 From: martincostello Date: Thu, 13 Dec 2018 13:53:44 +0000 Subject: [PATCH 1/9] Make StatsDUtf8Formatter sealed Make the internal class StatsDUtf8Formatter sealed. --- src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs b/src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs index 66e48729..eee6a99d 100644 --- a/src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs +++ b/src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs @@ -4,7 +4,7 @@ namespace JustEat.StatsD.Buffered { - internal class StatsDUtf8Formatter + internal sealed class StatsDUtf8Formatter { private readonly byte[] _utf8Prefix; From 4d295ac12cc5ddfb0bb9460bd26230e2de44c741 Mon Sep 17 00:00:00 2001 From: martincostello Date: Thu, 13 Dec 2018 13:54:00 +0000 Subject: [PATCH 2/9] Update grammar Fix grammar in class comments. --- src/JustEat.StatsD/EndpointLookups/DnsLookupIpEndpointSource.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/JustEat.StatsD/EndpointLookups/DnsLookupIpEndpointSource.cs b/src/JustEat.StatsD/EndpointLookups/DnsLookupIpEndpointSource.cs index b6e6053d..a820844e 100644 --- a/src/JustEat.StatsD/EndpointLookups/DnsLookupIpEndpointSource.cs +++ b/src/JustEat.StatsD/EndpointLookups/DnsLookupIpEndpointSource.cs @@ -7,7 +7,7 @@ namespace JustEat.StatsD.EndpointLookups { /// /// A class representing an implementation of that looks up - /// the for DNS hostname to resolve its IP address. + /// the for a DNS hostname to resolve its IP address. /// public class DnsLookupIpEndpointSource : IEndPointSource { From a2d2fa686d2d3f36c5f1af47f29ca9a267532c56 Mon Sep 17 00:00:00 2001 From: martincostello Date: Thu, 13 Dec 2018 13:54:35 +0000 Subject: [PATCH 3/9] Validate cache duration Validate that the cache duration is a positive TimeSpan to flush out configuration mistakes. --- .../EndpointLookups/CachedEndpointSource.cs | 9 +++++++++ .../EndpointLookups/CachedEndpointSourceTests.cs | 15 +++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/JustEat.StatsD/EndpointLookups/CachedEndpointSource.cs b/src/JustEat.StatsD/EndpointLookups/CachedEndpointSource.cs index a9d60386..8b8d59b1 100644 --- a/src/JustEat.StatsD/EndpointLookups/CachedEndpointSource.cs +++ b/src/JustEat.StatsD/EndpointLookups/CachedEndpointSource.cs @@ -22,9 +22,18 @@ public class CachedEndpointSource : IEndPointSource /// /// is . /// + /// + /// is less than or equal to . + /// public CachedEndpointSource(IEndPointSource inner, TimeSpan cacheDuration) { _inner = inner ?? throw new ArgumentNullException(nameof(inner)); + + if (cacheDuration <= TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException(nameof(cacheDuration), cacheDuration, "The end point cache duration must be a positive TimeSpan value."); + } + _cachedValue = null; _cacheDuration = cacheDuration; } diff --git a/tests/JustEat.StatsD.Tests/EndpointLookups/CachedEndpointSourceTests.cs b/tests/JustEat.StatsD.Tests/EndpointLookups/CachedEndpointSourceTests.cs index b8cbe94d..c94529fa 100644 --- a/tests/JustEat.StatsD.Tests/EndpointLookups/CachedEndpointSourceTests.cs +++ b/tests/JustEat.StatsD.Tests/EndpointLookups/CachedEndpointSourceTests.cs @@ -62,6 +62,21 @@ public static async Task CachedValueIsReturnedAgainAfterExpiry() mockInner.Verify(x => x.GetEndpoint(), Times.Exactly(2)); } + [Theory] + [InlineData(0)] + [InlineData(-1)] + public static void ConstructorThrowsIfCacheDurationIsInvalid(long ticks) + { + // Arrange + var inner = Mock.Of(); + var cacheDuration = TimeSpan.FromTicks(ticks); + + // Act and Assert + var exception = Assert.Throws("cacheDuration", () => new CachedEndpointSource(inner, cacheDuration)); + + exception.ActualValue.ShouldBe(cacheDuration); + } + private static IPEndPoint MakeTestIpEndPoint() { return new IPEndPoint(new IPAddress(new byte[] { 1, 2, 3, 4 }), 8125); From 208a9ea5c092d9eb84ed159aa25906bc9fda1467 Mon Sep 17 00:00:00 2001 From: martincostello Date: Thu, 13 Dec 2018 14:03:13 +0000 Subject: [PATCH 4/9] Add exception docs for TimerExtensions Add XML documentation for exceptions to TimerExtensions' methods. Rename constructor argument to match parameter names at call sites. --- src/JustEat.StatsD/DisposableTimer.cs | 8 ++++---- src/JustEat.StatsD/TimerExtensions.cs | 27 +++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/JustEat.StatsD/DisposableTimer.cs b/src/JustEat.StatsD/DisposableTimer.cs index b90040bc..70a7b26c 100644 --- a/src/JustEat.StatsD/DisposableTimer.cs +++ b/src/JustEat.StatsD/DisposableTimer.cs @@ -12,16 +12,16 @@ internal sealed class DisposableTimer : IDisposableTimer public string StatName { get; set; } - public DisposableTimer(IStatsDPublisher publisher, string statName) + public DisposableTimer(IStatsDPublisher publisher, string bucket) { _publisher = publisher ?? throw new ArgumentNullException(nameof(publisher)); - if (string.IsNullOrEmpty(statName)) + if (string.IsNullOrEmpty(bucket)) { - throw new ArgumentNullException(nameof(statName)); + throw new ArgumentNullException(nameof(bucket)); } - StatName = statName; + StatName = bucket; _stopwatch = Stopwatch.StartNew(); } diff --git a/src/JustEat.StatsD/TimerExtensions.cs b/src/JustEat.StatsD/TimerExtensions.cs index ddab4d90..b06f40f1 100644 --- a/src/JustEat.StatsD/TimerExtensions.cs +++ b/src/JustEat.StatsD/TimerExtensions.cs @@ -18,6 +18,9 @@ public static class TimerExtensions /// /// An that publishes the metric when the instance is disposed of. /// + /// + /// or is . + /// public static IDisposableTimer StartTimer(this IStatsDPublisher publisher, string bucket) { return new DisposableTimer(publisher, bucket); @@ -30,6 +33,9 @@ public static IDisposableTimer StartTimer(this IStatsDPublisher publisher, strin /// The to publish with. /// The bucket to publish the timer for. /// A delegate to a method whose invocation should be timed. + /// + /// or is . + /// public static void Time(this IStatsDPublisher publisher, string bucket, Action action) { using (StartTimer(publisher, bucket)) @@ -45,6 +51,9 @@ public static void Time(this IStatsDPublisher publisher, string bucket, Action a /// The to publish with. /// The bucket to publish the timer for. /// A delegate to a method whose invocation should be timed. + /// + /// or is . + /// public static void Time(this IStatsDPublisher publisher, string bucket, Action action) { using (var timer = StartTimer(publisher, bucket)) @@ -63,6 +72,9 @@ public static void Time(this IStatsDPublisher publisher, string bucket, Action /// A representing the asynchronous operation to time. /// + /// + /// or is . + /// public static async Task Time(this IStatsDPublisher publisher, string bucket, Func action) { using (StartTimer(publisher, bucket)) @@ -82,6 +94,9 @@ public static async Task Time(this IStatsDPublisher publisher, string bucket, Fu /// /// A representing the asynchronous operation to time. /// + /// + /// or is . + /// public static async Task Time(this IStatsDPublisher publisher, string bucket, Func action) { using (var timer = StartTimer(publisher, bucket)) @@ -101,6 +116,9 @@ public static async Task Time(this IStatsDPublisher publisher, string bucket, Fu /// /// The value from invoking . /// + /// + /// or is . + /// public static T Time(this IStatsDPublisher publisher, string bucket, Func func) { using (StartTimer(publisher, bucket)) @@ -120,6 +138,9 @@ public static T Time(this IStatsDPublisher publisher, string bucket, Func /// /// The value from invoking . /// + /// + /// or is . + /// public static T Time(this IStatsDPublisher publisher, string bucket, Func func) { using (var timer = StartTimer(publisher, bucket)) @@ -139,6 +160,9 @@ public static T Time(this IStatsDPublisher publisher, string bucket, Func /// A representing the asynchronous operation to time. /// + /// + /// or is . + /// public static async Task Time(this IStatsDPublisher publisher, string bucket, Func> func) { using (StartTimer(publisher, bucket)) @@ -158,6 +182,9 @@ public static async Task Time(this IStatsDPublisher publisher, string buck /// /// A representing the asynchronous operation to time. /// + /// + /// or is . + /// public static async Task Time(this IStatsDPublisher publisher, string bucket, Func> func) { using (var timer = StartTimer(publisher, bucket)) From fc910d64cba4017dff5f1f9696928e8bfc5ba746 Mon Sep 17 00:00:00 2001 From: martincostello Date: Thu, 13 Dec 2018 14:11:22 +0000 Subject: [PATCH 5/9] Update comments on SocketProtocol Update the XML comments for the SocketProtocol enumeration. --- src/JustEat.StatsD/SocketProtocol.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/JustEat.StatsD/SocketProtocol.cs b/src/JustEat.StatsD/SocketProtocol.cs index e0327336..e32b78d5 100644 --- a/src/JustEat.StatsD/SocketProtocol.cs +++ b/src/JustEat.StatsD/SocketProtocol.cs @@ -1,9 +1,11 @@ +using System.Net.Sockets; + namespace JustEat.StatsD { /// - /// The subset of ProtocolType that are supported by SocketTransport - /// UDP or IP. - /// UDP is the default, but IP transport is required for AWS Lambdas. + /// An emueration defining the subset of values that are supported by . + /// + /// UDP is the default, but IP transport is required for some environments such as AWS Lambda functions. /// public enum SocketProtocol { From bfd3611b3e4f54d195c305ec90866264439e08aa Mon Sep 17 00:00:00 2001 From: martincostello Date: Thu, 13 Dec 2018 14:18:08 +0000 Subject: [PATCH 6/9] Fix typo Spell "enumeration" correctly. --- src/JustEat.StatsD/SocketProtocol.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/JustEat.StatsD/SocketProtocol.cs b/src/JustEat.StatsD/SocketProtocol.cs index e32b78d5..8694d958 100644 --- a/src/JustEat.StatsD/SocketProtocol.cs +++ b/src/JustEat.StatsD/SocketProtocol.cs @@ -3,7 +3,7 @@ namespace JustEat.StatsD { /// - /// An emueration defining the subset of values that are supported by . + /// An enumeration defining the subset of values that are supported by . /// /// UDP is the default, but IP transport is required for some environments such as AWS Lambda functions. /// From 87e11b2749dd3b6f37827637caf8e73d418aa2a0 Mon Sep 17 00:00:00 2001 From: martincostello Date: Thu, 13 Dec 2018 14:36:39 +0000 Subject: [PATCH 7/9] Rename property Rename StatName to Bucket. --- src/JustEat.StatsD/DisposableTimer.cs | 10 +++++----- src/JustEat.StatsD/IDisposableTimer.cs | 2 +- .../JustEat.StatsD.Tests/Extensions/ExtensionsTests.cs | 6 +++--- .../Extensions/SimpleTimerStatNameTests.cs | 8 ++++---- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/JustEat.StatsD/DisposableTimer.cs b/src/JustEat.StatsD/DisposableTimer.cs index 70a7b26c..0931bcb4 100644 --- a/src/JustEat.StatsD/DisposableTimer.cs +++ b/src/JustEat.StatsD/DisposableTimer.cs @@ -10,7 +10,7 @@ internal sealed class DisposableTimer : IDisposableTimer private IStatsDPublisher _publisher; private Stopwatch _stopwatch; - public string StatName { get; set; } + public string Bucket { get; set; } public DisposableTimer(IStatsDPublisher publisher, string bucket) { @@ -21,7 +21,7 @@ public DisposableTimer(IStatsDPublisher publisher, string bucket) throw new ArgumentNullException(nameof(bucket)); } - StatName = bucket; + Bucket = bucket; _stopwatch = Stopwatch.StartNew(); } @@ -32,12 +32,12 @@ public void Dispose() _disposed = true; _stopwatch.Stop(); - if (string.IsNullOrEmpty(StatName)) + if (string.IsNullOrEmpty(Bucket)) { - throw new InvalidOperationException($"The {nameof(StatName)} property must have a value."); + throw new InvalidOperationException($"The {nameof(Bucket)} property must have a value."); } - _publisher.Timing(_stopwatch.Elapsed, StatName); + _publisher.Timing(_stopwatch.Elapsed, Bucket); _stopwatch = null; _publisher = null; diff --git a/src/JustEat.StatsD/IDisposableTimer.cs b/src/JustEat.StatsD/IDisposableTimer.cs index 325ce90f..f1c937c9 100644 --- a/src/JustEat.StatsD/IDisposableTimer.cs +++ b/src/JustEat.StatsD/IDisposableTimer.cs @@ -10,6 +10,6 @@ public interface IDisposableTimer : IDisposable /// /// Gets or sets the name of the StatsD bucket associated with the timer. /// - string StatName { get; set; } + string Bucket { get; set; } } } diff --git a/tests/JustEat.StatsD.Tests/Extensions/ExtensionsTests.cs b/tests/JustEat.StatsD.Tests/Extensions/ExtensionsTests.cs index b052fbdf..230734ed 100644 --- a/tests/JustEat.StatsD.Tests/Extensions/ExtensionsTests.cs +++ b/tests/JustEat.StatsD.Tests/Extensions/ExtensionsTests.cs @@ -29,7 +29,7 @@ public static void CanChangeStatName() using (var timer = publisher.StartTimer("defaultName")) { Delay(); - timer.StatName = "otherStat"; + timer.Bucket = "otherStat"; } PublisherAssertions.SingleStatNameIs(publisher, "otherStat"); @@ -120,7 +120,7 @@ public static void CanChangeStatNameInAction() publisher.Time("defaultName", t => { Delay(); - t.StatName = "otherStat"; + t.Bucket = "otherStat"; }); PublisherAssertions.SingleStatNameIs(publisher, "otherStat"); @@ -182,7 +182,7 @@ public static async Task CanChangeStatNameInAsyncFunction() await publisher.Time("defaultName", async t => { var result = await DelayedAnswerAsync(); - t.StatName = "afterTheAwait"; + t.Bucket = "afterTheAwait"; return result; }); diff --git a/tests/JustEat.StatsD.Tests/Extensions/SimpleTimerStatNameTests.cs b/tests/JustEat.StatsD.Tests/Extensions/SimpleTimerStatNameTests.cs index b771c06a..22c60266 100644 --- a/tests/JustEat.StatsD.Tests/Extensions/SimpleTimerStatNameTests.cs +++ b/tests/JustEat.StatsD.Tests/Extensions/SimpleTimerStatNameTests.cs @@ -28,7 +28,7 @@ public static void CanChangeStatNameDuringOperation() using (var timer = publisher.StartTimer("initialStat")) { Delay(); - timer.StatName = "changedValue"; + timer.Bucket = "changedValue"; } PublisherAssertions.SingleStatNameIs(publisher, "changedValue"); @@ -42,7 +42,7 @@ public static void StatNameCanBeAppended() using (var timer = publisher.StartTimer("Some.")) { Delay(); - timer.StatName += "More"; + timer.Bucket += "More"; } PublisherAssertions.SingleStatNameIs(publisher, "Some.More"); @@ -69,7 +69,7 @@ public static void StatWithoutNameAtEndThrows() using (var timer = publisher.StartTimer("valid.Stat")) { Delay(); - timer.StatName = null; + timer.Bucket = null; } }); @@ -88,7 +88,7 @@ public static void StatNameIsInitialValueWhenExceptionIsThrown() using (var timer = publisher.StartTimer("initialStat")) { Fail(); - timer.StatName = "changedValue"; + timer.Bucket = "changedValue"; } } catch (Exception) From f8a61978d2b122d66b925918bab32d99d14e0f3e Mon Sep 17 00:00:00 2001 From: martincostello Date: Thu, 13 Dec 2018 14:44:21 +0000 Subject: [PATCH 8/9] Add null checks for delegates Check that actions and functions are not null and throw if they are. --- src/JustEat.StatsD/TimerExtensions.cs | 56 +++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/src/JustEat.StatsD/TimerExtensions.cs b/src/JustEat.StatsD/TimerExtensions.cs index b06f40f1..06315e07 100644 --- a/src/JustEat.StatsD/TimerExtensions.cs +++ b/src/JustEat.StatsD/TimerExtensions.cs @@ -34,10 +34,15 @@ public static IDisposableTimer StartTimer(this IStatsDPublisher publisher, strin /// The bucket to publish the timer for. /// A delegate to a method whose invocation should be timed. /// - /// or is . + /// , or is . /// public static void Time(this IStatsDPublisher publisher, string bucket, Action action) { + if (action == null) + { + throw new ArgumentNullException(nameof(action)); + } + using (StartTimer(publisher, bucket)) { action(); @@ -52,10 +57,15 @@ public static void Time(this IStatsDPublisher publisher, string bucket, Action a /// The bucket to publish the timer for. /// A delegate to a method whose invocation should be timed. /// - /// or is . + /// , or is . /// public static void Time(this IStatsDPublisher publisher, string bucket, Action action) { + if (action == null) + { + throw new ArgumentNullException(nameof(action)); + } + using (var timer = StartTimer(publisher, bucket)) { action(timer); @@ -73,10 +83,15 @@ public static void Time(this IStatsDPublisher publisher, string bucket, Action representing the asynchronous operation to time. /// /// - /// or is . + /// , or is . /// public static async Task Time(this IStatsDPublisher publisher, string bucket, Func action) { + if (action == null) + { + throw new ArgumentNullException(nameof(action)); + } + using (StartTimer(publisher, bucket)) { await action().ConfigureAwait(false); @@ -95,10 +110,15 @@ public static async Task Time(this IStatsDPublisher publisher, string bucket, Fu /// A representing the asynchronous operation to time. /// /// - /// or is . + /// , or is . /// public static async Task Time(this IStatsDPublisher publisher, string bucket, Func action) { + if (action == null) + { + throw new ArgumentNullException(nameof(action)); + } + using (var timer = StartTimer(publisher, bucket)) { await action(timer).ConfigureAwait(false); @@ -117,10 +137,15 @@ public static async Task Time(this IStatsDPublisher publisher, string bucket, Fu /// The value from invoking . /// /// - /// or is . + /// , or is . /// public static T Time(this IStatsDPublisher publisher, string bucket, Func func) { + if (func == null) + { + throw new ArgumentNullException(nameof(func)); + } + using (StartTimer(publisher, bucket)) { return func(); @@ -139,10 +164,15 @@ public static T Time(this IStatsDPublisher publisher, string bucket, Func /// The value from invoking . /// /// - /// or is . + /// , or is . /// public static T Time(this IStatsDPublisher publisher, string bucket, Func func) { + if (func == null) + { + throw new ArgumentNullException(nameof(func)); + } + using (var timer = StartTimer(publisher, bucket)) { return func(timer); @@ -161,10 +191,15 @@ public static T Time(this IStatsDPublisher publisher, string bucket, Func representing the asynchronous operation to time. /// /// - /// or is . + /// , or is . /// public static async Task Time(this IStatsDPublisher publisher, string bucket, Func> func) { + if (func == null) + { + throw new ArgumentNullException(nameof(func)); + } + using (StartTimer(publisher, bucket)) { return await func().ConfigureAwait(false); @@ -183,10 +218,15 @@ public static async Task Time(this IStatsDPublisher publisher, string buck /// A representing the asynchronous operation to time. /// /// - /// or is . + /// , or is . /// public static async Task Time(this IStatsDPublisher publisher, string bucket, Func> func) { + if (func == null) + { + throw new ArgumentNullException(nameof(func)); + } + using (var timer = StartTimer(publisher, bucket)) { return await func(timer).ConfigureAwait(false); From 5487adf3f8c462a412df39e2a38dfaf6b36552b3 Mon Sep 17 00:00:00 2001 From: martincostello Date: Thu, 13 Dec 2018 14:53:41 +0000 Subject: [PATCH 9/9] Change description Change the /// comment as suggested. --- src/JustEat.StatsD/IDisposableTimer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/JustEat.StatsD/IDisposableTimer.cs b/src/JustEat.StatsD/IDisposableTimer.cs index f1c937c9..9f60575e 100644 --- a/src/JustEat.StatsD/IDisposableTimer.cs +++ b/src/JustEat.StatsD/IDisposableTimer.cs @@ -8,7 +8,7 @@ namespace JustEat.StatsD public interface IDisposableTimer : IDisposable { /// - /// Gets or sets the name of the StatsD bucket associated with the timer. + /// Gets or sets the StatsD bucket associated with the timer. /// string Bucket { get; set; } }