Report data loss stats to Jaeger backend #482
Conversation
Codecov Report
@@ Coverage Diff @@
## master #482 +/- ##
==========================================
+ Coverage 88.29% 88.43% +0.14%
==========================================
Files 59 59
Lines 3553 3581 +28
==========================================
+ Hits 3137 3167 +30
+ Misses 304 303 -1
+ Partials 112 111 -1
Continue to review full report at Codecov.
|
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
yurishkuro
force-pushed
the
client-telemetry
branch
from
8500a28
to
584e1c6
Compare
Signed-off-by: Yuri Shkuro <ys@uber.com>
reporter.go
Outdated
| @@ -214,10 +216,18 @@ func NewRemoteReporter(sender Transport, opts ...ReporterOption) Reporter { | |||
| sender: sender, | |||
| queue: make(chan reporterQueueItem, options.queueSize), | |||
| } | |||
| if receiver, ok := sender.(reporterstats.Receiver); ok { | |||
| receiver.SetReporterStats(reporter) | |||
There was a problem hiding this comment.
It's a little bit confusing to have remoteReporter implemented ReporterStats interface and use reporter as ReporterStats in the setter.
I think it might be better to have specific ReporterStats implementation and use it in the reporter, other reporter can reuse the stats too. Both queueLength and droppedCount can go to the Stats interface.
There was a problem hiding this comment.
I am not sure what you're suggesting. Reporter is a provider of ReporterStats, which is why it passes itself to the receiver. queueLength is not a data loss metric and does not need to be made available to the transports.
There was a problem hiding this comment.
I think it's clearer if we have a specific provider of ReporterStats and pass it to reporter. Here reporter does too much stuff.
queueLength is not a data loss metric, but seems a reasonable reporter related metric and ReporterStats does not directly mean data loss stats.
There was a problem hiding this comment.
ok, I pulled reporterStats into a composable helper struct. But I don't want to change queueLength handling, it's not in scope of this change, and grouping it with ReporterStats provides no benefits.
Signed-off-by: Yuri Shkuro <ys@uber.com>
| // The following counters are always non-negative, but we need to send them in signed i64 Thrift fields, | ||
| // so we keep them as signed. At 10k QPS, overflow happens in about 300 million years. |
There was a problem hiding this comment.
nice, let's hope Jaeger will still be around after 300 million years! :)
There was a problem hiding this comment.
and running without a restart
Which problem is this PR solving?
Short description of the changes