[agent] Process data loss stats from clients #2010

yurishkuro · 2020-01-09T00:06:25Z

Which problem is this PR solving?

Reporting client data loss stats to Jaeger backend #2005

Short description of the changes

Keeps track of stats reported by clients in jaegerThrift.Batch, per client-uuid
Emits corresponding metrics
Cleans-up the per-client-uuid cache periodically

This is how new metrics look:

$ curl -s http://localhost:14269/metrics | grep client_stats
# HELP jaeger_agent_client_stats_batches_sent_total Total count of batches sent by clients
# TYPE jaeger_agent_client_stats_batches_sent_total counter
jaeger_agent_client_stats_batches_sent_total 4
# HELP jaeger_agent_client_stats_connected_clients Total count of unique clients sending data to the agent
# TYPE jaeger_agent_client_stats_connected_clients gauge
jaeger_agent_client_stats_connected_clients 1
# HELP jaeger_agent_client_stats_spans_dropped_total Total count of spans dropped by clients
# TYPE jaeger_agent_client_stats_spans_dropped_total counter
jaeger_agent_client_stats_spans_dropped_total{cause="full-queue"} 0
jaeger_agent_client_stats_spans_dropped_total{cause="send-failure"} 0
jaeger_agent_client_stats_spans_dropped_total{cause="too-large"} 0

TODO

Pin correct jaeger-client-go and jaeger-idl once their respective PRs are merged
Add unit tests for new functionality

jpkrohling

It could use a bit more logging to my taste, but looks like a promising first draft.

Makefile

cmd/agent/app/reporter/client_metrics.go

codecov · 2020-01-10T07:02:42Z

Codecov Report

Merging #2010 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2010      +/-   ##
==========================================
+ Coverage   97.38%   97.41%   +0.02%     
==========================================
  Files         206      207       +1     
  Lines       10136    10243     +107     
==========================================
+ Hits         9871     9978     +107     
  Misses        222      222              
  Partials       43       43

Impacted Files	Coverage Δ
cmd/agent/app/processors/thrift_processor.go	`100% <ø> (ø)`	⬆️
cmd/agent/app/reporter/client_metrics.go	`100% <100%> (ø)`
cmd/agent/app/reporter/tchannel/collector_proxy.go	`100% <100%> (ø)`	⬆️
cmd/agent/app/reporter/metrics.go	`100% <100%> (ø)`	⬆️
cmd/agent/app/reporter/grpc/collector_proxy.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 641dd92...4d2f581. Read the comment docs.

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro · 2020-01-14T05:18:00Z

this is ready for review

jpkrohling

Looking good so far. This PR assumes that clients will only talk to a single agent for the whole lifecycle of the client, which is a reasonable assumption. Once there's a PR ready to be tested on the client side, I'll run an end-to-end manual test.

cmd/agent/app/reporter/client_metrics_test.go

cmd/agent/app/reporter/client_metrics.go

cmd/agent/app/reporter/client_metrics_test.go

cmd/agent/app/reporter/metrics_test.go

yurishkuro · 2020-01-14T16:18:21Z

Once there's a PR ready to be tested on the client side, I'll run an end-to-end manual test.

