Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Split zipkin span which contains server and client annotations #453

Merged
merged 4 commits into from
Oct 26, 2017
Merged

Split zipkin span which contains server and client annotations #453

merged 4 commits into from
Oct 26, 2017

Conversation

pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Oct 4, 2017 •
edited
Loading

@ghost ghost assigned pavolloffay Oct 4, 2017
@ghost ghost added the review label Oct 4, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7ad3561 on pavolloffay:zipkin-shared-spans-span-kind into 5030513 on jaegertracing:master.

@pavolloffay pavolloffay force-pushed the zipkin-shared-spans-span-kind branch 2 times, most recently from 55beced to c247c50 Compare October 19, 2017 13:54
@pavolloffay pavolloffay changed the title Zipkin toDomain allow multiple span.kind in one span Split zipkin span which contains server and client annotations Oct 19, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4e99d30 on pavolloffay:zipkin-shared-spans-span-kind into 897831a on jaegertracing:master.

pavolloffay
pavolloffay commented Oct 19, 2017
View reviewed changes
model/converter/thrift/zipkin/to_domain.go Outdated
@@ -309,7 +356,10 @@ func (td toDomain) getLogFields(annotation *zipkincore.Annotation) []model.KeyVa

func (td toDomain) getSpanKindTag(annotations []*zipkincore.Annotation) (model.KeyValue, bool) {
for _, a := range annotations {
if spanKind, ok := coreAnnotations[a.Value]; ok {
if spanKind, ok := coreClientAnnotations[a.Value]; ok {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have split core annotations to be sure that client kind is always created first. We could keep annotations in one map and then in transformSpan check which one has been transformed and create missing one based on that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be sure that client kind is always created first

why does it matter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then in transformSpan I know that only server span kind has to be added.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5c46743 on pavolloffay:zipkin-shared-spans-span-kind into 897831a on jaegertracing:master.

@pavolloffay pavolloffay force-pushed the zipkin-shared-spans-span-kind branch from b124050 to c53f31b Compare October 25, 2017 11:47
@pavolloffay
Split zipkin span
9da8b4b 
Split zipkin span if it contains client and server annotations

Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay pavolloffay force-pushed the zipkin-shared-spans-span-kind branch from c53f31b to 9da8b4b Compare October 25, 2017 11:48
@pavolloffay
revert unnecessary change
f11ec41 
Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay
Copy link
Member Author

@yurishkuro it should be ready for review

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f11ec41 on pavolloffay:zipkin-shared-spans-span-kind into a7f203b on jaegertracing:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f11ec41 on pavolloffay:zipkin-shared-spans-span-kind into a7f203b on jaegertracing:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f11ec41 on pavolloffay:zipkin-shared-spans-span-kind into a7f203b on jaegertracing:master.

yurishkuro
yurishkuro reviewed Oct 25, 2017
View reviewed changes
cmd/collector/app/span_handler.go Outdated
@@ -129,7 +129,7 @@ func (h *zipkinSpanHandler) SubmitZipkinBatch(ctx thrift.Context, spans []*zipki
}

// ConvertZipkinToModel is a helper function that logs warnings during conversion
func ConvertZipkinToModel(zSpan *zipkincore.Span, logger *zap.Logger) *model.Span {
func convertZipkinToModel(zSpan *zipkincore.Span, logger *zap.Logger) []*model.Span {
mSpan, err := zipkin.ToDomainSpan(zSpan)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/mSpan/mSpans/

model/converter/thrift/zipkin/to_domain.go Outdated
var csOk bool
var srOk bool
for _, ann := range zSpan.Annotations {
if ann.Value == "cs" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use constants from zipkincore namespace instead of strings

model/converter/thrift/zipkin/to_domain.go Outdated
}
return trace, multierror.Wrap(errors)
}

func (td toDomain) ToDomainSpan(zSpan *zipkincore.Span) (*model.Span, error) {
jSpan := td.transformSpan(zSpan)
func (td toDomain) ToDomainSpan(zSpan *zipkincore.Span) ([]*model.Span, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make the naming consistent across the board: ToDomainSpans (plural)

model/converter/thrift/zipkin/to_domain.go
TraceID: model.TraceID{High: uint64(traceIDHigh), Low: uint64(zSpan.TraceID)},
SpanID: model.SpanID(zSpan.ID),
OperationName: zSpan.Name,
ParentSpanID: model.SpanID(parentID),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing Flags, StartTime, Duration?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added with similar impl to zipkin.

model/converter/thrift/zipkin/to_domain.go Outdated
}
// if the first span is a client span we create server span and vice-versa.
if spanKindTag.VStr == string(ext.SpanKindRPCClient.Value.(ext.SpanKindEnum)) {
s.Tags = []model.KeyValue{model.String(ext.SpanKindRPCServer.Key, string(ext.SpanKindRPCServer.Value.(ext.SpanKindEnum)))}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why cast the value ext.SpanKindRPCServer.Value.(ext.SpanKindEnum)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, why is the first argument SpanKindRPCServer instead of just SpanKind?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why cast the value ext.SpanKindRPCServer.Value.(ext.SpanKindEnum)?

because it fails on cannot convert ext.SpanKindRPCClient.Value (type interface {}) to type string

model/converter/thrift/zipkin/to_domain.go Outdated
tags := td.getTags(zSpan.BinaryAnnotations, td.isSpanTag)
if spanKindTag, ok := td.getSpanKindTag(zSpan.Annotations); ok {
var spanKindTag model.KeyValue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to capture this tag, the model span has helper methods like span.IsRPCClient()

@pavolloffay
fix review comments
24db65d 
Signed-off-by: Pavol Loffay <[email protected]>
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 99.938% when pulling 24db65d on pavolloffay:zipkin-shared-spans-span-kind into a7f203b on jaegertracing:master.

yurishkuro
yurishkuro reviewed Oct 26, 2017
View reviewed changes
model/converter/thrift/zipkin/to_domain.go Outdated
}
// if the first span is a client span we create server span and vice-versa.
if result[0].IsRPCClient() {
s.Tags = []model.KeyValue{model.String(ext.SpanKindRPCServer.Key, string(ext.SpanKindRPCServer.Value.(ext.SpanKindEnum)))}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a lot simpler: model.String(string(ext.SpanKind), string(ext.SpanKindRPCServerEnum))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 changed

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c158563 on pavolloffay:zipkin-shared-spans-span-kind into a7f203b on jaegertracing:master.

@yurishkuro yurishkuro merged commit 7f28c77 into jaegertracing:master Oct 26, 2017
@ghost ghost removed the review label Oct 26, 2017
isaachier pushed a commit to isaachier/jaeger that referenced this pull request Nov 1, 2017
@pavolloffay
Split zipkin span which contains server and client annotations (jaege…
dc2e1d2 
…rtracing#453)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@black-adder black-adder Awaiting requested review from black-adder

@vprithvi vprithvi Awaiting requested review from vprithvi

Assignees

@pavolloffay pavolloffay

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pavolloffay @coveralls @yurishkuro