search

lukevenediger / statsd-csharp-client

A simple c# client library for statsd and statsd.net
MIT License
47 stars 20 forks source link

Add float counter overloads #16

Closed TristanRhodes closed 7 years ago

niemyjski commented 8 years ago

Thanks for your PR, I've added some comments to it :)

TristanRhodes commented 8 years ago

Huh. I didn't mean to PR the fork, was doing the PR to master for a colleague. I think I just failed at GitHub :)

I spent a while making the code work with the tests, I'm happy to roll back the changes, but I'll also need to update the tests. How do you want that done? Cheers Tristan

  From: Blake Niemyjski <notifications@github.com>

To: lukevenediger/statsd-csharp-client statsd-csharp-client@noreply.github.com Cc: Tristan Rhodes TristanJERhodes@yahoo.co.uk; Author author@noreply.github.com Sent: Friday, 3 June 2016, 18:24 Subject: Re: [lukevenediger/statsd-csharp-client] Add float counter overloads (#16)

Thanks for your PR, I've added some comments to it :)— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

niemyjski commented 8 years ago

I think it all looked good, just don't think that we should be throwing exceptions for stats if the number is less than 0.

niemyjski commented 8 years ago

I don't think we should be rolling back any tests :) unless they are checking for an exception to be thrown.. @lukevenediger @TristanRhodes what do you think?

TristanRhodes commented 8 years ago

I made the changes to get the tests here passing: https://github.com/lukevenediger/statsd-csharp-client/blob/master/StatsdClientTests/StatsdTests.cs#L36

They are all expecting an ArgumentOutOfRangeException or an ArgumentNullException, but I think due to the refactoring to make the statsD client Async, they are now all catching AggregateExceptions which is causing the tests to fail. (At least it was in the NCrunch runner).

I can revert and let it throw the AggregateExceptions but then we will need to update the tests to drill into that and check that it has the correct internal exception.

Some tests also were expecting exceptions which did not occur as the throw seems to have been replaced with Trace.Log.

Let me know if you're happy for me to change the tests to work with the original code, excluding the float / double change.

niemyjski commented 8 years ago

The solution is to do .GetAwaiter.GetResult(); instead of .Wait(); && .Result. This will cause it to block but bubble up the real exception.

Also, anytime we are doing a GetResult we should be calling .ConfigureAwait(false).GetAwaiter.GetResult() in the core library (never do ConfigureAwait(false) in UI or test projects). If you update this, it should fix the issues you ran into 👍

niemyjski commented 7 years ago

@TristanRhodes any updates on this?

niemyjski commented 7 years ago

@TristanRhodes I just merged this in manually and gave you credit...