Conversation
…sharp-client into olivierg/prepare-client-side-aggregation # Conflicts: # src/StatsdClient/SerializedMetric.cs # src/StatsdClient/StatsdBuilder.cs # tests/StatsdClient.Tests/Bufferize/BufferBuilderTests.cs
truthbk
left a comment
truthbk
left a comment
There was a problem hiding this comment.
This looks good to me! Nice PR as always.
| private readonly bool _truncateIfTooLong; | ||
| private readonly IStopWatchFactory _stopwatchFactory; | ||
| private readonly IRandomGenerator _randomGenerator; | ||
| private readonly Pool<Stats> _pool; |
There was a problem hiding this comment.
Were you able to spot any performance regressions with this new pool? (I don't see anything obvious, but asking just in case)
There was a problem hiding this comment.
I have added a pool of Stats but removed a pool of SerializedMetric. SerializedMetric is instantiated once at https://github.com/DataDog/dogstatsd-csharp-client/pull/133/files#diff-ec21dde55792b9925788589e847dea98R15.
I do not think there is a performance regression as there is the same number of call of the pool as before.
| throw new ArgumentException($"{nameof(SendMetric)} does not support `MetricType.Set`."); | ||
| } | ||
|
|
||
| if (_randomGenerator.ShouldSend(sampleRate)) |
There was a problem hiding this comment.
How are sample rates handled in the agent? Should we do this twice when not aggregating? Sampling should only be done once.
There was a problem hiding this comment.
Good catch. A metric with different values of sampleRate cannot be aggregated directly.
There was a problem hiding this comment.
In this PR, there is no client side aggregation so the fix must be done in #134.
| /// <param name="tags">Array of tags to be added to the data.</param> | ||
| /// <typeparam name="T">The type of the value.</typeparam> | ||
| public void Counter<T>(string statName, T value, double sampleRate = 1.0, string[] tags = null) | ||
| public void Counter(string statName, double value, double sampleRate = 1.0, string[] tags = null) |
There was a problem hiding this comment.
The generic to double should not break the API as an implicit cast would happen here. Correct?
There was a problem hiding this comment.
There is an implicit cast to double when no information is lost.
The problem is that T can be BitInteger https://docs.microsoft.com/en-us/dotnet/api/system.numerics.biginteger?view=netcore-3.1 and there is no implicit conversion to double. It can also be the string "42" as the previous code only assumes that there is a ToString() method which is available for all objects in C#.
This is a breaking change even if it is unlikely to break anything.
There was a problem hiding this comment.
Do you plan to release this in a new major?
There was a problem hiding this comment.
I am going to release this in a new major as it may break compilation in some cases.
truthbk
left a comment
truthbk
left a comment
There was a problem hiding this comment.
I think this is good to go. We need to decide how we'll version the changes in master.
2 participants
This PR updates the current pipeline to prepare client side aggregation dev.
Before:
1 When calling
DogStatsdService.Increment, the metric was serialized as a raw data (byte[])2 The raw data is sent to the concurrent queue.
3 Another thread read the raw data from the queue and aggregated it into a buffer.
4 The buffer is sent
After:
1 When calling
DogStatsdService.Increment, the metric is stored as aStatsinstance2 The
Statsinstance is sent to the concurrent queue.3 Another thread read the
Statsinstance, serialized it to raw data and aggregated into a buffer.4 The buffer is sent
Implementation notes:
Generic was removed from DogStatsdService and so this PR introduces a breaking change in the API. Only custom type should be impacted.
All metrics are sent as a double. For performance reasons,
Countingshould also be sent as long. It is not yet done and I am going to work on it in another PR.