statsd: support sending counters and gauges with timestamp.#185
statsd: support sending counters and gauges with timestamp.#185
Conversation
This PR adds the `timestamp` parameter to `Gauge()` and `Counter()`. Metrics sent with a `timestamp` to the DogStatsD server will be directly sent to the intake with no aggregation done server-side. Even if the client side aggregation is enabled, metrics submitted with a timestamp won't go through any aggregation, they will be written in the serialization buffer as-is.
remeh
force-pushed
the
remeh/timed-sample
branch
from
928facc to
3edcc86
Compare
| // available on older versions of .Net. | ||
| // Snippet from: https://github.com/dotnet/dotnet-api-docs/blob/13d97c7/snippets/csharp/System/DateTimeOffset/ToUnixTimeSeconds/tounixtimeseconds1.cs | ||
| // License: https://github.com/dotnet/dotnet-api-docs/blob/13d97c7/LICENSE-CODE | ||
| internal class DateTimeOffsetHelper |
There was a problem hiding this comment.
I am going to remove the support for old frameworks and so this code will not be needed anymore. I will open a PR soon.
| { | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
I suggest merging if (metric.Timestamp > 0) with if (_optionalCountAggregator != null), ie if (_optionalCountAggregator != null && metric.Timestamp == 0) to highlight that aggregation happens when it is enabled and when metric.Timestamp > 0.
| } | ||
|
|
||
| [Test] | ||
| public void SendCounterWithTimestamp() |
There was a problem hiding this comment.
I think this is not needed to have 3 tests as the timestamp is just append at the end.
There was a problem hiding this comment.
It validates that the timestamp is correctly appended in every situation (i.e. when there is a sample rate and when there is not, when there is tags and when there is not).
There was a problem hiding this comment.
Your change is very simple https://github.com/DataDog/dogstatsd-csharp-client/pull/185/files#diff-9b6622ef9d4ae2b0b231e2334fb6e961f88421564541be2ae1021429795e6e1dR57-R61 and unit tests need to be maintain over the time.
There was a problem hiding this comment.
While I agree that current code is just appending, units test are not here to test current code but to test current code and its future changes. Right now the implementation is basically appending to the end, in the future it may be different and, I want to test that there is a timestamp regardless if there is a sample rate, tags, or none of these.
remeh
force-pushed
the
remeh/timed-sample
branch
from
1d8e8c5 to
875f669
Compare
| @@ -1,343 +1,427 @@ | |||
| using System; | |||
There was a problem hiding this comment.
The end of the lines were changed.
2 participants
This PR adds the
timestampparameter toGauge()andCounter().Metrics sent with a
timestampto the DogStatsD server will be directly sent to the intake with no aggregation done server-side.Even if the client side aggregation is enabled, metrics submitted with a timestamp won't go through any aggregation, they will be written in the serialization buffer as-is.