Adding dogstatsd telemetry to the client #97
Conversation
hush-hush
force-pushed
the
maxime/telemetry
branch
3 times, most recently
from
466ca4d
to
223f420
Compare
hush-hush
force-pushed
the
maxime/telemetry
branch
from
223f420
to
9bbdda3
Compare
hush-hush
changed the title
hush-hush
force-pushed
the
maxime/telemetry
branch
from
9bbdda3
to
f75b87a
Compare
|
Approving for the .md docs file, but please add another dev to review the java portion. |
hush-hush
force-pushed
the
maxime/telemetry
branch
2 times, most recently
from
bb4611e
to
8bd4026
Compare
hush-hush
force-pushed
the
maxime/telemetry
branch
2 times, most recently
from
7e0532a
to
fad5caf
Compare
| @@ -91,12 +97,17 @@ public void run() { | |||
| sizeOfBuffer)); | |||
| } | |||
|
|
|||
| telemetry.incrBytesSent(sizeOfBuffer); | |||
There was a problem hiding this comment.
why are we not using sentBytes instead of sizeOfBuffer?
There was a problem hiding this comment.
Both are the smae if we're at that line since we check if (sizeOfBuffer != sentBytes) before.
| || clientError.telemetry.packetsDropped.get() == 0 | ||
| || clientError.telemetry.bytesDropped.get() == 0) { | ||
| try { | ||
| Thread.sleep(50L); |
There was a problem hiding this comment.
Am I correct to assume that you're doing this because you enabled the telemetry to flush in 50ms intervals here: https://github.com/DataDog/java-dogstatsd-client/pull/97/files#diff-bedc79efc014c08d01856bdf92f57801R288 ? If so, we're making the assumption the tests will run in order, which may be a safe assumption but is something I would prefer we did not assume in any case. If every test can be self-contained, that would be best.
There was a problem hiding this comment.
No we're actually waiting for the whole process to flush metrics. This is the same than when we wait for messages in our DummyServer https://github.com/DataDog/java-dogstatsd-client/blob/master/src/test/java/com/timgroup/statsd/DummyStatsDServer.java#L61.
Tests are unrelated from one another (one use client and the other clientError also).
hush-hush
force-pushed
the
maxime/telemetry
branch
3 times, most recently
from
128f386
to
8233c1a
Compare
hush-hush
force-pushed
the
maxime/telemetry
branch
from
8233c1a
to
b8afcdc
Compare
|
|
||
| private void sendMetric(final String message) { | ||
| send(message); | ||
| this.telemetry.incrMetricsSent(1); |
There was a problem hiding this comment.
Should this be done only if telemetry is enabled? It could maybe be implemented on the telemetry end, as a NOP if it's not enabled. Same would apply to all increment operations scattered all over the code.
| protected AtomicInteger packetsDropped; | ||
| protected AtomicInteger packetsDroppedQueue; | ||
|
|
||
| protected String metricsSentMetric; |
There was a problem hiding this comment.
This is going to conflict a little bit with the implementation in #105, no worries we can merge this first and I can fix it in that PR. These should be final too, I can also fix that in a follow-up.
| @@ -0,0 +1 @@ | |||
| dogstatsd_client_version=${project.version} | |||
Adding telemetry to the client about packet sent and dropped. By default we flush the telemetry every 5s.