Skip to content

Unify zipkin v1 and v2 annotation/tag parsing logic #1002

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

Merged
merged 12 commits into from
Jun 23, 2020
Merged

Unify zipkin v1 and v2 annotation/tag parsing logic #1002

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2080cdc
Move zipkin v2 trace conversion code into translator/trace/zipkin
chris-smith-zocdoc Jun 4, 2020
45c7df8
fallback to error tag if set
chris-smith-zocdoc Jun 4, 2020
5acb4a0
return nil instead of unknown
chris-smith-zocdoc Jun 5, 2020
bbd9ad3
return nil ocStatus when only message is provided
chris-smith-zocdoc Jun 9, 2020
2cdac1b
zipkin v1: The time event should use the Description for the zipkin v…
chris-smith-zocdoc Jun 10, 2020
937621f
Merge branch 'upstream' into cs_975
chris-smith-zocdoc Jun 12, 2020
56a079d
Merge branch 'upstream' into cs_975
chris-smith-zocdoc Jun 12, 2020
945fe81
add missing test case
chris-smith-zocdoc Jun 12, 2020
0ea765f
increase test coverage on the zipkin receiver
chris-smith-zocdoc Jun 13, 2020
1470dca
add some more coverage
chris-smith-zocdoc Jun 15, 2020
8d07b93
minor coverage improvement for zipkin v1
chris-smith-zocdoc Jun 16, 2020
d08da47
Prioritize valid oc status code in error over http
chris-smith-zocdoc Jun 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions translator/trace/zipkin/testdata/zipkin_v1_multiple_batches.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@
"serviceName": "service1"
}
},
{
"timestamp": 1544805927450000,
"value": "custom time event",
"endpoint": {
"ipv4": "172.31.0.4",
"port": 0,
"serviceName": "service1"
}
},
{
"timestamp": 1544805927460102,
"value": "ss",
Expand Down
9 changes: 9 additions & 0 deletions translator/trace/zipkin/testdata/zipkin_v1_single_batch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@
}
},
{
"timestamp": 1544805927450000,
"value": "custom time event",
"endpoint": {
"ipv4": "172.31.0.4",
"port": 0,
"serviceName": "service1"
}
},
{
"timestamp": 1544805927460102,
"value": "ss",
"endpoint": {
Expand Down
9 changes: 9 additions & 0 deletions translator/trace/zipkin/testdata/zipkin_v1_thrift_single_batch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@
}
},
{
"timestamp": 1544805927450000,
"value": "custom time event",
"host": {
"ipv4": -1407254524,
"port": 0,
"service_name": "service1"
}
},
{
"timestamp": 1544805927460102,
"value": "ss",
"host": {
Expand Down
12 changes: 1 addition & 11 deletions translator/trace/zipkin/zipkinv1_to_protospan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,17 +359,7 @@ func parseZipkinV1Annotations(annotations []*annotation) *annotationParseResult
// Using the more expensive annotation until/if something cheaper is needed.
Value: &tracepb.Span_TimeEvent_Annotation_{
Annotation: &tracepb.Span_TimeEvent_Annotation{
Attributes: &tracepb.Span_Attributes{
AttributeMap: map[string]*tracepb.AttributeValue{
currAnnotation.Value: {
Value: &tracepb.AttributeValue_StringValue{
StringValue: &tracepb.TruncatableString{
Value: endpointName,
},
},
},
},
},
Description: &tracepb.TruncatableString{Value: currAnnotation.Value},
Copy link
Contributor Author

@chris-smith-zocdoc chris-smith-zocdoc Jun 10, 2020

Choose a reason for hiding this comment

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

@dmitryax found another difference with the v1 parsing on Time Events, converted it to the v2 behavior

opentelemetry-collector/receiver/zipkinreceiver/trace_receiver.go

Lines 562 to 574 in 8aa2731

func zipkinAnnotationToProtoAnnotation(zas zipkinmodel.Annotation) *tracepb.Span_TimeEvent {
if zas == blankAnnotation {
return nil
}
return &tracepb.Span_TimeEvent{
Time: internal.TimeToTimestamp(zas.Timestamp),
Value: &tracepb.Span_TimeEvent_Annotation_{
Annotation: &tracepb.Span_TimeEvent_Annotation{
Description: &tracepb.TruncatableString{Value: zas.Value},
},
},
}
}

without this change, the exporter would emit time events without a name

Copy link
Member

@dmitryax dmitryax Jun 10, 2020

Choose a reason for hiding this comment

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

Will it still work as expected for v1 spans?

Copy link
Contributor Author

@chris-smith-zocdoc chris-smith-zocdoc Jun 10, 2020

Choose a reason for hiding this comment

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

Previously it didn't work, it should now

The time event (annotation in zipkin terms) looks like this

annotations: 
[ {
    "timestamp": 1591814867,
    "value": "my custom annotation",
    "endpoint": {
       "serviceName": "my-service"
   }
} ]

Previously this was being converted into this proto structure

time_events: 
[ {
    time: 1591814867,
    attributes: {
       "my custom annotation": "my-service"
    }
} ]

Now it is

time_events: 
[ {
    time: 1591814867,
    description: "my custom annotation"
} ]

Copy link
Member

@dmitryax dmitryax Jun 11, 2020

Choose a reason for hiding this comment

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

Ok. looks good

},
},
}
Expand Down
21 changes: 14 additions & 7 deletions translator/trace/zipkin/zipkinv1_to_protospan_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,7 @@ func TestSingleJSONV1BatchToOCProto(t *testing.T) {
sortTraceByNodeName(want)
sortTraceByNodeName(got)

if !reflect.DeepEqual(got, want) {
t.Fatalf("Unsuccessful conversion\nGot:\n\t%v\nWant:\n\t%v", got, want)
}
assert.EqualValues(t, got, want)
}

func TestMultipleJSONV1BatchesToOCProto(t *testing.T) {
Expand Down Expand Up @@ -234,9 +232,7 @@ func TestMultipleJSONV1BatchesToOCProto(t *testing.T) {
sortTraceByNodeName(want)
sortTraceByNodeName(got)

if !reflect.DeepEqual(got, want) {
t.Fatalf("Unsuccessful conversion\nGot:\n\t%v\nWant:\n\t%v", got, want)
}
assert.EqualValues(t, got, want)
}

func sortTraceByNodeName(trace []consumerdata.TraceData) {
Expand Down Expand Up @@ -600,7 +596,18 @@ var ocBatchesFromZipkinV1 = []consumerdata.TraceData{
Kind: tracepb.Span_SERVER,
StartTime: &timestamp.Timestamp{Seconds: 1544805927, Nanos: 448081000},
EndTime: &timestamp.Timestamp{Seconds: 1544805927, Nanos: 460102000},
TimeEvents: nil,
TimeEvents: &tracepb.Span_TimeEvents{
TimeEvent: []*tracepb.Span_TimeEvent{
{
Time: &timestamp.Timestamp{Seconds: 1544805927, Nanos: 450000000},
Value: &tracepb.Span_TimeEvent_Annotation_{
Annotation: &tracepb.Span_TimeEvent_Annotation{
Description: &tracepb.TruncatableString{Value: "custom time event"},
},
},
},
},
},
},
{
TraceId: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0e, 0xd2, 0xe6, 0x3c, 0xbe, 0x71, 0xf5, 0xa8},
Expand Down