-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor handlers to separate transport and span format concerns #1458
Conversation
"github.com/jaegertracing/jaeger/thrift-gen/zipkincore" | ||
) | ||
|
||
type tchannelHandler struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: what are the implications of having the InboundTransport
not being specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't skip, but assign an "undefined" value so that the total counter is still correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we create constants if they don't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jude is doing it in the next PR. Right now these strings are not used anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have the followup story to address that.
implements #1446 (comment)