Zero allocating StatsDPublisher #104

dv00d00 · 2018-09-05T20:55:55Z

I've added a proof of concept implementation of buffer based statsd publisher.

Key points:

A concept of statsd message struct
Do not attempt formatting before sampling
Format directly into utf8
v2 transport interface uses ArraySegment which was in .net since 4.5
zero allocations in .net core 2.1 with pooled udp transport

Utf8FormatterBench

Method	Mean	Error	StdDev	Scaled	Gen 0	Allocated
StringBased	609.8 ns	2.1574 ns	0.1219 ns	1.00	0.2203	696 B
BufferBased	279.9 ns	0.1432 ns	0.0081 ns	0.46	-	0 B

StatSendingBenchmark

Method	Mean	Error	StdDev	Gen 0	Allocated
RunUdp	288.132 us	6.1692 us	0.3486 us	0.4883	2944 B
RunUdpWithSampling	30.062 us	2.3620 us	0.1335 us	0.1221	467 B
RunIp	264.734 us	9.0817 us	0.5131 us	0.4883	2944 B
RunIpWithSampling	29.965 us	2.8734 us	0.1624 us	0.1221	468 B
RunV2	62.660 us	1.9967 us	0.1128 us	-	0 B
RunV2WithSampling	6.438 us	0.3993 us	0.0226 us	-	0 B

dv00d00 · 2018-09-05T21:16:34Z

Open questions:

The fate of TimeStamp in gauges, see Unclear meaning of DateTime for Gauges #103
Max size of the encoded message. In theory, UDP datagrams could be as large as 8kb, in practice its usually 512 bytes max. Should we add an invariant on it?
Gauge does support sampling in statsd, should we expose it?
How to expose new functionality on a library level?

dv00d00 · 2018-09-05T21:24:54Z

Due to the purpose of this pr as it is 3rd in a series.

Infrastructural code, as in this repository, must run as fast as possible. Which in .net realities does mean it should also allocate as less as possible. We got tools to do so and therefore should.

slang25 · 2018-09-06T09:06:11Z

src/Benchmark/StatSendingBenchmark.cs

@@ -2,13 +2,16 @@
 using BenchmarkDotNet.Attributes;
 using JustEat.StatsD;
 using JustEat.StatsD.EndpointLookups;
+using JustEat.StatsD.V2;


Perhaps the new publisher, transport types and namespaces should have a name other than V2, naming stuff is hard though.

Something that indicates it offers better perf / zero allocations.

+1, V2 is confusing given that the library is at version 3.

slang25 · 2018-09-06T09:09:49Z

This PR is fantastic work @dv00d00, zero allocations and it's not a breaking change! 💯

martincostello

Overall this looks good, but there's some TODOs to deal with and I think this would be better done without all the "v2" duplication by making the existing concrete types support the "new" and "old" way.

I'd also probably argue that this would be v4, so we could finally make the pooled implementation the main implementation so we don't have to ship two, further reducing the duplication caused by making the pooled transport in 3.1 not be a breaking change.

martincostello · 2018-09-11T13:06:19Z

Directory.Build.props

@@ -24,5 +24,6 @@
    <RepositoryType>git</RepositoryType>
    <RepositoryUrl>$(PackageProjectUrl).git</RepositoryUrl>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
+    <LangVersion>7.3</LangVersion>


This is redundant, we already use the latest version here: https://github.com/justeat/JustEat.StatsD/blob/a3ee3711594b048e94be4068fba54dffd6c63094/Directory.Build.props#L14

src/JustEat.StatsD.Tests/Utf8FormatterTests.cs

+            var message = StatsDMessage.Timing(128, "bucket");
+            Check(message, 0.5, "prefix.bucket:128|ms|@0.5");
+        }
+


martincostello · 2018-09-11T13:08:27Z

src/JustEat.StatsD.Tests/Utf8FormatterTests.cs

