Refactor handlers to separate transport and span format concerns #1458
Conversation
yurishkuro
requested review from
black-adder,
jpkrohling,
objectiser,
pavolloffay,
tiffon and
vprithvi
as code owners
yurishkuro
changed the title
| "github.com/jaegertracing/jaeger/thrift-gen/zipkincore" | ||
| ) | ||
|
|
||
| type tchannelHandler struct { |
There was a problem hiding this comment.
Maybe we can add an interface for tchannel handler, that way it's more clear this handler is satisfying the thrift definition?
type tchanHandler struct {
zipkinSpansHandler ZipkinSpansHandler
jaegerSpansHandler JaegerBatchesHandler
}
There was a problem hiding this comment.
it already implements existing interfaces, no benefit of defining more. I added comments and the API checks to the test file,
Codecov Report
@@ Coverage Diff @@
## master #1458 +/- ##
==========================================
+ Coverage 99.82% 99.82% +<.01%
==========================================
Files 172 173 +1
Lines 8146 8162 +16
==========================================
+ Hits 8132 8148 +16
Misses 7 7
Partials 7 7
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>
yurishkuro
force-pushed
the
refactor-handlers
branch
from
1c2fecb
to
60cbb71
Compare
|
Actually can we put handlers into separate folders (e.g transport/representation/processor)? The current flat |
|
We could, but the packages will be very small, one file, it's not the recommended practice in Go to have so small packages. |
| @@ -26,6 +26,18 @@ import ( | |||
| "github.com/jaegertracing/jaeger/storage/spanstore" | |||
| ) | |||
|
|
|||
| // ProcessSpansOptions additional options passed to processor along with the spans. | |||
| type ProcessSpansOptions struct { | |||
There was a problem hiding this comment.
What would happen if neither SpanFormat nor InboundTransport are set?
| @@ -35,22 +34,21 @@ const ( | |||
| UnknownFormatType = "unknown" | |||
| ) | |||
|
|
|||
| // SubmitBatchOptions are passed to Submit methods of the handlers. | |||
| type SubmitBatchOptions struct { | |||
| InboundTransport string | |||
There was a problem hiding this comment.
Same as above: what are the implications of having the InboundTransport not being specified?
There was a problem hiding this comment.
I have a followup story to take care of that. I imagine if InBoundTransport is not set, we would simply skip setting another dimension for span counter metrics.
There was a problem hiding this comment.
We shouldn't skip, but assign an "undefined" value so that the total counter is still correct.
There was a problem hiding this comment.
We shouldn't skip, but assign an "undefined" value so that the total counter is still correct.
hmmm. What I meant was something like the following:
tags := map[string]string{"svc": serviceName, "debug": debugStr}
if endpointType != "" {
tags["transport"] = endpointType
}
c := m.factory.Counter(metrics.Options{Name: m.category, Tags: tags})
so if endpointType is empty we still count it just not having a transport tag. Is adding undefined tag mandatory here for metrics to work correctly?
There was a problem hiding this comment.
Well, first, we don't create counter objects with labels in real time, because it's expensive and we always prefer to pre-create them when possible and cache. Second, when creating a counter we must define a set of labels (tags) it has. Some metrics backends may not like the value of the label to be blank. Why not be explicit about the value then?
tags := map[string]string{
"svc": serviceName,
"debug": debugStr,
"transport": "undefined", // default
}
if endpointType != "" {
tags["transport"] = endpointType
}
| @@ -43,7 +43,10 @@ func (g *GRPCHandler) PostSpans(ctx context.Context, r *api_v2.PostSpansRequest) | |||
| span.Process = r.Batch.Process | |||
| } | |||
| } | |||
| _, err := g.spanProcessor.ProcessSpans(r.GetBatch().Spans, JaegerFormatType) | |||
| _, err := g.spanProcessor.ProcessSpans(r.GetBatch().Spans, ProcessSpansOptions{ | |||
| InboundTransport: "grpc", // TODO do we have a constant? | |||
There was a problem hiding this comment.
Could we create constants if they don't exist?
There was a problem hiding this comment.
Jude is doing it in the next PR. Right now these strings are not used anywhere.
There was a problem hiding this comment.
Yes, I have the followup story to address that.
implements #1446 (comment)