@jpkrohling the working client version is already included here as dependency (jaegertracing/jaeger-client-go#482), and the functionality can be tested with all-in-one (since query service is self-tracing).

Signed-off-by: Yuri Shkuro <ys@uber.com>

jpkrohling

Let's just figure out the debug-logging discussion and add the expected gauges/counters to a test, and it's ready to be merged.

Signed-off-by: Yuri Shkuro <ys@uber.com>

guo0693 · 2020-01-15T18:46:46Z

cmd/agent/app/reporter/tchannel/collector_proxy.go

@@ -25,22 +25,28 @@ import (

 // ProxyBuilder holds objects communicating with collector
 type ProxyBuilder struct {
-	reporter reporter.Reporter
+	reporter *reporter.ClientMetricsReporter
 	manager  configmanager.ClientConfigManager
 	tchanRep *Reporter
 }

 // NewCollectorProxy creates ProxyBuilder
 func NewCollectorProxy(builder *Builder, mFactory metrics.Factory, logger *zap.Logger) (*ProxyBuilder, error) {


a little bit irrelevant to data loss, we don't need to change it in this PR. But usually Builder.NewXXX means build a new XXX, seems more reasonable for NewCollectorProxy to create a CollectorProxy instead of ProxyBuilder.

I agree, the naming here has become weird overtime.

guo0693 · 2020-01-15T18:48:44Z

thrift-gen/jaeger/tchan-jaeger.go

@@ -43,10 +43,6 @@ func (c *tchanCollectorClient) SubmitBatches(ctx thrift.Context, batches []*Batc
 	}
 	success, err := c.client.Call(ctx, c.thriftService, "submitBatches", &args, &resp)
 	if err == nil && !success {
-		switch {


Seems generated deletion looks weird. Why no logging?

I can't help it, it's how the recent generator works. But the new code looks correct - propagating the error up, rather than introducing that odd override.

Also, low-level libraries are not supposed to log.

guo0693 · 2020-01-15T19:04:45Z

cmd/agent/app/reporter/tchannel/collector_proxy.go

-		tchanRep: r,
-		reporter: reporter.WrapWithMetrics(r, tchannelMetrics),
-		manager:  configmanager.WrapWithMetrics(tchannel.NewConfigManager(r.CollectorServiceName(), r.Channel()), tchannelMetrics),
+		tchanRep: tchanRep,


Why do we need to export tchanRep? Is tchanRep used any where? If not, we could remove tchanRep from below struct:
type ProxyBuilder struct { reporter reporter.Reporter manager configmanager.ClientConfigManager tchanRep *Reporter }

Yes, it's unfortunate side effect of the way constructors are defined. I think the Sampling server depends on the underlying tchannel that is being created here.

guo0693 · 2020-01-15T19:15:10Z

cmd/agent/app/reporter/grpc/collector_proxy.go

@@ -39,9 +39,16 @@ func NewCollectorProxy(builder *ConnBuilder, agentTags map[string]string, mFacto
 		return nil, err
 	}
 	grpcMetrics := mFactory.Namespace(metrics.NSOptions{Name: "", Tags: map[string]string{"protocol": "grpc"}})
+	r1 := NewReporter(conn, agentTags, logger)
+	r2 := reporter.WrapWithMetrics(r1, grpcMetrics)
+	r3 := reporter.WrapWithClientMetrics(reporter.ClientMetricsReporterParams{


Is it possible to have a generic WrappedReporter? Essentially r2 & r3 are wrapped reporter with callbacks around EmitBatch and EmitZipkinBatch. It will be great if we can have chained wrapper, something like:
NewReporter(...).wrapWith().wrapWith().

in what sense "generic"?

I haven't thought it through, not sure if it's totally possible or not. Some rough idea as below:

type WrappedReporter interface { EmitZipkinBatch(spans []*zipkincore.Span) (err error) EmitBatch(batch *jaeger.Batch) (err error) WrapWith(func onEmitZipkinBatch, func onEmitBatch) (*WrappedReporter, error) }

We may need to solve the problem whether the callback is called before or after EmitBatch, seems like metrics.go & client_metrics.go do those operations in different order, one before while the other one after.

what benefits would this provide? The two existing decorators have specific things they are doing before or after calling the underlying, there's little room to generalize there. And WrapWith as part of the interface smells like an anti-pattern due to unnecessary coupling, i.e. each of the two decorators does not need to know if it itself is being decorated further.

As I read the code, I felt confused when I first saw metrics.go and client_metrics.go, after spent some time reading, I understood they are reporters wrapped with different call backs.

Regarding detailed implementation of the callbacks, yes they will be very different and no need to be generalized. The generalization is for better readability and extendability I guess, it will be easy to wrap another callback around emit calls in the future instead of implement a new reader if necessary .

I agree, the decorators don't need to know if they will be decorated further or not. I just got the idea from chained promise from javascript, it will be good if we can support similar feature here. This way, you don't need:

r1 := NewReporter(conn, agentTags, logger) r2 := reporter.WrapWithMetrics(r1, grpcMetrics) r3 := reporter.WrapWithClientMetrics(...)

You just need:

NewReporter(...).wrapWith(//r2 functionalities).wrapWith(//r3 functinoalities)

guo0693

LGTM overall, it just take me sometime to understand the difference between metrics.go (reporter wrapped with metrics) and client_metrics.go (reporter wrapped with client metrics). It will be great if we can generalize the wrapped reporter, if it's not feasible, we can merge the PR as of now.

yurishkuro · 2020-01-15T20:10:03Z

I think I addressed @jpkrohling comments, so merging.

yurishkuro requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon and vprithvi as code owners January 9, 2020 00:06

yurishkuro mentioned this pull request Jan 9, 2020

Reporting client data loss stats to Jaeger backend #2005

Closed

4 tasks

yurishkuro changed the title ~~[agent] Process data loss stats from clients~~ WIP - [agent] Process data loss stats from clients Jan 9, 2020

jpkrohling reviewed Jan 9, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

cmd/agent/app/reporter/client_metrics.go Show resolved Hide resolved

cmd/agent/app/reporter/client_metrics.go Outdated Show resolved Hide resolved

cmd/agent/app/reporter/client_metrics.go Outdated Show resolved Hide resolved

yurishkuro force-pushed the data-loss-reporting branch from 2cbd9e3 to 756bc71 Compare January 9, 2020 18:33

Yuri Shkuro added 17 commits January 13, 2020 14:55

Collect data loss metrics from clients

a4624ef

Signed-off-by: Yuri Shkuro <ys@uber.com>

Regenerate thrift

ec7badc

Signed-off-by: Yuri Shkuro <ys@uber.com>

Prototype

c7b7db7

Signed-off-by: Yuri Shkuro <ys@uber.com>

Upgrade Jaeger client

0b02d2b

Signed-off-by: Yuri Shkuro <ys@uber.com>

Handle wrapped counters

28bf127

Signed-off-by: Yuri Shkuro <ys@uber.com>

Fix formatting

a4aa423

Signed-off-by: Yuri Shkuro <ys@uber.com>

Move to new file

f3e6781

Signed-off-by: Yuri Shkuro <ys@uber.com>

Shutdown

95a73dc

Signed-off-by: Yuri Shkuro <ys@uber.com>

Undo unnecessary change

af30389

Signed-off-by: Yuri Shkuro <ys@uber.com>

regenerate

112c7d0

Signed-off-by: Yuri Shkuro <ys@uber.com>

Add logging

583616e

Signed-off-by: Yuri Shkuro <ys@uber.com>

cleanup

6823d63

Signed-off-by: Yuri Shkuro <ys@uber.com>

delint

f4f8c4c

Signed-off-by: Yuri Shkuro <ys@uber.com>

Add some tests

07fddf7

Signed-off-by: Yuri Shkuro <ys@uber.com>

Clean up

846d9a0

Signed-off-by: Yuri Shkuro <ys@uber.com>

more tests

b04c9be

Signed-off-by: Yuri Shkuro <ys@uber.com>

More tests

fae5f77

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro force-pushed the data-loss-reporting branch from ef1bc48 to fae5f77 Compare January 13, 2020 23:17

Reduce nesting

7a9376f

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro mentioned this pull request Jan 14, 2020

Report data loss stats to Jaeger backend jaegertracing/jaeger-client-go#482

Merged

yurishkuro changed the title ~~WIP - [agent] Process data loss stats from clients~~ [agent] Process data loss stats from clients Jan 14, 2020

jpkrohling reviewed Jan 14, 2020

View reviewed changes

ignore overflow

81f5f41

Signed-off-by: Yuri Shkuro <ys@uber.com>

jpkrohling reviewed Jan 15, 2020

View reviewed changes

Yuri Shkuro added 4 commits January 15, 2020 11:32

deps

af699ff

Signed-off-by: Yuri Shkuro <ys@uber.com>

test

68722aa

Signed-off-by: Yuri Shkuro <ys@uber.com>

rename

6a0392b

Signed-off-by: Yuri Shkuro <ys@uber.com>

comments

4d2f581

Signed-off-by: Yuri Shkuro <ys@uber.com>

guo0693 reviewed Jan 15, 2020

View reviewed changes

guo0693 approved these changes Jan 15, 2020

View reviewed changes

yurishkuro merged commit 43be2e7 into jaegertracing:master Jan 15, 2020

yurishkuro deleted the data-loss-reporting branch January 15, 2020 20:10

This was referenced Jan 17, 2020

[agent] Do not increment data loss counters on the first client batch #2028

Merged

Count received batches from conforming clients #2030

Merged

pavolloffay mentioned this pull request Mar 2, 2020

Fix flaky test TestClientMetricsReporter_Expire #2108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[agent] Process data loss stats from clients #2010

[agent] Process data loss stats from clients #2010

yurishkuro commented Jan 9, 2020 •

edited

Loading

jpkrohling left a comment

codecov bot commented Jan 10, 2020 •

edited

Loading

yurishkuro commented Jan 14, 2020

jpkrohling left a comment

yurishkuro commented Jan 14, 2020

jpkrohling left a comment

guo0693 Jan 15, 2020 •

edited

Loading

yurishkuro Jan 15, 2020

guo0693 Jan 15, 2020

yurishkuro Jan 15, 2020

guo0693 Jan 15, 2020

yurishkuro Jan 15, 2020

guo0693 Jan 15, 2020

yurishkuro Jan 15, 2020

guo0693 Jan 15, 2020 •

edited

Loading

yurishkuro Jan 15, 2020

guo0693 Jan 15, 2020

guo0693 left a comment

yurishkuro commented Jan 15, 2020

[agent] Process data loss stats from clients #2010

[agent] Process data loss stats from clients #2010

Conversation

yurishkuro commented Jan 9, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

TODO

jpkrohling left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2020 • edited Loading

Codecov Report

yurishkuro commented Jan 14, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

yurishkuro commented Jan 14, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

guo0693 Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guo0693 Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guo0693 left a comment

Choose a reason for hiding this comment

yurishkuro commented Jan 15, 2020

yurishkuro commented Jan 9, 2020 •

edited

Loading

codecov bot commented Jan 10, 2020 •

edited

Loading

guo0693 Jan 15, 2020 •

edited

Loading

guo0693 Jan 15, 2020 •

edited

Loading