+        private static void Check(StatsDMessage message, double sampleRate, string expected)
+        {
+            Formatter.TryFormat(message, sampleRate, Buffer, out int written).ShouldBe(true);
+            var result = Encoding.UTF8.GetString(Buffer.AsSpan().Slice(0, written));


I think there's an AsSpan() overload that lets you do this in one:

var result = Encoding.UTF8.GetString(Buffer.AsSpan(0, written));

src/JustEat.StatsD/JustEat.StatsD.csproj

martincostello · 2018-09-11T13:09:46Z

src/JustEat.StatsD/Properties/AssemblyInfo.cs


 [assembly: ComVisible(false)]
 [assembly: Guid("8f4ff09e-4130-4872-a50f-b290e9ccb04b")]
+
+[assembly:InternalsVisibleTo("JustEat.StatsD.Tests")]


Spaces after colons.

martincostello · 2018-09-11T13:42:33Z

src/JustEat.StatsD/V2/StatsDUtf8Formatter.cs

+                case StatsDMessage.Kind.Timing:
+                {
+                    if (!buffer.TryWriteLong(magnitudeIntegral)) return false;
+                    if (!buffer.TryWriteBytes((byte)'|', (byte)'m', (byte)'s')) return false;


martincostello · 2018-09-11T13:42:39Z

src/JustEat.StatsD/V2/StatsDUtf8Formatter.cs

+                        if (!buffer.TryWriteDouble(msg.Magnitude)) return false;;
+                    }
+
+                    if (!buffer.TryWriteBytes((byte)'|', (byte)'g')) return false;


martincostello · 2018-09-11T13:42:47Z

src/JustEat.StatsD/V2/StatsDUtf8Formatter.cs

+
+            if (sampleRate < 1.0 && sampleRate > 0.0)
+            {
+                if (!buffer.TryWriteBytes((byte)'|', (byte)'@')) return false;


martincostello · 2018-09-11T13:42:56Z

src/JustEat.StatsD/V2/StatsDUtf8Formatter.cs

+            written = buffer.Written;
+            return true;
+        }
+


Extra new line.

martincostello · 2018-09-11T13:43:28Z

src/JustEat.StatsD/V2/UdpTransportV2.cs

+            _endpointSource = endPointSource ?? throw new ArgumentNullException(nameof(endPointSource));
+        }
+
+        public void Send(ArraySegment<byte> metric)


As per previous comments, we should be able to make the existing types implement both ways.

dv00d00 · 2018-09-11T16:13:50Z

I am going to address V2 naming and structure in upcoming commits

added formatter tests

dv00d00 · 2018-09-12T21:05:52Z

Ok, I think I am done with refactoring and addressing issues, would appreciate another review

martincostello · 2018-09-12T21:29:10Z

src/JustEat.StatsD.Tests/WhenTheTransportThrows.cs

 using Shouldly;
 using Xunit;

+#pragma warning disable xUnit1026 // Theory methods should use all of their parameters disabled to render test case name


Can you provide a screenshot of the before and after with this warning disabled?

martincostello · 2018-09-12T21:30:13Z

src/JustEat.StatsD.Tests/WhenTheTransportThrows.cs

    }
+
+    public delegate IStatsDPublisher Factory(StatsDConfiguration config);


Could you not just use Func<StatsDConfiguration, IStatsDPublisher>?

martincostello · 2018-09-12T21:32:36Z

src/JustEat.StatsD/Buffered/BufferBasedStatsDPublisher.cs

