From e371a02bbdc55ac60e1df4716ede0a202dd20b6f Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Thu, 15 Aug 2019 22:12:30 +0200 Subject: [PATCH 1/8] extend ids to parse traceID from bytes Signed-off-by: Abhilash Gnan --- cmd/collector/app/zipkin/protov2.go | 18 ++---------------- model/ids.go | 23 +++++++++++++++-------- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/cmd/collector/app/zipkin/protov2.go b/cmd/collector/app/zipkin/protov2.go index ca4b8b0c681..d49667f3df3 100644 --- a/cmd/collector/app/zipkin/protov2.go +++ b/cmd/collector/app/zipkin/protov2.go @@ -42,7 +42,8 @@ func protoSpanV2ToThrift(s *zipkinProto.Span) (*zipkincore.Span, error) { return nil, fmt.Errorf("invalid length for Span ID") } id := binary.BigEndian.Uint64(s.Id) - traceID, err := traceIDFromBytes(s.TraceId) + traceID := model.TraceID{} + err := traceID.Unmarshal(s.TraceId) if err != nil { return nil, err } @@ -105,21 +106,6 @@ func protoSpanV2ToThrift(s *zipkinProto.Span) (*zipkincore.Span, error) { return tSpan, nil } -func traceIDFromBytes(tid []byte) (model.TraceID, error) { - var hi, lo uint64 - switch { - case len(tid) > model.TraceIDLongBytesLen: - return model.TraceID{}, fmt.Errorf("invalid length for traceId") - case len(tid) > model.TraceIDShortBytesLen: - hiLen := len(tid) - model.TraceIDShortBytesLen - hi = binary.BigEndian.Uint64(tid[:hiLen]) - lo = binary.BigEndian.Uint64(tid[hiLen:]) - default: - lo = binary.BigEndian.Uint64(tid) - } - return model.TraceID{High: hi, Low: lo}, nil -} - func protoRemoteEndpToAddrAnno(e *zipkinProto.Endpoint, kind zipkinProto.Span_Kind) (*zipkincore.BinaryAnnotation, error) { rEndp, err := protoEndpointV2ToThrift(e) if err != nil { diff --git a/model/ids.go b/model/ids.go index a814d039789..dccbb879abc 100644 --- a/model/ids.go +++ b/model/ids.go @@ -25,10 +25,10 @@ import ( ) const ( - // TraceIDShortBytesLen indicates length of 64bit traceID when represented as list of bytes - TraceIDShortBytesLen = 8 - // TraceIDLongBytesLen indicates length of 128bit traceID when represented as list of bytes - TraceIDLongBytesLen = 16 + // traceIDShortBytesLen indicates length of 64bit traceID when represented as list of bytes + traceIDShortBytesLen = 8 + // traceIDLongBytesLen indicates length of 128bit traceID when represented as list of bytes + traceIDLongBytesLen = 16 ) // TraceID is a random 128bit identifier for a trace @@ -102,11 +102,18 @@ func (t *TraceID) MarshalTo(data []byte) (n int, err error) { // Unmarshal inflates this trace ID from binary representation. Called by protobuf serialization. func (t *TraceID) Unmarshal(data []byte) error { - if len(data) < 16 { - return fmt.Errorf("buffer is too short") + var hi, lo uint64 + switch { + case len(data) > traceIDLongBytesLen: + return fmt.Errorf("invalid length for TraceID") + case len(data) > traceIDShortBytesLen: + hiLen := len(data) - traceIDShortBytesLen + hi = binary.BigEndian.Uint64(data[:hiLen]) + lo = binary.BigEndian.Uint64(data[hiLen:]) + default: + lo = binary.BigEndian.Uint64(data) } - t.High = binary.BigEndian.Uint64(data[:8]) - t.Low = binary.BigEndian.Uint64(data[8:]) + t.High, t.Low = hi, lo return nil } From 1be4256b08d7555ad4107667e1ccd5815f6016ec Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Mon, 19 Aug 2019 20:06:04 +0200 Subject: [PATCH 2/8] improve unmarshal ids Signed-off-by: Abhilash Gnan --- cmd/collector/app/zipkin/protov2.go | 21 +++++++++++---------- cmd/collector/app/zipkin/protov2_test.go | 6 +++--- model/ids.go | 13 +++++++++++-- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/cmd/collector/app/zipkin/protov2.go b/cmd/collector/app/zipkin/protov2.go index d49667f3df3..5a07b67e9a0 100644 --- a/cmd/collector/app/zipkin/protov2.go +++ b/cmd/collector/app/zipkin/protov2.go @@ -38,15 +38,17 @@ func protoSpansV2ToThrift(listOfSpans *zipkinProto.ListOfSpans) ([]*zipkincore.S } func protoSpanV2ToThrift(s *zipkinProto.Span) (*zipkincore.Span, error) { - if len(s.Id) != model.TraceIDShortBytesLen { - return nil, fmt.Errorf("invalid length for Span ID") + var id model.SpanID + var err error + if id, err = model.SpanIDFromBytes(s.Id); err != nil { + return nil, err } - id := binary.BigEndian.Uint64(s.Id) + traceID := model.TraceID{} - err := traceID.Unmarshal(s.TraceId) - if err != nil { + if err = traceID.Unmarshal(s.TraceId); err != nil { return nil, err } + ts, d := int64(s.Timestamp), int64(s.Duration) tSpan := &zipkincore.Span{ ID: int64(id), @@ -62,18 +64,17 @@ func protoSpanV2ToThrift(s *zipkinProto.Span) (*zipkincore.Span, error) { } if len(s.ParentId) > 0 { - if len(s.ParentId) != model.TraceIDShortBytesLen { - return nil, fmt.Errorf("invalid length for parentId") + var parentID model.SpanID + if parentID, err = model.SpanIDFromBytes(s.ParentId); err != nil { + return nil, err } - parentID := binary.BigEndian.Uint64(s.ParentId) signed := int64(parentID) tSpan.ParentID = &signed } var localE *zipkincore.Endpoint if s.LocalEndpoint != nil { - localE, err = protoEndpointV2ToThrift(s.LocalEndpoint) - if err != nil { + if localE, err = protoEndpointV2ToThrift(s.LocalEndpoint); err != nil { return nil, err } } diff --git a/cmd/collector/app/zipkin/protov2_test.go b/cmd/collector/app/zipkin/protov2_test.go index 2ba5637d576..362186f2e45 100644 --- a/cmd/collector/app/zipkin/protov2_test.go +++ b/cmd/collector/app/zipkin/protov2_test.go @@ -82,9 +82,9 @@ func TestIdErrs(t *testing.T) { span zipkinProto.Span errMsg string }{ - {span: zipkinProto.Span{Id: randBytesOfLen(16)}, errMsg: "invalid length for Span ID"}, - {span: zipkinProto.Span{Id: validID, TraceId: invalidTraceID}, errMsg: "invalid length for traceId"}, - {span: zipkinProto.Span{Id: validID, TraceId: validTraceID, ParentId: invalidParentID}, errMsg: "invalid length for parentId"}, + {span: zipkinProto.Span{Id: randBytesOfLen(16)}, errMsg: "invalid length for SpanID"}, + {span: zipkinProto.Span{Id: validID, TraceId: invalidTraceID}, errMsg: "invalid length for TraceID"}, + {span: zipkinProto.Span{Id: validID, TraceId: validTraceID, ParentId: invalidParentID}, errMsg: "invalid length for SpanID"}, } for _, test := range tests { _, err := protoSpanV2ToThrift(&test.span) diff --git a/model/ids.go b/model/ids.go index dccbb879abc..f1fd00f8c5c 100644 --- a/model/ids.go +++ b/model/ids.go @@ -173,6 +173,15 @@ func SpanIDFromString(s string) (SpanID, error) { return SpanID(id), nil } +// SpanIDFromBytes creates a SpandID from list of bytes +func SpanIDFromBytes(data []byte) (SpanID, error) { + id := SpanID(0) + if err := id.Unmarshal(data); err != nil { + return SpanID(0), err + } + return id, nil +} + // MarshalText is called by encoding/json, which we do not want people to use. func (s SpanID) MarshalText() ([]byte, error) { return nil, fmt.Errorf("unsupported method SpanID.MarshalText; please use github.com/gogo/protobuf/jsonpb for marshalling") @@ -197,8 +206,8 @@ func (s *SpanID) MarshalTo(data []byte) (n int, err error) { // Unmarshal inflates span ID from a binary representation. Called by protobuf serialization. func (s *SpanID) Unmarshal(data []byte) error { - if len(data) < 8 { - return fmt.Errorf("buffer is too short") + if len(data) != traceIDShortBytesLen { + return fmt.Errorf("invalid length for SpanID") } *s = NewSpanID(binary.BigEndian.Uint64(data)) return nil From 068bbd9e9d0ad839bff8948eddc86472157b8e7e Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Mon, 19 Aug 2019 20:27:09 +0200 Subject: [PATCH 3/8] fix tests Signed-off-by: Abhilash Gnan --- cmd/collector/app/zipkin/protov2.go | 4 ++-- model/ids.go | 33 +++++++++++++++++++---------- model/span_test.go | 2 +- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/cmd/collector/app/zipkin/protov2.go b/cmd/collector/app/zipkin/protov2.go index 5a07b67e9a0..bce0298a752 100644 --- a/cmd/collector/app/zipkin/protov2.go +++ b/cmd/collector/app/zipkin/protov2.go @@ -44,8 +44,8 @@ func protoSpanV2ToThrift(s *zipkinProto.Span) (*zipkincore.Span, error) { return nil, err } - traceID := model.TraceID{} - if err = traceID.Unmarshal(s.TraceId); err != nil { + var traceID model.TraceID + if traceID, err = model.TraceIDFromBytes(s.TraceId); err != nil { return nil, err } diff --git a/model/ids.go b/model/ids.go index f1fd00f8c5c..a051675c613 100644 --- a/model/ids.go +++ b/model/ids.go @@ -77,6 +77,24 @@ func TraceIDFromString(s string) (TraceID, error) { return TraceID{High: hi, Low: lo}, nil } +// TraceIDFromBytes creates a TraceID from list of bytes +func TraceIDFromBytes(data []byte) (TraceID, error) { + var t TraceID + var hi, lo uint64 + switch { + case len(data) > traceIDLongBytesLen: + return TraceID{}, fmt.Errorf("invalid length for TraceID") + case len(data) > traceIDShortBytesLen: + hiLen := len(data) - traceIDShortBytesLen + hi = binary.BigEndian.Uint64(data[:hiLen]) + lo = binary.BigEndian.Uint64(data[hiLen:]) + default: + lo = binary.BigEndian.Uint64(data) + } + t.High, t.Low = hi, lo + return t, nil +} + // MarshalText is called by encoding/json, which we do not want people to use. func (t TraceID) MarshalText() ([]byte, error) { return nil, fmt.Errorf("unsupported method TraceID.MarshalText; please use github.com/gogo/protobuf/jsonpb for marshalling") @@ -102,18 +120,11 @@ func (t *TraceID) MarshalTo(data []byte) (n int, err error) { // Unmarshal inflates this trace ID from binary representation. Called by protobuf serialization. func (t *TraceID) Unmarshal(data []byte) error { - var hi, lo uint64 - switch { - case len(data) > traceIDLongBytesLen: - return fmt.Errorf("invalid length for TraceID") - case len(data) > traceIDShortBytesLen: - hiLen := len(data) - traceIDShortBytesLen - hi = binary.BigEndian.Uint64(data[:hiLen]) - lo = binary.BigEndian.Uint64(data[hiLen:]) - default: - lo = binary.BigEndian.Uint64(data) + if len(data) < 16 { + return fmt.Errorf("buffer is too short") } - t.High, t.Low = hi, lo + t.High = binary.BigEndian.Uint64(data[:8]) + t.Low = binary.BigEndian.Uint64(data[8:]) return nil } diff --git a/model/span_test.go b/model/span_test.go index 71acb4cfb5b..dfd4d20a101 100644 --- a/model/span_test.go +++ b/model/span_test.go @@ -175,7 +175,7 @@ func TestSpanIDUnmarshalJSONErrors(t *testing.T) { err = id.UnmarshalJSONPB(nil, []byte("")) require.Error(t, err) - assert.Contains(t, err.Error(), "buffer is too short") + assert.Contains(t, err.Error(), "invalid length for SpanID") err = id.UnmarshalJSONPB(nil, []byte("123")) require.Error(t, err) assert.Contains(t, err.Error(), "illegal base64 data") From e64ff0af139d964668f2d1c84599eda534da7164 Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Tue, 20 Aug 2019 20:31:04 +0200 Subject: [PATCH 4/8] add tests Signed-off-by: Abhilash Gnan --- model/ids.go | 15 ++++++--------- model/ids_test.go | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/model/ids.go b/model/ids.go index a051675c613..54fc070925b 100644 --- a/model/ids.go +++ b/model/ids.go @@ -80,18 +80,15 @@ func TraceIDFromString(s string) (TraceID, error) { // TraceIDFromBytes creates a TraceID from list of bytes func TraceIDFromBytes(data []byte) (TraceID, error) { var t TraceID - var hi, lo uint64 switch { - case len(data) > traceIDLongBytesLen: - return TraceID{}, fmt.Errorf("invalid length for TraceID") - case len(data) > traceIDShortBytesLen: - hiLen := len(data) - traceIDShortBytesLen - hi = binary.BigEndian.Uint64(data[:hiLen]) - lo = binary.BigEndian.Uint64(data[hiLen:]) + case len(data) == traceIDLongBytesLen: + t.High = binary.BigEndian.Uint64(data[:traceIDShortBytesLen]) + t.Low = binary.BigEndian.Uint64(data[traceIDShortBytesLen:]) + case len(data) == traceIDShortBytesLen: + t.Low = binary.BigEndian.Uint64(data) default: - lo = binary.BigEndian.Uint64(data) + return TraceID{}, fmt.Errorf("invalid length for TraceID") } - t.High, t.Low = hi, lo return t, nil } diff --git a/model/ids_test.go b/model/ids_test.go index ffdd4125709..68f8c59aef6 100644 --- a/model/ids_test.go +++ b/model/ids_test.go @@ -86,3 +86,48 @@ func TestTraceSpanIDMarshalProto(t *testing.T) { }) } } + +func TestSpanIDFromBytes(t *testing.T) { + tests := []struct { + data []byte + errMsg string + }{ + {data: []byte{0, 0, 0, 0}, errMsg: "invalid length for SpanID"}, + {data: []byte{0, 0, 0, 0, 0, 0, 0, 13, 0}, errMsg: "invalid length for SpanID"}, + } + for _, test := range tests { + _, err := model.SpanIDFromBytes(test.data) + require.Error(t, err) + assert.Equal(t, err.Error(), test.errMsg) + } + + spanID, err := model.SpanIDFromBytes([]byte{0, 0, 0, 0, 0, 0, 0, 13}) + require.NoError(t, err) + assert.Equal(t, spanID, model.NewSpanID(13)) +} + +func TestTraceIDFromBytes(t *testing.T) { + errTests := [][]byte{ + []byte{0, 0, 0, 0, 0, 0, 0, 13, 0, 0, 0, 0, 0, 0, 0, 0, 13}, + []byte{0, 0, 0, 0, 0, 0, 0, 13, 0, 0, 0, 0, 0, 0, 13}, + []byte{0, 0, 0, 0, 0, 0, 13}, + } + for _, data := range errTests { + _, err := model.TraceIDFromBytes(data) + require.Error(t, err) + assert.Equal(t, err.Error(), "invalid length for TraceID") + } + + tests := []struct { + data []byte + expected model.TraceID + }{ + {data: []byte{0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 3}, expected: model.NewTraceID(2, 3)}, + {data: []byte{0, 0, 0, 0, 0, 0, 0, 2}, expected: model.NewTraceID(0, 2)}, + } + for _, test := range tests { + traceID, err := model.TraceIDFromBytes(test.data) + require.NoError(t, err) + assert.Equal(t, traceID, test.expected) + } +} From 71164476d7ac84da2f3976e4c494574c9dfb8fb9 Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Tue, 20 Aug 2019 20:45:32 +0200 Subject: [PATCH 5/8] fmt Signed-off-by: Abhilash Gnan --- model/ids_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/model/ids_test.go b/model/ids_test.go index 68f8c59aef6..d11cdcc9cb8 100644 --- a/model/ids_test.go +++ b/model/ids_test.go @@ -108,9 +108,9 @@ func TestSpanIDFromBytes(t *testing.T) { func TestTraceIDFromBytes(t *testing.T) { errTests := [][]byte{ - []byte{0, 0, 0, 0, 0, 0, 0, 13, 0, 0, 0, 0, 0, 0, 0, 0, 13}, - []byte{0, 0, 0, 0, 0, 0, 0, 13, 0, 0, 0, 0, 0, 0, 13}, - []byte{0, 0, 0, 0, 0, 0, 13}, + {0, 0, 0, 0, 0, 0, 0, 13, 0, 0, 0, 0, 0, 0, 0, 0, 13}, + {0, 0, 0, 0, 0, 0, 0, 13, 0, 0, 0, 0, 0, 0, 13}, + {0, 0, 0, 0, 0, 0, 13}, } for _, data := range errTests { _, err := model.TraceIDFromBytes(data) From 52aa14589a0f1d22d1176a1bb48893b6c82ca67f Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Tue, 20 Aug 2019 20:48:12 +0200 Subject: [PATCH 6/8] minor refactor Signed-off-by: Abhilash Gnan --- model/ids_test.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/model/ids_test.go b/model/ids_test.go index d11cdcc9cb8..aa9af0babfd 100644 --- a/model/ids_test.go +++ b/model/ids_test.go @@ -88,17 +88,14 @@ func TestTraceSpanIDMarshalProto(t *testing.T) { } func TestSpanIDFromBytes(t *testing.T) { - tests := []struct { - data []byte - errMsg string - }{ - {data: []byte{0, 0, 0, 0}, errMsg: "invalid length for SpanID"}, - {data: []byte{0, 0, 0, 0, 0, 0, 0, 13, 0}, errMsg: "invalid length for SpanID"}, + errTests := [][]byte{ + {0, 0, 0, 0}, + {0, 0, 0, 0, 0, 0, 0, 13, 0}, } - for _, test := range tests { - _, err := model.SpanIDFromBytes(test.data) + for _, data := range errTests { + _, err := model.SpanIDFromBytes(data) require.Error(t, err) - assert.Equal(t, err.Error(), test.errMsg) + assert.Equal(t, err.Error(), "invalid length for SpanID") } spanID, err := model.SpanIDFromBytes([]byte{0, 0, 0, 0, 0, 0, 0, 13}) From 6024a07d86d5e915a86843d780e16035c94ee44f Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Tue, 20 Aug 2019 21:24:23 +0200 Subject: [PATCH 7/8] fix tests Signed-off-by: Abhilash Gnan --- cmd/collector/app/zipkin/http_handler_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/collector/app/zipkin/http_handler_test.go b/cmd/collector/app/zipkin/http_handler_test.go index 3a29d56a13e..62a41d251f9 100644 --- a/cmd/collector/app/zipkin/http_handler_test.go +++ b/cmd/collector/app/zipkin/http_handler_test.go @@ -295,9 +295,9 @@ func TestSaveProtoSpansV2(t *testing.T) { resBody string }{ {Span: zipkinProto.Span{Id: validID, TraceId: validTraceID, LocalEndpoint: &zipkinProto.Endpoint{Ipv4: randBytesOfLen(4)}, Kind: zipkinProto.Span_CLIENT}, StatusCode: http.StatusAccepted}, - {Span: zipkinProto.Span{Id: randBytesOfLen(4)}, StatusCode: http.StatusBadRequest, resBody: "Unable to process request body: invalid length for Span ID\n"}, - {Span: zipkinProto.Span{Id: validID, TraceId: randBytesOfLen(32)}, StatusCode: http.StatusBadRequest, resBody: "Unable to process request body: invalid length for traceId\n"}, - {Span: zipkinProto.Span{Id: validID, TraceId: validTraceID, ParentId: randBytesOfLen(16)}, StatusCode: http.StatusBadRequest, resBody: "Unable to process request body: invalid length for parentId\n"}, + {Span: zipkinProto.Span{Id: randBytesOfLen(4)}, StatusCode: http.StatusBadRequest, resBody: "Unable to process request body: invalid length for SpanID\n"}, + {Span: zipkinProto.Span{Id: validID, TraceId: randBytesOfLen(32)}, StatusCode: http.StatusBadRequest, resBody: "Unable to process request body: invalid length for TraceID\n"}, + {Span: zipkinProto.Span{Id: validID, TraceId: validTraceID, ParentId: randBytesOfLen(16)}, StatusCode: http.StatusBadRequest, resBody: "Unable to process request body: invalid length for SpanID\n"}, {Span: zipkinProto.Span{Id: validID, TraceId: validTraceID, LocalEndpoint: &zipkinProto.Endpoint{Ipv4: randBytesOfLen(2)}}, StatusCode: http.StatusBadRequest, resBody: "Unable to process request body: wrong Ipv4\n"}, } for _, test := range tests { From 749c71db506328959f2873e0ffd8ff059c10c7dd Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Wed, 21 Aug 2019 19:29:33 +0200 Subject: [PATCH 8/8] use public func in Unmarshal Signed-off-by: Abhilash Gnan --- model/ids.go | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/model/ids.go b/model/ids.go index 54fc070925b..8b34f7c9a33 100644 --- a/model/ids.go +++ b/model/ids.go @@ -117,12 +117,9 @@ func (t *TraceID) MarshalTo(data []byte) (n int, err error) { // Unmarshal inflates this trace ID from binary representation. Called by protobuf serialization. func (t *TraceID) Unmarshal(data []byte) error { - if len(data) < 16 { - return fmt.Errorf("buffer is too short") - } - t.High = binary.BigEndian.Uint64(data[:8]) - t.Low = binary.BigEndian.Uint64(data[8:]) - return nil + var err error + *t, err = TraceIDFromBytes(data) + return err } func marshalBytes(dst []byte, src []byte) (n int, err error) { @@ -183,11 +180,10 @@ func SpanIDFromString(s string) (SpanID, error) { // SpanIDFromBytes creates a SpandID from list of bytes func SpanIDFromBytes(data []byte) (SpanID, error) { - id := SpanID(0) - if err := id.Unmarshal(data); err != nil { - return SpanID(0), err + if len(data) != traceIDShortBytesLen { + return SpanID(0), fmt.Errorf("invalid length for SpanID") } - return id, nil + return NewSpanID(binary.BigEndian.Uint64(data)), nil } // MarshalText is called by encoding/json, which we do not want people to use. @@ -214,11 +210,9 @@ func (s *SpanID) MarshalTo(data []byte) (n int, err error) { // Unmarshal inflates span ID from a binary representation. Called by protobuf serialization. func (s *SpanID) Unmarshal(data []byte) error { - if len(data) != traceIDShortBytesLen { - return fmt.Errorf("invalid length for SpanID") - } - *s = NewSpanID(binary.BigEndian.Uint64(data)) - return nil + var err error + *s, err = SpanIDFromBytes(data) + return err } // MarshalJSON converts span id into a base64 string enclosed in quotes.