pereingo / statsd-csharp-client Goto Github PK

It appears the nuspec file has the dependency for System.Net.NameResolution as system.net.nameresolution for .NETStandard1.3, and this causes issues when using this on a .net core application.

I've updated the nuget package locally to include this in the dependencies portion, and this works properly:

      <group targetFramework=".NETFramework4.5" />
      <group targetFramework=".NETStandard1.3">
        <dependency id="NETStandard.Library" version="[1.6.0, )" />
        <dependency id="System.Net.NameResolution" version="[4.0.0, )" />
      </group>
    </dependencies>

StatsdUDP is IDisposable and apparently not disposing of it can cause leaks. This is because it wraps a UdpClient. Classes that wrap a disposable object often themselves become disposable in this way.

Should we document the requirement to dispose the UdpClient?
Alternatively, the StatsdUDP is used in the Statsd class, e.g.
Statsd s = new Statsd(new StatsdUDP(HOSTNAME, PORT));

If a StatsdUDP is never re-used across different Statsd, should Statsd become IDisposable too?

@goncalopereira started this project then I took it over, but I've recently moved jobs so I'm not longer using it at all. Don't really feel comfortable making changes for a client I don't personally use, so I'd rather find someone else to take ownership. Takers? @pekiZG I know you've been pretty active on issues/PR's, would you be interested? Still happy to chime in on design or code reviews, just don't want to be the one making the calls :)

Right now no checking is done to ensure that the UDP packet size limit is not exceeded when sending metrics - the SocketException is swallowed and nothing is sent. This can be a problem when the user uses Statsd.Add() to queue up a lot of metrics.

Although the version bump to 1.0.0.16 was committed after commit 2c09813, the Nuget package 1.0.0.16 does not contain the changes from that commit.

Any idea what's going on?

After the change from UdpClient to UdpSocket I'm unable to connection to my statsd server from an asp.net web application.

The following exception is thrown when calling Metrics.Configure:
{"No such host is known"}
[System.Net.Sockets.SocketException]: {"No such host is known"}
Data: {System.Collections.ListDictionaryInternal}
HelpLink: null
HResult: -2147467259
InnerException: null
Message: "No such host is known"
Source: "System"
StackTrace: " at System.Net.Dns.InternalGetHostByAddress(IPAddress address, Boolean includeIPv6)\r\n at System.Net.Dns.GetHostEntry(String hostNameOrAddress)\r\n at StatsdClient.StatsdUDP..ctor(String name, Int32 port)\r\n at StatsdClient.Metrics.Configure(MetricsConfig config)\r\n at WebApplication1.Global.Application_Start(Object sender, EventArgs e) in c:\Users\tskauge.247MS\Development\WebApplication1\WebApplication1\Global.asax.cs:line 24"
TargetSite: {System.Net.IPHostEntry InternalGetHostByAddress(System.Net.IPAddress, Boolean)}

Steps to reproduce:

1. Create a new empty web application
2. Add latest statsd client via nuget (Install-Package StatsdClient)
3. Configure Metrics in Global asax to connect send metrics to existing server using the IP address of the server (note: not the server name - not all servers has names).
4. Start the application

Result: The exception above is thrown
Expected: No exception is thrown

I don't know the exact reason for replacing the UdpClient with this new implementation. This is why I'm creating this as an issue. I can however report that the previous implementation worked perfectly for me.

This bug also occurs if the Statsd Client is used in a console application where the host name is an ipaddress.

Hi,

I have noticed that Gauge is accepting and sends double values which are not accepted by the Jroland/StatsdNet service. For it to work with the service, the value should be an integer.

The following fatal exception occurs in our app:

Unhandled Exception: System.IndexOutOfRangeException: Index was outside the bounds of the array.
  at System.Collections.Generic.List`1.Add(T item)
  at StatsdClient.Statsd.Add[TCommandType](String name, Double value)
...

I believe this may be a thread safety issue. Is access to the Commands List thread safe?

Hello. Thank you for this great library.
Any plans to add async support for Send operations?

It would be nice to configure via MetricsConfig a global sample rate for all metrics.

Hi,

I have modified the source StatsUDP.cs to use the socket class instead of UdpClient, because UdpClient is not thread safe. So far it's doing fine. I wonder if it's something you've tried before?

using System;
using System.Net.Sockets;
using System.Text;
using System.Net;

namespace StatsdClient
{
public class StatsdUDP : IDisposable, IStatsdUDP
{
private string Name { get; set; }
private int Port { get; set; }
//private UdpClient UDPClient { get; set; }
private Socket sendSocket;
private IPAddress sendTo;
private EndPoint sendEndPoint;

    public StatsdUDP(string name, int port)
    {
        Name = name;
        Port = port;
        //UDPClient = new UdpClient(Name, Port);           
        sendSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
        sendTo = IPAddress.Parse(Name);
        sendEndPoint = new IPEndPoint(sendTo, Port);
    }

    public void Send(string command)
    {
        byte[] encodedCommand = Encoding.ASCII.GetBytes(command);
        //UDPClient.Send(encodedCommand, encodedCommand.Length);

        sendSocket.SendTo(encodedCommand, SocketFlags.None, sendEndPoint);
    }

    public void Dispose()
    {
        //UDPClient.Close();
        sendSocket.Close();
    }
}

}

Rgds,
Patrick

I want to maintain a separate nuget project that is compatible for core clr. I read in the licence that I can publish this code. I just want to check with the owner. I will mention it in the description of package. Please let me know. Thanks !

I know this maybe preventive optimization, but is string.Format in GetCommand really needed? Maybe simple string.Concat will suffice?

On many modern UNIX systems localhost has both IPv4 (127.0.0.1) and IPv6 (::1) entries.

Therefore simple setting StatsdServerName = "localhost" fails with:

Cannot handle address family 49744
Stacktrace:

  at <unknown> <0xffffffff>
  at (wrapper managed-to-native) System.Net.Dns.GetHostByName_internal (string,string&,string[]&,string[]&) <0xffffffff>
  at System.Net.Dns.GetHostByName (string) <0x0004d>
  at System.Net.Dns.GetHostEntry (string) <0x000a5>
  at StatsdClient.StatsdUDP.GetIpv4Address (string) <0x0004f>
  at StatsdClient.StatsdUDP..ctor (string,int,int) <0x000f1>
  at StatsdClient.Metrics.CreateStatsD (StatsdClient.MetricsConfig) <0x000e5>
  at StatsdClient.Metrics.Configure (StatsdClient.MetricsConfig) <0x000c7>

Environment: recent Mono (4.3.0 built from git, @jpasichnyk reports that for Mono 4.0.1 as well)

I noticed what appears to be a bug in the TCP sending mechanism. The send method disposes of the socket after sending any metric and hence any further metrics are not sent. This might be an issue with .Net core only as the Framework version only closes the socket.

Hello,

Could you please sign the packages generated? We are very fond of your library, but have to re-download it every time, compile it and sign it ourselves, which is tedious at best.

Having an issue where counters are sent from multiple threads on the same process and duplicate messages are being sent from one thread but the message from the other thread is not. I have switched to a fork which tackles this (https://github.com/Kyle2123/statsd-csharp-client) but it is not as up to date as you repo.

From @AnthonySteele (on the 7digital fork, which is now gone)...

We have an assembly called "Configuration" here. IMHO this isn't a very good name for a couple of reasons.

When you see "Configuration.dll" in the bin folder, it's hard to tell which reference it came from, or if it's in-house or from some third party kit. It's not a "world-configuration" library so it shouldn't be named as such.
When you reference it in code, it can easily clash with namespaces inside the project. it's hard to reference a namespace called "Configuration" in a class called .e.g. My.Api.Infrastructure.Configuration.ConnectionStringProvider you have to say global::Configuration otherwise the compiler assumes you mean your own parent namespace.
It's also really small and doesn't do much which is a design smell for a separate .dll.

This got a +1 from @knocte too.

The Metrics.Time methods should have async overloads. Requires upgrading the project to .NET 4.5. Not sure if .NET 3.5 is still worth supporting?

Statsd server support tcp connection as well as udp. This is sometimes needed especially for accurate counters.

The gauge specification by Etsy allows for sending updates to the current gauge value by prepending the value submitted with an explicit '+' or '-'.

Unfortunately, this functionality is missing from this statsd client. Adding an optional argument (isDelta=false) to the Gauge function would not break behaviour of existing users but allow for this usecase.

Is it possible to completely disable the library or even disable it at run time? This could come in handy when there is no need to run it at production in order to reduce the over head.

Hi,

Is it in the roadmap support/upgrade to .net coreclr?
Thanks.

When timing a block of code or a method using Metrics (running Metrics.Time(() => DoMagic(), "stat-name"); or using (Metrics.StartTimer("stat-name")) { DoMagic(); }), if an unhandled exception is thrown in the block/method, a statsd timer metric will still be submitted (that contains the time spent executing up to the point that the unhandled exception occurred.

Not sure if this is intended functionality. Personally I don't think that it makes sense to submit a metric in this case since the code doesn't completely execute, thus making it an invalid data point.

Are there any plans for Async support?

Courtesy of @AnthonySteele...

Your code has:

       public static T Time<T>(Func<T> func, string statName)
        {
            using (StartTimer(statName))
            {
                return func();
            }
        }

This will not do the right thing for async code, e.g. when the type T is actually "async Task"
You will always record a time that is much shorter than the actual duration. In simple terms, you are recording the time taken to produce the Task, not the time taken for the task to complete and produce the int.

The C# compiler generates a lot of code when there is an "await", this code is suspended, and resumes to do stuff when the task is actually complete. And without that "await" you don't get this, you just time the production of the task.

You need an overload like this:

       public static async Task<T> Time<T>(Func<Task<T>> func, string statName)
        {
            using (StartTimer(statName))
            {
                return await func();
            }
        }

I tested and confirmed this here and made code for it here