+
+        public void Increment(long value, double sampleRate, params string[] buckets)
+        {
+            if (buckets == null)


Could also shortcut if buckets.Length == 0 to remove the need to allocate the enumerator.

martincostello · 2018-09-12T21:34:38Z

src/JustEat.StatsD/Buffered/BufferBasedStatsDPublisher.cs

+                    {
+                        // so we was not able to write to resized buffer
+                        // that means there is a bug in formatter
+                        throw new Exception("Utf8 Formatting Error. This is a bug." +


I'm not sure about throwing vague exceptions to users going "oh, there's a bug".

src/JustEat.StatsD/Buffered/BufferExtensions.cs

martincostello · 2018-09-12T21:42:00Z

src/JustEat.StatsD/JustEat.StatsD.csproj

+    <PackageReference Include="System.Memory" Version="4.5.1" />
+    <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="2.0.0" />
+  </ItemGroup>
+  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp2.1' ">


Could change this to Condition=" '$(TargetFramework)' != 'net451' " and then remove the need to reference it twice for both netstandard2.0 and netcoreapp2.1.

src/JustEat.StatsD/PooledUdpTransport.cs

martincostello · 2018-09-12T21:43:16Z

src/JustEat.StatsD/StatsDPublisher.cs


-        public StatsDPublisher(StatsDConfiguration configuration, IStatsDTransport transport)
+        public StatsDPublisher(StatsDConfiguration configuration, IStatsDTransport transport, bool preferBufferedTransport = false)


This is a binary breaking change - is that intentional?

martincostello · 2018-09-12T21:43:27Z

src/JustEat.StatsD/StatsDPublisher.cs

        }

-        public StatsDPublisher(StatsDConfiguration configuration)
+        public StatsDPublisher(StatsDConfiguration configuration, bool preferBufferedTransport = false)


src/JustEat.StatsD/UdpTransport.cs

dv00d00 · 2018-09-13T11:01:09Z

There is currently no way to specify a constructor on StasDPublisher which would accept IBufferedStatsDTransport, it needs to have the separate signature, otherwise, compiler complains over Ambiguous invocation.

martincostello · 2018-09-13T11:43:12Z

src/JustEat.StatsD/StatsDPublisher.cs

+        {
+        }
+
+        public StatsDPublisher(StatsDConfiguration configuration, bool preferBufferedTransport)


What's the reasoning behind making "better performance" opt-in? I think it would be better to add a constructor to accept the new interface, and have the existing two try use the better implementation, if possible, by default.

As I have added new transport interface implementation to the existing transports, adding a constructor with signature accepting transport would result in a compilation error for the consumer.

I will make buffered implementation a default though.

I'm not sure whether it's a good idea or not, but if you swapped the parameter order for the new one, it wouldn't be ambiguous to the compiler?

The compiler will accept it without issues. 😸
Looks hacky though.

Other options I can think of:

Static factory methods on StatsDPublisher

Dedicated factory class

Dedicated transports per interface

Expose new public implementation of BufferdPublisher, String publisher will retain the old name

martincostello · 2018-09-13T11:44:03Z

src/JustEat.StatsD/Buffered/IStatsDBufferedTransport.cs

@@ -2,7 +2,7 @@

 namespace JustEat.StatsD.Buffered


I think this would be better in JustEat.StatsD rather than JustEat.StatsD.Buffered to make it more discoverable.

martincostello · 2018-09-14T13:48:03Z

So I think just two things left on this for me:

Is there a way we can make the constructor for the publisher accept the new interface without creating ambiguity to the compiler? Maybe we could add a new factory type that could simplify the transport creation and hide the buffering/not away. Or maybe instead of the new bool argument we could add a new option to StatsDConfiguration that defaults to preferring buffering? Then we don't need to change the existing signatures at all.
Bump the version to 3.2.0.

martincostello · 2018-09-14T13:49:25Z

src/JustEat.StatsD/StatsDPublisher.cs

+
+            var transport = new PooledUdpTransport(endpointSource);
+
+            if (preferBufferedTransport)


As alluded to in my comment, maybe we could have if (configuration.PreferBufferedTransport) instead?

Now it seems so obvious, thx @martincostello

dv00d00 · 2018-09-14T14:54:17Z

pushed updates as requested

version.props

martincostello · 2018-09-14T15:00:40Z

src/JustEat.StatsD/StatsDPublisher.cs

-            _onError = configuration.OnError;
+            switch (transport)
+            {
+                case IStatsDBufferedTransport transportV2 when configuration.PreferBufferedTransport:


Now we've dropped V2, maybe this should be called bufferedTransport?

martincostello · 2018-09-14T15:01:19Z

src/JustEat.StatsD/StatsDPublisher.cs

-        private readonly StatsDMessageFormatter _formatter;
-        private readonly IStatsDTransport _transport;
-        private readonly Func<Exception, bool> _onError;
+        private readonly IStatsDPublisher _publisher;


As this class is now more of a wrapper, maybe call this _inner?

martincostello · 2018-09-14T15:03:37Z

I think I'm basically happy with this now as a 3.2.0 release - great job! I'd like @AnthonySteele to review it as well though before we merge it.

Can we get one last set of performance test results from the tip of this branch?

dv00d00 · 2018-09-14T15:31:07Z

This run of benchmarks is taken from the different laptop. So could not be compared directly.

Utf8FormatterBenchmark

Method	Mean	Error	StdDev	Median	Scaled	ScaledSD	Gen 0	Allocated
StringBased	876.9 ns	17.473 ns	35.69 ns	862.9 ns	1.00	0.00	0.4416	696 B
BufferBased	392.1 ns	7.869 ns	18.55 ns	389.7 ns	0.45	0.03	-	0 B

UdpStatSendingBenchmark

Method	Mean	Error	StdDev	Scaled	Gen 0	Allocated
SendStatUdp	1,131.69 us	22.6319 us	58.0142 us	1.00	-	1184 B
SendStatPooledUdp	31.63 us	0.6324 us	1.1240 us	0.03	0.1831	384 B
SendStatPooledUdpBuffered	30.85 us	0.5901 us	0.6796 us	0.03	-	0 B
SendStatPooledUdpCoveredByAdapter	31.96 us	0.6388 us	1.6374 us	0.03	-	0 B

StatSendingBenchmark

Method	Mean	Error	StdDev	Median	Gen 0	Allocated
RunUdp	2,503.119 us	49.9168 us	77.7144 us	2,485.259 us	-	2944 B
RunUdpWithSampling	199.564 us	3.8886 us	5.8202 us	199.324 us	0.2441	597 B
RunIp	2,432.112 us	47.6688 us	71.3484 us	2,416.822 us	-	2944 B
RunIpWithSampling	222.738 us	6.0002 us	17.1189 us	223.859 us	0.2441	601 B
RunBuffered	83.943 us	2.1686 us	6.3943 us	85.700 us	-	0 B
RunBufferedWithSampling	6.422 us	0.1279 us	0.1617 us	6.432 us	-	0 B

slang25 · 2018-09-17T17:15:19Z

src/JustEat.StatsD/Buffered/Buffer.cs

+{
+    internal ref struct Buffer
+    {
+        public Buffer(Span<byte> source) : this()


Last minute thought, what's with : this(), isn't that redundant?

thx, good catch

AnthonySteele · 2018-09-20T12:11:58Z

As discussed with marticC

Look good, might need some trials under load.

Not sure if this will be V3.2 with no / minimal breaking changes; or 4.0 with other things tidied up as well.

When we can have breaking changes, we don't need both code paths: if we have two classes that work the same, and we know that they both work well in production loads, then we only need one of them (the faster / lower allocation one), there is no real user choice here. So that extra bool PreferBufferedTransport will fall away.

There still in a choice of Udp vs. Ip transports, we need both for different cases (e.g. AWS lambda). The idea of "a new factory type that could simplify the transport creation and hide the details" sounds worthwhile (though there is something like that in StatsDServiceCollectionExtensions ). So make the built-in, common cases trivial, and keep the existing more complex ways for users to plug in their own code overrides. I prefer enum options to bool in this case - adding new values to an enum is less disruptive. it's not user extensible but it's more flexible than bools.

dv00d00 · 2018-10-01T09:43:12Z

Any updates on this one @martincostello @AnthonySteele ?

martincostello · 2018-10-01T10:45:10Z

I think we just need to decide whether this is a 3.2.0 or a 4.0.0 (as this PR changed the number, if it's going to be a 4.0.0 we should update it here before merging), then this is good to merge.

AnthonySteele · 2018-10-01T13:21:49Z

If we go for a 4.0.0 then merge this and allow other changes before finalising it.

martincostello · 2018-10-01T13:26:23Z

So which are we doing, 3.2.0 or 4.0.0?

martincostello · 2018-10-02T10:07:51Z

Myself and @AnthonySteele discussed this offline, and we're going to release this as a 3.2.0 later today as something users can opt-in to if they want to use it. Then we'll make this and the changes from #81 the defaults and no longer an opt-in as part of a future 4.0.0 release.

martincostello · 2018-10-02T11:23:43Z

https://www.nuget.org/packages/JustEat.StatsD/3.2.0

Dmitry Kushnir and others added 4 commits September 5, 2018 18:38

poc

754ea27

tidy up

3da35f6

renaming

00e726e

tests

59ae9ea

dv00d00 requested a review from a team as a code owner September 5, 2018 20:55

projects cleanup

a3ee371

slang25 reviewed Sep 6, 2018

View reviewed changes

martincostello reviewed Sep 11, 2018

View reviewed changes

react to code review part one

77ef965

Dmitry Kushnir added 5 commits September 11, 2018 21:18

react to codereview part two

dffa0ea

react to code review part two

98b915b

refactored utf8 formatter a bit

60c0ca4

removed global buffer size

94d9e99

added formatter tests

naming and more tests

0d0bea0

Dmitry Kushnir added 2 commits September 13, 2018 00:11

few more fixes

4ad8de8

mini fixes

9f375fe

martincostello reviewed Sep 12, 2018

View reviewed changes

react to code review 2

dcc64ee

martincostello reviewed Sep 13, 2018

View reviewed changes

Dmitry Kushnir added 2 commits September 13, 2018 21:36

fixes

2fe60c6

fixed tests

e449c14

martincostello reviewed Sep 14, 2018

View reviewed changes

moved transport switch into the StatsDConfig

243c185

bumped version to 3.2.0

06d3c93

martincostello reviewed Sep 14, 2018

View reviewed changes

version.props Show resolved Hide resolved

Dmitry Kushnir added 2 commits September 14, 2018 17:58

csproj celanup

70a9e7d

appveyor version bump

44c0999

martincostello reviewed Sep 14, 2018

View reviewed changes

martincostello requested a review from AnthonySteele September 14, 2018 15:02

Dmitry Kushnir added 3 commits September 14, 2018 18:08

get rid of v2

d38482e

include benchmark for adapted statsd publisher

0a5b655

updated benchmarks

617ce3b

slang25 reviewed Sep 17, 2018

View reviewed changes

removed redundant call

2b6f2af

AnthonySteele approved these changes Sep 20, 2018

View reviewed changes

slang25 approved these changes Oct 1, 2018

View reviewed changes

martincostello merged commit b1edccc into justeattakeaway:master Oct 2, 2018

martincostello added this to the v3.2.0 milestone Oct 2, 2018

This was referenced Oct 3, 2018

Incorrect timing stats published in 3.2.0 #110

Closed

Implement IDisposable on StatsDPublisher #125

Merged

Fix unbuffered batch metrics send #128

Merged

		}

		public delegate IStatsDPublisher Factory(StatsDConfiguration config);


		public StatsDPublisher(StatsDConfiguration configuration, IStatsDTransport transport)
		public StatsDPublisher(StatsDConfiguration configuration, IStatsDTransport transport, bool preferBufferedTransport = false)


		var transport = new PooledUdpTransport(endpointSource);

		if (preferBufferedTransport)

Zero allocating StatsDPublisher #104

Zero allocating StatsDPublisher #104

Conversation

dv00d00 commented Sep 5, 2018

dv00d00 commented Sep 5, 2018 • edited Loading

dv00d00 commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slang25 commented Sep 6, 2018

martincostello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dv00d00 commented Sep 11, 2018

dv00d00 commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dv00d00 commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martincostello commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dv00d00 commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martincostello commented Sep 14, 2018

dv00d00 commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonySteele commented Sep 20, 2018 • edited Loading

dv00d00 commented Oct 1, 2018

martincostello commented Oct 1, 2018

AnthonySteele commented Oct 1, 2018

martincostello commented Oct 1, 2018

martincostello commented Oct 2, 2018

martincostello commented Oct 2, 2018

dv00d00 commented Sep 5, 2018 •

edited

Loading

AnthonySteele commented Sep 20, 2018 •

edited

Loading