Dedupe process in grpc reporter #1181

pavolloffay · 2018-11-14T14:21:44Z

Related to #773 (comment) and #1165 (comment)

Signed-off-by: Pavol Loffay ploffay@redhat.com

pavolloffay · 2018-11-14T14:22:56Z

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

+	return r.send(trace.Spans, process)
+}
+
+func (r *Reporter) send(spans []*model.Span, process model.Process) error {


Note that jaeger spans will have process nil whereas zipkin spans will have the process and process passed here is from the first span.

Maybe we should pass process=nil for zipkin path? I don't recall how collector handles batches, it should not apply batch-level Process to each span if spans already have a Process attached. This would be more compatible with, say, Istio use case where one client can emit spans for different processes.

Once we do that, we can kill this zipkin code path in the processor API and pull it higher in the chain, i.e. since you're already calling toDomain in the grpc package (but not in tchannel package), if we confirm that we don't cause a data loss we can call toDomain before invoking the processor.

NB: we may still need to keep the zipkin tchannel endpoint in the collector because some very old clients may be using direct tchannel connections to the collector, although I find it hard to believe since the agent was implemented long before we open sourced Jaeger backend (like 1yr before), so nobody outside of Uber should be using those old clients. If so, we can remove that endpoint and re-keep it in our internal build only.

I don't recall how collector handles batches

// PostSpans implements gRPC CollectorService. func (g *GRPCHandler) PostSpans(ctx context.Context, r *api_v2.PostSpansRequest) (*api_v2.PostSpansResponse, error) { for _, span := range r.GetBatch().Spans { if span.GetProcess() == nil { span.Process = &r.Batch.Process } } _, err := g.spanProcessor.ProcessSpans(r.GetBatch().Spans, JaegerFormatType) if err != nil { g.logger.Error("cannot process spans", zap.Error(err)) return nil, err } return &api_v2.PostSpansResponse{}, nil }

It sets the process only when it is missing.

Another issue is that this zipkin path skips zipkin sanitizers which are used in collector handler.

Once we do that, we can kill this zipkin code path in the processor API and pull it higher in the chain, i.e. since you're already calling toDomain in the grpc package (but not in tchannel package), if we confirm that we don't cause a data loss we can call toDomain before invoking the processor.

I don't get you here. Once we do what pass the nill process? I have just added sanitizers directly to the agent.

so nobody outside of Uber should be using those old clients. If so, we can remove that endpoint and re-keep it in our internal build only.

tracked in #1186 for

Right now we have this flow:

udp server jaeger.thrift -> reporter::SubmitBatch udp server zipkin.thrift -> reporter::SubmitZipkinSpans tchannel reporter::SubmitBatch -> pass through to collector tchannel reporter::SubmitZipkinSpans -> pass through to collector grpc reporter::SubmitBatch -> convert to domain and send to collector grpc reporter::SubmitZipkinSpans -> convert to jaeger.thrift, then to domain and send to collector

What I am proposing (could be done in a different PR), is to eliminate reporter::SubmitZipkinSpans completely and have this:

udp server jaeger.thrift -> reporter::SubmitBatch tchannel reporter::SubmitBatch -> pass through to collector grpc reporter::SubmitBatch -> convert to domain and send to collector udp server zipking.thrift -> convert to jaeger.thrift -> reporter::SubmitBatch

This way the Reporter only has a single method for Jaeger spans (in Thrift, for now).

Once we remove tchannel completely, we can push thrift->domain conversion into UDP servers as well, and Reporter interface effectively becomes == Collector service from proto IDL.

I see, it sounds reasonable to me. I would prefer a separate PR to do it as it might be bigger than I think right now.

I have created to track it separately #1193

codecov · 2018-11-14T14:30:45Z

Codecov Report

Merging #1181 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1181   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         157     157           
  Lines        7107    7104    -3     
======================================
- Hits         7107    7104    -3

Impacted Files	Coverage Δ
...d/collector/app/sanitizer/zipkin/span_sanitizer.go	`100% <ø> (ø)`	⬆️
cmd/collector/app/builder/span_handler_builder.go	`100% <100%> (ø)`	⬆️
model/converter/thrift/jaeger/to_domain.go	`100% <100%> (ø)`	⬆️
cmd/collector/app/grpc_handler.go	`100% <100%> (ø)`	⬆️
cmd/agent/app/reporter/grpc/reporter.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 c7da0b4...90326eb. Read the comment docs.

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

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2018-11-16T11:18:32Z

I think I have addressed all comments. I have created #1193 to remove EmitBatch. If there are no other comments I think it could go in and also the metrics PR #1180. The #1193 will handle removal of duplicated metrics e.g.

jaeger_agent_tchannel_reporter_batches_submitted{format="jaeger"} 0
jaeger_agent_tchannel_reporter_batches_submitted{format="zipkin"} 0

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro · 2018-11-16T18:42:22Z

cmd/collector/app/sanitizer/zipkin/span_sanitizer.go

@@ -28,6 +28,8 @@ const (

 var (
 	defaultDuration = int64(1)
+	// StandardSanitizers is a list of standard zipkin sanitizers.
+	StandardSanitizers = []Sanitizer{NewSpanStartTimeSanitizer(), NewSpanDurationSanitizer(), NewParentIDSanitizer(), NewErrorTagSanitizer()}


nit: one per line

yurishkuro · 2018-11-16T18:43:46Z

model/proto/model.proto

@@ -155,6 +155,6 @@ message Trace {
 message Batch {
    repeated Span spans = 1;
    Process process = 2 [
-      (gogoproto.nullable) = false
+      (gogoproto.nullable) = true


nit: I would remove the option altogether (true is the default afaik)

pavolloffay requested review from black-adder, jpkrohling, objectiser, tiffon, vprithvi and yurishkuro as code owners November 14, 2018 14:21

ghost assigned pavolloffay Nov 14, 2018

ghost added the review label Nov 14, 2018

pavolloffay commented Nov 14, 2018

View reviewed changes

pavolloffay mentioned this pull request Nov 14, 2018

Use Protobuf and gRPC for internal communications #773

Open

24 tasks

yurishkuro reviewed Nov 14, 2018

View reviewed changes

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

pavolloffay mentioned this pull request Nov 15, 2018

Remove zipkin thrift endpoint from agent #1186

Open

yurishkuro reviewed Nov 16, 2018

View reviewed changes

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

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

yurishkuro mentioned this pull request Nov 16, 2018

Add gRPC metrics to agent #1180

Merged

pavolloffay added 5 commits November 16, 2018 12:11

Dedupe process in grpc reporter

709333f

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix review comments

fc5e803

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Pass nill process and sanitize zipkin spans in grpc reporter

857f55a

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Standard sanitizers

fce1a95

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Make process optional in proto model

f981d30

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the dedupe_process_reporter branch from 6627b51 to f981d30 Compare November 16, 2018 11:11

pavolloffay mentioned this pull request Nov 16, 2018

Remove EmitZipkinBatch([]*zipkincore.Span) from agent reporter #1193

Open

Increase number of requests

90326eb

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro approved these changes Nov 16, 2018

View reviewed changes

yurishkuro merged commit 56ec296 into jaegertracing:master Nov 16, 2018

ghost removed the review label Nov 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dedupe process in grpc reporter #1181

Dedupe process in grpc reporter #1181

pavolloffay commented Nov 14, 2018

pavolloffay Nov 14, 2018

yurishkuro Nov 14, 2018

pavolloffay Nov 15, 2018

pavolloffay Nov 15, 2018 •

edited

Loading

yurishkuro Nov 16, 2018 •

edited

Loading

pavolloffay Nov 16, 2018

pavolloffay Nov 16, 2018

codecov bot commented Nov 14, 2018 •

edited

Loading

pavolloffay commented Nov 16, 2018

yurishkuro Nov 16, 2018

yurishkuro Nov 16, 2018

Dedupe process in grpc reporter #1181

Dedupe process in grpc reporter #1181

Conversation

pavolloffay commented Nov 14, 2018

pavolloffay Nov 14, 2018

Choose a reason for hiding this comment

yurishkuro Nov 14, 2018

Choose a reason for hiding this comment

pavolloffay Nov 15, 2018

Choose a reason for hiding this comment

pavolloffay Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

yurishkuro Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

pavolloffay Nov 16, 2018

Choose a reason for hiding this comment

pavolloffay Nov 16, 2018

Choose a reason for hiding this comment

codecov bot commented Nov 14, 2018 • edited Loading

Codecov Report

pavolloffay commented Nov 16, 2018

yurishkuro Nov 16, 2018

Choose a reason for hiding this comment

yurishkuro Nov 16, 2018

Choose a reason for hiding this comment

pavolloffay Nov 15, 2018 •

edited

Loading

yurishkuro Nov 16, 2018 •

edited

Loading

codecov bot commented Nov 14, 2018 •

edited

Loading