ReplaceStatsd class by several smaller components. #123
Conversation
truthbk
left a comment
truthbk
left a comment
There was a problem hiding this comment.
Looks great, just a couple of comments! Some of my comments may be rendere moot by the work you had on the other PRs that were stacked on top.
|
|
||
| public class Event | ||
| { | ||
| private const int MaxSize = 8 * 1024; |
There was a problem hiding this comment.
Should we make MaxSize configurable? I think I mentioned elsewhere, this 8Kb limit might not work well with remote UDP as it will likely require fragmentation which might be a bad idea.
There was a problem hiding this comment.
This is a very good comment.
MaxSize could be StatsdMaxUDPPacketSize from https://github.com/DataDog/dogstatsd-csharp-client/blob/master/src/StatsdClient/StatsdConfig.cs#L83, but if the message size cannot be reduced, the code throws an exception which is not caught (I can addtry / catch).
I am wondering if the logic of trying reducing the message size is really useful.
Indeed https://github.com/DataDog/dogstatsd-csharp-client/blob/master/src/StatsdClient/Bufferize/StatsBufferize.cs#L67-L70 already handle too long message. Adding an exception handler as in Java client will let the user know when something is wrong.
I think this change should be done in a separate PR, but I would love to hear your opinion about this topic.
2 participants
This PR replaces
Statsdclass by several smaller components to make code easier to maintain.Statsdis now unused but it is kept for backward compatibility reasons.For reviewer:
MetricSerializercontains a lot of code from the classStatsdthat could be improved, butMetricSerializeris going to be refactored in a later PR.