Update udpTransport to only send Jaeger.thrift #147

black-adder · 2017-05-02T21:39:02Z

Need to remove the notion of peer in span.go and use the tags instead
Update thrift_span.go to accommodate the above change
Pass in logger to transport layer so that it can log errors if the batch is too big and our checks didn't suffice

coveralls · 2017-05-02T21:47:19Z

Coverage decreased (-1.3%) to 83.755% when pulling 86602b2 on do_something_useful into ea8f4c2 on master.

badiib · 2017-05-02T21:44:59Z

testutils/mock_agent.go

 	"github.com/uber/jaeger-client-go/thrift-gen/agent"
+	"github.com/uber/jaeger-client-go/thrift-gen/jaeger"
 	"github.com/uber/jaeger-client-go/thrift-gen/sampling"
 	"github.com/uber/jaeger-client-go/thrift-gen/zipkincore"


this is still a dependency?

aye since MockAgent has to implement EmitZipkinBatch until it can be removed from thrift.idl

badiib · 2017-05-02T21:45:47Z

transport_udp.go


-const defaultUDPSpanServerHostPort = "localhost:5775"
+const defaultUDPSpanServerHostPort = "localhost:6831"


why the change?

port 5775 on the agent is for zipkin-compact, 6831 is for jaeger-compact

badiib · 2017-05-02T21:46:41Z

transport_udp.go

