Support -H, -B, --jaeger for headers and baggage #121

kriskowal · 2016-10-18T00:17:34Z

No description provided.

mention-bot · 2016-10-18T00:17:35Z

@kriskowal, thanks for your PR! By analyzing the history of the files in this pull request, we identified @prashantv, @abhinav and @blampe to be potential reviewers.

kriskowal · 2016-10-18T00:25:08Z

main.go

@@ -302,6 +330,7 @@ func runWithOptions(opts Options, out output) {
 	runBenchmark(out, opts, benchmarkMethod{
 		serializer: serializer,
 		req:        req,
+		tracer:     tracer,


could be removed and assume NoopTracer downstream from here

prashantv · 2016-10-18T20:36:30Z

transport/http.go

@@ -30,6 +30,8 @@ import (
 	"strconv"
 	"time"

+	"github.com/opentracing/opentracing-go"
+


nit: can remove blank since this is a third party import like the below import

prashantv · 2016-10-18T20:37:46Z

bench_method_test.go

 		ServiceName: "foo",
 		HostPorts:   []string{s.hostPort()},
 	}
-	tp, err := getTransport(tOpts, encoding.Thrift)
+	tp, err := getTransport(tOpts, encoding.Thrift, nil)


What is the difference in behaviour between nil here and NoopTracer in bench_method.go?

If you thread nil, TChannel and YARPC will fall back to the global tracer. I’m going to explicitly thread NoopTracer and mandate a non-nil tracer in getTransport, so there’s no question for yab.

prashantv · 2016-10-18T20:39:04Z

bench_method.go

@@ -38,7 +39,7 @@ type benchmarkMethod struct {
 // up by making some number of requests through it.
 func (m benchmarkMethod) WarmTransport(opts TransportOptions, warmupRequests int) (transport.Transport, error) {
 	opts.benchmarking = true
-	transport, err := getTransport(opts, m.serializer.Encoding())
+	transport, err := getTransport(opts, m.serializer.Encoding(), opentracing.NoopTracer{})


Just realized, baggage only gets with the Jaeger tracer right? Perhaps we can't do this unless we explicitly disallow baggage with benchmarking? Otherwise, the initial request will use baggage but the benchmarking requests will not.

Good observation. I’ll simplify this so the only rule regarding tracing is that it must be enabled to pass baggage. I’ll work out a way to ensure that only the first request is sampled, and sampling is off for benchmarking. Whether to use Jaeger and whether to sample should be orthogonal.

prashantv · 2016-10-18T20:40:48Z

main.go

+		opts.TOpts.CallerName = "yab-" + os.Getenv("USER")
+	}
+
+	var tracer opentracing.Tracer


can we move the logic of get a tracer into a separate method?

prashantv · 2016-10-18T20:44:07Z

transport/interface.go

+	Baggage         map[string]string
+	RoutingKey      string
+	RoutingDelegate string
+	ShardKey        string


I don't know if routing key/delegate/shard key belong as top level fields on the request, unless we've made those transport-agnostic concerns in YARPC?

These are made transport-agnostic in YARPC. There are equivalent HTTP headers for all of these properties and they’re threaded by reqmeta in YARPC. However, I didn’t finish that angle in this PR, so I’ll take them out for now. We can add them either when we refactor yab to use YARPC or explicitly plumb them in future work.

prashantv · 2016-10-18T20:45:18Z

transport/interface.go

+	RoutingDelegate string
+	ShardKey        string
+	Body            []byte
+	Tracer          opentracing.Tracer


The Request and Response objects only contain values that represent the on-wire presentation currently. I'm not sure if Tracer should be here, should we move it into the Call interface instead?

In YARPC, the tracer is a dependency for instantiating a transport. I’ll try to do something similar here instead.

HelloGrayson

Is this still a WIP? Can you prepend the title with "WIP - " in that case?

HelloGrayson · 2016-10-19T20:05:00Z

CHANGELOG.md

@@ -2,6 +2,9 @@ Changelog
 =========

 * Upgrade to TChannel 1.2.0 and add Open Tracing support.
+* Add support for -H key:value header arguments.


Suggest:

Add support for headers with argument -H key:value.

Add support for baggage with argument -B key:value when --jaeger is passed.

HelloGrayson

lgtm

HelloGrayson · 2016-10-27T00:06:46Z

main_test.go

@@ -384,8 +384,7 @@ func TestAlises(t *testing.T) {
 		{
 			args: []cmdArgs{
 				{"-P", "file"},
-				{"-H", "file"},


So we're stealing the -H flag for headers?

kriskowal · 2016-11-01T20:01:37Z

Added passing tests for baggage. Added a passing test for TChannel headers. Left a broken test for HTTP headers with #125

Now seeking only a stamp from @prashantv

kriskowal · 2016-11-01T20:06:17Z

main.go

 	return t.Call(ctx, request)
 }
+
+func makeInitialRequest(out output, transport transport.Transport, serializer encoding.Serializer, req *transport.Request) {


I am not sure where this new code is coming from. There might be rebase magic here.

yeah looks like the diff needs to be rebased off the latest dev branch

abhinav · 2016-11-01T23:52:09Z

bench_method.go

@@ -25,6 +25,7 @@ import (
 	"sync"
 	"time"

+	"github.com/opentracing/opentracing-go"


Import group ordering. Third party imports should be in a separate group.

+1, same throughout this change

kriskowal

import group ordering nits

kriskowal · 2016-11-01T23:54:07Z

transport.go

 	"strings"

+	"github.com/opentracing/opentracing-go"
+	"github.com/uber/tchannel-go"
 	"gopkg.in/yaml.v2"


third party group should be below first party group

kriskowal · 2016-11-01T23:54:27Z

transport/interface.go

@@ -23,15 +23,19 @@ package transport
 import (
 	"time"

+	"github.com/opentracing/opentracing-go"


should be in same group as bellow

kriskowal · 2016-11-01T23:54:48Z

transport_test.go

 	"os"
 	"testing"
 	"time"

+	"github.com/opentracing/opentracing-go"
+	opentracing_ext "github.com/opentracing/opentracing-go/ext"


should separate third party group below

prashantv · 2016-11-02T00:00:56Z

bench_method.go

@@ -25,6 +25,7 @@ import (
 	"sync"
 	"time"

+	"github.com/opentracing/opentracing-go"


+1, same throughout this change

prashantv · 2016-11-02T00:02:24Z

server_util_test.go

@@ -78,11 +83,21 @@ func (methodsT) echo() handler {

 func (methodsT) traceEnabled() handler {


can you add a comment that explains the returned result from the method

prashantv · 2016-11-02T00:04:54Z

bench_method_test.go

 	}{
 		{
 			hostPort: s.hostPort(),
 		},
-		{


this looks unrelated to headers -- should this have been handled in a previous diff?

This is a far flung effect of refactoring tracing. Tracing is no longer a transport level concern, since we no longer use trace sampling on the channel level. Consequently it becomes a request scoped concern and the logic to detect whether the caller was overridden had to be factored upward so I could validate benchmark options.

…which invalidated this test case.

prashantv · 2016-11-02T00:07:21Z

server_util_test.go

@@ -37,8 +39,10 @@ type server struct {

 type handler func(ctx context.Context, args *raw.Args) (*raw.Res, error)

-func newServer(t *testing.T) *server {
-	ch := testutils.NewServer(t, testutils.NewOpts().SetServiceName("foo").DisableLogVerification())
+func newServer(t *testing.T, tracer opentracing.Tracer) *server {


Rather than take a tracer (and get into the pattern of everyone passing nil for default options), we should probably just implement a lightweight options pattern here,

func withTracer(opentracing.Tracer) func(*testutils.ChannelOpts) { return func(opts *testutils.ChannelOpts) { opts.Tracer = tracer } } func newServer(t *testing.T, opts... func(*testutils.ChannelOpts)) { chOpts := testutils.NewOpts().[...] for _, opt := range opts { opt(chOpts) } [...] }

prashantv · 2016-11-02T00:09:59Z

server_util_test.go

+				ret = 0xfe
+			}
+		} else {
+			ret = 0xff


no span in context case returning 0 seems like a good default

these cases were helpful in isolating test errors during development. I’ll scale it back down.

prashantv · 2016-11-02T00:14:17Z

main.go

 	return t.Call(ctx, request)
 }
+
+func makeInitialRequest(out output, transport transport.Transport, serializer encoding.Serializer, req *transport.Request) {


yeah looks like the diff needs to be rebased off the latest dev branch

prashantv · 2016-11-02T00:16:14Z

request_test.go

@@ -141,7 +141,7 @@ func TestGetHeaders(t *testing.T) {
 	}

 	for _, tt := range tests {
-		got, err := getHeaders(tt.inline, tt.file)
+		got, err := getHeaders(tt.inline, tt.file, nil)


can we add unit tests for the override here

prashantv · 2016-11-02T00:16:31Z

transport/tchannel_test.go

@@ -174,7 +174,6 @@ func TestTChannelCallSuccessRaw(t *testing.T) {
 	for _, tt := range tests {
 		var lastSpan uint64
 		testutils.RegisterFunc(svr, "echo", func(ctx context.Context, args *raw.Args) (*raw.Res, error) {
-			// assert.False(t, tchannel.CurrentSpan(ctx).TracingEnabled(), "Tracing should be disabled")


intentional change? shouldn't this just be uncommenting?

prashantv · 2016-11-02T00:16:46Z

transport/tchannel.go

 func (t *tchan) Protocol() Protocol {
 	return TChannel
 }

 func (t *tchan) Call(ctx context.Context, r *Request) (*Response, error) {
-	call, err := t.sc.BeginCall(ctx, r.Method, t.callOptions)
+	req := *r


can you add a comment for why we do this

abhinav · 2016-11-01T23:54:27Z

bench_method_test.go

@@ -54,24 +55,18 @@ func benchmarkMethodForTest(t *testing.T, methodString string, p transport.Proto
 }

 func TestBenchmarkMethodWarmTransport(t *testing.T) {
-	s := newServer(t)
+	s := newServer(t, nil)


optional nit: naked nils are uninformative. Consider using a parameter object
where you set Tracer only when you need it. Alternatively, add a /* tracer */
comment next to the nil.

abhinav · 2016-11-02T00:29:44Z

main.go

+func getTracer(opts Options, out output) (opentracing.Tracer, io.Closer) {
+	var tracer opentracing.Tracer
+	var closer io.Closer
+	tracer = opentracing.NoopTracer{}


nit: use var group and initialize tracer at declaration:

var ( tracer opentracing.Tracer = opentracing.NoopTracer{} closer io.Closer )

prashantv

lgtm, just left a few minor comments

prashantv · 2016-11-02T15:20:25Z

bench_method_test.go

 	}{
 		{
 			hostPort: s.hostPort(),
 		},
-		{


prashantv · 2016-11-02T15:25:09Z

integration_test.go

+	val := span.BaggageItem("baggagekey")
+	if val == "" {
+		return errors.New("missing baggage")
+	} else if val != "baggagevalue" {


nit: no need for else since the above case returns,

val := span.BaggageItem(..) if val == "" { return errors.New("missing baggage") } if val != "baggagevalue" { return errors.New("unexpected baggage") } return nil

prashantv · 2016-11-02T15:26:04Z

integration_test.go

+
+func verifyThriftHeaders(ctx thrift.Context) error {
+	headers := ctx.Headers()
+	if val, ok := headers["headerkey"]; ok {


nit: i'd write this the same as the verifyBaggage to reduce nesting

val, ok := ctx.Headers()["headerkey"] if !ok { return errors.New("missing header") } if val != "headervalue" { return errors.New("unexpected header") } return nil

prashantv · 2016-11-02T15:26:59Z

integration_test.go

+}
+
+func verifyYARPCHeaders(reqMeta yarpc.ReqMeta) error {
+	headers := reqMeta.Headers()


nit: can we instead have a single veriyHeaders(headers map[string]string) error method, and pass in the ctx.Headers() or reqMeta.Headers()? That way we can avoid the duplication here

YARPC does not expose Headers().Items().

Ahh, I forgot that yarpc headers are not an actual map, this is fine then.

prashantv · 2016-11-02T15:31:52Z

transport/tchannel.go

@@ -75,7 +81,7 @@ func NewTChannel(opts TChannelOptions) (Transport, error) {
 	}

 	callerName := opts.SourceService
-	if cn, ok := opts.TransportOpts["cn"]; ok {
+	if cn, ok := opts.TransportOpts["cn"]; ok && cn != "" {


nit: should we keep this as is -- it might be we want to test what happens if the cn header is empty in a request.

kriskowal commented Oct 18, 2016

View reviewed changes

prashantv reviewed Oct 18, 2016

View reviewed changes

kriskowal force-pushed the baggage branch from 4ad4ddd to 0803963 Compare October 19, 2016 03:46

kriskowal changed the base branch from dev to tchannel-v1.2.0 October 19, 2016 03:47

kriskowal changed the title ~~WIP - support -H, -B, --jaeger for headers and baggage~~ Support -H, -B, --jaeger for headers and baggage Oct 19, 2016

HelloGrayson reviewed Oct 19, 2016

View reviewed changes

kriskowal force-pushed the baggage branch from 0803963 to c18cf2b Compare October 21, 2016 00:38

HelloGrayson approved these changes Oct 27, 2016

View reviewed changes

kriskowal force-pushed the baggage branch from f03c011 to 61c1484 Compare November 1, 2016 19:51

kriskowal commented Nov 1, 2016

View reviewed changes

abhinav reviewed Nov 1, 2016

View reviewed changes

kriskowal commented Nov 1, 2016

View reviewed changes

prashantv reviewed Nov 2, 2016

View reviewed changes

kriskowal force-pushed the baggage branch from 4bd6d6e to 42ea4ee Compare November 2, 2016 00:24

abhinav reviewed Nov 2, 2016

View reviewed changes

kriskowal changed the base branch from tchannel-v1.2.0 to dev November 2, 2016 00:32

kriskowal added 12 commits November 1, 2016 17:40

Add support for -H, -B, --jaeger headers, baggage, and jaeger

8dab554

Clarify changelog

d575f11

Thread tracer through transports

84cc0de

Make explicit tracer required to construct a transport

6226778

Factor out getTracer

d6d5515

Separate routing work

8336d81

Threaded tracer through http

466559e

Add integration test for baggage and headers over TChannel

e927a7c

Typo fix

4598a67

Reenable trace enabled tests

843a936

Fix import grouping

9f9245f

Use light options pattern for newServer test helper

9b85a05

Use light options pattern for newServer test helper

8b3169b

kriskowal force-pushed the baggage branch from 4ccf51d to 8b3169b Compare November 2, 2016 00:40

abhinav approved these changes Nov 2, 2016

View reviewed changes

kriskowal added 3 commits November 1, 2016 17:49

Nits

34b480b

Add tests for overridden headers

a11503b

Explain the shallow copy of the request object in tchannel

7cea315

prashantv approved these changes Nov 2, 2016

View reviewed changes

Address nits

d2515f2

kriskowal merged commit e7c5b5a into dev Nov 2, 2016

This was referenced Nov 2, 2016

Update to latest tchannel with OpenTracing integration #114

Closed

Support opentracing baggage #113

Closed

kriskowal deleted the baggage branch November 2, 2016 22:41

		@@ -78,11 +83,21 @@ func (methodsT) echo() handler {

		func (methodsT) traceEnabled() handler {

Support -H, -B, --jaeger for headers and baggage #121

Support -H, -B, --jaeger for headers and baggage #121

Conversation

kriskowal commented Oct 18, 2016

mention-bot commented Oct 18, 2016

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

HelloGrayson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HelloGrayson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented Nov 1, 2016

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

kriskowal left a 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

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

prashantv left a 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