+	if s.process == nil {
+		s.process = &j.Process{
+			ServiceName: span.tracer.serviceName,
+			Tags:        buildTags(span.tracer.tags),


can this ever change?

Nope, tracer level tags are set once in the constructor and that's it.

badiib · 2017-05-02T21:47:40Z

utils/udp_client.go

@@ -78,14 +79,19 @@ func NewAgentClientUDP(hostPort string, maxPacketSize int) (*AgentClientUDP, err

 // EmitZipkinBatch implements EmitZipkinBatch() of Agent interface
 func (a *AgentClientUDP) EmitZipkinBatch(spans []*zipkincore.Span) error {


this is gonna go away right?

Yeah, but to remove it here I need to remove it from the thrift.idl and to do that I need to remove this from all the clients first. So just stubbing out for now.

coveralls · 2017-05-03T04:12:48Z

Coverage increased (+0.05%) to 85.135% when pulling 54fc44f on do_something_useful into ea8f4c2 on master.

yurishkuro · 2017-05-03T16:04:57Z

span.go

-	}
-	if !handled {
-		s.tags = append(s.tags, Tag{key: key, value: value})
+		if !handler(s, key, value) {


I think once we switch to jaeger.thrift, the only special handler we need is for sampling.priority

yurishkuro · 2017-05-03T18:01:28Z

jaeger_span.go

-// buildJaegerSpan builds jaeger span based on internal span.
-func buildJaegerSpan(span *Span) *j.Span {
+// BuildJaegerSpan builds jaeger span based on internal span.
+func BuildJaegerSpan(span *Span) *j.Span {


BuildJaegerThrift(Span) would be better name. And the file name jaeger_thrift_span.go.

yurishkuro · 2017-05-03T18:04:25Z

propagation_test.go

@@ -102,7 +102,7 @@ func TestSpanPropagator(t *testing.T) {
 			sp.duration, sp.startTime = exp.duration, exp.startTime
 		}
 		assert.Equal(t, exp.context, sp.context, formatName)
-		assert.Equal(t, expTags, sp.tags, formatName)
+		assert.Equal(t, expTags, sp.tags[1:] /*skip span.kind tag*/, formatName)


if no code in the test has changed, why does the outcome change?

ext.RPCServerOption(childCtx) adds the span.kind=server tag which was previously handled 'specially'

yurishkuro · 2017-05-03T18:07:49Z

reporter_test.go

-	s.Equal(2, len(zSpan.Annotations), "expecting two annotations for events")
-	s.Equal(len(expected), len(zSpan.BinaryAnnotations),
+	span := s.collector.Spans()[0]
+	s.Equal(2, len(span.logs), "expecting two annotations for events")


nit: fix message. "expecting two logs" ?

yurishkuro · 2017-05-03T18:08:06Z

reporter_test.go

-	s.Equal(len(expected), len(zSpan.BinaryAnnotations),
+	span := s.collector.Spans()[0]
+	s.Equal(2, len(span.logs), "expecting two annotations for events")
+	s.Equal(len(expected), len(span.tags),
 		"expecting %d binary annotations", len(expected))


nit: fix message - shouldn't be mentioning annotations anywhere

yurishkuro · 2017-05-03T18:09:41Z

reporter_test.go

@@ -127,13 +122,9 @@ func (s *reporterSuite) TestClientSpanAnnotations() {
 	sp.Finish()
 	s.flushReporter()
 	s.Equal(2, len(s.collector.Spans()))
-	zSpan := s.collector.Spans()[0] // child span is reported first
-	s.EqualValues(zSpan.ID, sp2.(*Span).context.spanID)


do we have a flag to use / not-use zipkin's one-span-per-rpc model? I'd expect the span ID check to be there.

yurishkuro · 2017-05-03T18:51:41Z

transport_udp_test.go

-		assert.True(t, overhead <= emitSpanBatchOverhead,
-			"test %d, n=%d, expected overhead %d <= %d", i, n, overhead, emitSpanBatchOverhead)
+		overhead := transport.Len() - n*spanSize - processSize
+		assert.True(t, overhead <= emitBatchOverhead,


Should we add t.Logf("span count: %d, overhead: %d", n, overhead)? I am curious if it's less than 30 bytes now - I don't like leaving unexplained magic numbers in the code.

yurishkuro · 2017-05-03T18:52:54Z

transport_udp_test.go

-	require.Equal(t, 1, len(spans), "agent should have received the span")
-	assert.Equal(t, span.Name, spans[0].Name)
+	batches := agent.GetJaegerBatches()
+	require.Equal(t, 1, len(batches), "agent should have received the batch")


require 1 span

yurishkuro · 2017-05-03T18:54:54Z

transport_udp_test.go

-		spans := agent.GetZipkinSpans()
-		require.Equal(t, test.expectSpansFlushed, len(spans), test.description)
+		batches := agent.GetJaegerBatches()
+		if test.expectSpansFlushed > 0 {


not an equivalent test - what if this condition is false?

yurishkuro · 2017-05-03T18:56:13Z

transport_udp_test.go

@@ -149,23 +176,29 @@ func TestUDPSenderAppend(t *testing.T) {
 		if test.expectFlush {
 			time.Sleep(5 * time.Millisecond)
 		}
-		spans := agent.GetZipkinSpans()


you could minimize the changes to the tests if you extract spans var from batch[0].

yurishkuro · 2017-05-03T18:59:41Z

transport_udp_test.go

@@ -132,11 +159,11 @@ func TestUDPSenderAppend(t *testing.T) {
 	}


you may want to add another control variable expectBatchesFlushed to avoid doing if statements in the test

coveralls · 2017-05-04T15:47:04Z

Coverage decreased (-0.5%) to 85.246% when pulling 4666122 on do_something_useful into 39b0a42 on master.

coveralls · 2017-05-04T15:54:28Z

Coverage decreased (-0.5%) to 85.246% when pulling cea1de5 on do_something_useful into 39b0a42 on master.

coveralls · 2017-06-02T15:28:23Z

Coverage decreased (-0.4%) to 85.311% when pulling 8ad14a5 on do_something_useful into bcb0e9e on master.

badiib reviewed May 2, 2017

View reviewed changes

black-adder changed the title ~~[WIP] Update udpTransport to only send Jaeger.thrift~~ Update udpTransport to only send Jaeger.thrift May 3, 2017

yurishkuro reviewed May 3, 2017

View reviewed changes

black-adder mentioned this pull request May 3, 2017

Move zipkin specific logic into thrift_span.go #148

Merged

black-adder force-pushed the do_something_useful branch from 54fc44f to 4666122 Compare May 4, 2017 15:42

yurishkuro approved these changes Jun 1, 2017

View reviewed changes

black-adder added 8 commits June 2, 2017 10:51

Update udpTransport to only send Jaeger

2f44fd6

Update port

5ee4932

finish todos

cbc3372

remove TODO

2cb5319

address comments

29b8c2e

yay

1b0ba89

wow

475cf8b

rebase

8ad14a5

black-adder force-pushed the do_something_useful branch from cea1de5 to 8ad14a5 Compare June 2, 2017 15:24

black-adder merged commit fda19be into master Jun 2, 2017

black-adder deleted the do_something_useful branch June 2, 2017 15:36

yurishkuro mentioned this pull request Jun 15, 2017

Upgrade to jaeger.thrift data model #134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update udpTransport to only send Jaeger.thrift #147

Update udpTransport to only send Jaeger.thrift #147

black-adder commented May 2, 2017 •

edited

Loading

coveralls commented May 2, 2017

badiib May 2, 2017

black-adder May 2, 2017

badiib May 2, 2017

black-adder May 2, 2017

badiib May 2, 2017

black-adder May 2, 2017

badiib May 2, 2017

black-adder May 2, 2017

coveralls commented May 3, 2017

yurishkuro May 3, 2017

yurishkuro May 3, 2017

yurishkuro May 3, 2017

black-adder May 3, 2017

yurishkuro May 3, 2017

yurishkuro May 3, 2017

yurishkuro May 3, 2017

yurishkuro May 3, 2017

yurishkuro May 3, 2017

yurishkuro May 3, 2017

yurishkuro May 3, 2017

yurishkuro May 3, 2017

coveralls commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented Jun 2, 2017


		const defaultUDPSpanServerHostPort = "localhost:5775"
		const defaultUDPSpanServerHostPort = "localhost:6831"

		@@ -78,14 +79,19 @@ func NewAgentClientUDP(hostPort string, maxPacketSize int) (*AgentClientUDP, err

		// EmitZipkinBatch implements EmitZipkinBatch() of Agent interface
		func (a AgentClientUDP) EmitZipkinBatch(spans []zipkincore.Span) error {

		@@ -132,11 +159,11 @@ func TestUDPSenderAppend(t *testing.T) {
		}

Update udpTransport to only send Jaeger.thrift #147

Update udpTransport to only send Jaeger.thrift #147

Conversation

black-adder commented May 2, 2017 • edited Loading

coveralls commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented Jun 2, 2017

black-adder commented May 2, 2017 •

edited

Loading