[m3db] Add Jaeger tracing to m3db #1506

benraskin92 · 2019-03-28T14:49:23Z

prateek · 2019-03-28T16:21:28Z

src/cmd/services/m3dbnode/config/config.go

@@ -137,6 +137,9 @@ type DBConfiguration struct {

 	// Write new series asynchronously for fast ingestion of new ID bursts.
 	WriteNewSeriesAsync bool `yaml:"writeNewSeriesAsync"`
+
+	// Tracing configures opentracing. If not provided, tracing is disabled.
+	Tracing instrument.TracingConfiguration `yaml:"tracing"`


i'd make this a pointer type to make it optional to specify

This should be fine since we check if its a no-op in server.go:

if _, ok := tracer.(opentracing.NoopTracer); ok { logger.Info("tracing disabled for m3dbnode; set `tracing.backend` to enable") }

prateek · 2019-03-28T16:22:04Z

src/dbnode/network/server/tchannelthrift/context.go

@@ -40,7 +40,9 @@ func RegisterServer(channel *tchannel.Channel, service thrift.TChanServer, conte
 	server := thrift.NewServer(channel)
 	server.Register(service, thrift.OptPostResponse(postResponseFn))
 	server.SetContextFn(func(ctx xnetcontext.Context, method string, headers map[string]string) thrift.Context {
-		ctxWithValue := xnetcontext.WithValue(ctx, contextKey, contextPool.Get())
+		xCtx := contextPool.Get()
+		xCtx.SetGoContext(xnetcontext.TODO())


can replace this line with xCtx.SetGoContext(ctx)

benraskin92 · 2019-03-29T16:13:26Z

src/dbnode/network/server/tchannelthrift/node/service.go

@@ -66,6 +70,9 @@ const (
 	maxSegmentArrayPooledLength = 32
 	// Any pooled error slices that grow beyond this capcity will be thrown away.
 	writeBatchPooledReqPoolMaxErrorsSliceSize = 4096
+
+	fetchTaggedSpanID = "fetch"


Better names

how about just "fetchTagged"

benraskin92 · 2019-03-29T17:33:09Z

scripts/development/m3_stack/start_m3.sh

@@ -11,7 +11,8 @@ fi

 echo "Bringing up nodes in the background with docker compose, remember to run ./stop.sh when done"
 docker-compose -f docker-compose.yml up $DOCKER_ARGS m3coordinator01
-docker-compose -f docker-compose.yml up $DOCKER_ARGS m3db_seed
+docker-compose -f docker-compose.yml up --build -d --renew-anon-volumes m3db_seed
+# docker-compose -f docker-compose.yml up $DOCKER_ARGS m3db_seed


Me testing - will revert before landing.

wanna just add an environment variable for it? :P

benraskin92 · 2019-03-29T17:34:09Z

scripts/development/m3_stack/start_m3.sh

@@ -211,6 +212,11 @@ if [[ "$AGGREGATOR_PIPELINE" = true ]]; then
    curl http://localhost:7206/api/v1/json/report -X POST -d '{"metrics":[{"type":"gauge","value":42,"tags":{"__name__":"foo_metric","foo":"bar"}}]}'
 fi

+if [[ "$START_JAEGER" = true ]] ; then
+    docker-compose -f docker-compose.yml up $DOCKER_ARGS jaeger


Add check to make sure localhost:16686 is available.

codecov · 2019-03-29T17:34:35Z

Codecov Report

Merging #1506 into master will increase coverage by <.1%.
The diff coverage is 72.3%.

@@           Coverage Diff            @@
##           master   #1506     +/-   ##
========================================
+ Coverage    71.7%   71.8%   +<.1%     
========================================
  Files         938     938             
  Lines       76588   76693    +105     
========================================
+ Hits        54974   55088    +114     
+ Misses      18092   18083      -9     
  Partials     3522    3522

Flag	Coverage Δ
#aggregator	`82.3% <ø> (ø)`	⬆️
#cluster	`85.7% <ø> (-0.1%)`	⬇️
#collector	`63.7% <ø> (ø)`	⬆️
#dbnode	`80.6% <72.1%> (ø)`	⬆️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74% <ø> (+0.1%)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.5% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (ø)`	⬆️
#query	`66.5% <100%> (ø)`	⬆️
#x	`85.7% <ø> (+0.3%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2228f13...491f5da. Read the comment docs.

benraskin92 · 2019-03-29T17:34:34Z

src/dbnode/config/m3dbnode-local-etcd.yml

@@ -27,6 +27,9 @@ coordinator:
    idScheme: quoted

 db:
+  tracing:


Remove before landing.

benraskin92 · 2019-04-02T13:50:02Z

src/dbnode/x/opentracing/context.go

@@ -0,0 +1,82 @@
+// Copyright (c) 2019 Uber Technologies, Inc.


Remove this once m3db/m3x#214 lands

benraskin92 · 2019-04-02T13:50:16Z

src/dbnode/x/opentracing/context_test.go

@@ -0,0 +1,106 @@
+// Copyright (c) 2019 Uber Technologies, Inc.


Remove this once m3db/m3x#214 lands

benraskin92 · 2019-04-02T17:11:42Z

scripts/development/m3_stack/start_m3.sh

+    docker-compose -f docker-compose.yml up $DOCKER_ARGS jaeger
+    sleep 5
+    JAEGER_STATUS=$(curl -s -o /dev/null -w '%{http_code}' localhost:14269)
+    if [ $JAEGER_STATUS -ne 204 ]; then


Once jaegertracing/jaeger#1450 is fixed, we can change this logic.

nit: for posterity, could you put in a comment saying the same thing in the file

Perhaps a nit, but is there not an explicit way to state the dependency in docker-compose.yml? https://docs.docker.com/compose/compose-file/ mentions depends_on

richardartoul · 2019-04-02T19:55:50Z

scripts/development/m3_stack/README.md

+
+To start Jaeger, add the following to your `m3dbnode.yml` file under `db`:
+
+```yaml


Is there any reason we can't just include this by default? Does it break if the jaeger image isnt running?

Yes- it'll break if Jaeger isn't running here:

localAgentHostPort: jaeger:6831

richardartoul · 2019-04-02T19:57:14Z

scripts/development/m3_stack/start_m3.sh

@@ -11,7 +11,8 @@ fi

 echo "Bringing up nodes in the background with docker compose, remember to run ./stop.sh when done"
 docker-compose -f docker-compose.yml up $DOCKER_ARGS m3coordinator01
-docker-compose -f docker-compose.yml up $DOCKER_ARGS m3db_seed
+docker-compose -f docker-compose.yml up --build -d --renew-anon-volumes m3db_seed
+# docker-compose -f docker-compose.yml up $DOCKER_ARGS m3db_seed


wanna just add an environment variable for it? :P

richardartoul · 2019-04-02T19:58:06Z

src/dbnode/network/server/tchannelthrift/node/service.go

@@ -66,6 +70,9 @@ const (
 	maxSegmentArrayPooledLength = 32
 	// Any pooled error slices that grow beyond this capcity will be thrown away.
 	writeBatchPooledReqPoolMaxErrorsSliceSize = 4096
+
+	fetchTaggedSpanID = "fetch"


how about just "fetchTagged"

richardartoul · 2019-04-02T19:58:54Z

src/dbnode/network/server/tchannelthrift/node/service.go

+	goCtx, exists := ctx.GoContext()
+	if exists {
+		sp, spCtx = opentracingutil.StartSpanFromContext(goCtx, querySpanID)
+		sp.LogFields(


will this get sampled?

Yeah, can we only go down this path if this request is being sampled? Is there a way to query whether this context is being sampled or not?

Yeah the default sampling rate is quite low for Jaeger.

richardartoul · 2019-04-02T20:00:09Z

src/dbnode/network/server/tchannelthrift/node/service.go

+		)
+
+		ctx.SetGoContext(spCtx)
+		defer sp.Finish()


This finish only applies to the jaeger trace right?

Yes I believe so.

richardartoul · 2019-04-02T20:04:37Z

src/dbnode/network/server/tchannelthrift/node/service.go

+		)
+
+		ctx.SetGoContext(spCtx)
+		defer sp.Finish()


Do we have to close the span? The context cant do that for us?

richardartoul · 2019-04-02T20:05:23Z

src/dbnode/server/server.go

@@ -147,6 +151,13 @@ func Run(runOpts RunOptions) {
 		os.Exit(1)
 	}

+	// Currently, the zapLogger is being used for tracing only.


how come we have to use zap for tracing? is that just how it works?

The thing Andrew wrote to create the tracer in m3x uses jaegerzap.NewLogger() - also @prateek wants to move to using a zap logger anyway.

yeah, i'll try to get around to this soon. want all our code to use zap instead of m3/x/log

Agreed, it is time to kill the x logger soon.

PS we should really add the ZapLogger to instrument options (or pull the Zap logger from there? Does it not already get created in instrument options by default?).

richardartoul · 2019-04-02T20:06:22Z

src/dbnode/server/server.go

+	} else {
+		// setup tracer
+		tracer, traceCloser, err = cfg.Tracing.NewTracer(serviceName, scope, zapLogger)
+		if err != nil {


Is there any way where we can keep the tracing configuration in config by default and have it just not break anything if jaeger is not available? I don't know how the buffering / the library work internally

so iiuc, traces are sent over UDP and will just silently fail if there is no Jaeger collector setup. However, if you don't have the Jaeger config at all, a no-op tracer is created which doesn't send any traces.

richardartoul · 2019-04-02T20:11:52Z

src/dbnode/storage/database.go

+
+	goCtx, exists := ctx.GoContext()
+	if exists {
+		sp, spCtx = opentracingutil.StartSpanFromContext(goCtx, dbQuerySpanID)


Does this span automatically get tied together with the span from the RPC layer?

Also could we add a method to our context that makes adding each span less redundant? Maybe something like:

sp, closer := ctx.StartSpan( spanID, opentracinglog.String("query", query.String()), opentracinglog.String("namespace", namespace.String()), opentracinglog.Int("limit", opts.Limit), opentracinglog.Time("start", opts.StartInclusive), opentracinglog.Time("start", opts.StartInclusive), ) defer closer() sp.LogFields() ...

and all of the ctx.GoContext() if exists etc boilerplate can get shoved into that helper method

That way its a little cleaner/shorter and the API allows you to add the logs that you have immediately right away and then if you need to add more later you can use the returned sp object.

richardartoul · 2019-04-02T20:13:18Z

src/dbnode/storage/database.go

+	if exists {
+		sp, spCtx = opentracingutil.StartSpanFromContext(goCtx, dbQuerySpanID)
+		sp.LogFields(
+			opentracinglog.String("query", query.String()),


Do we need to add the query to the span at every level? Same for StartInclusive and EndExclusive.

maybe I don't know how tracing works but I thought you could just add that information in one span and it was would associated with the entire trace and you wouldn't need to do it at each layer

I believe logs are associated with just a single span, but I'll double check

I believe logs are associated with just a single span, but I'll double check

So I think that's true, but I don't think that means we need to associate every span with the query. If we've already found the whole trace (multiple spans), we can get the query from the relevant span there. No point in logging it everywhere (and in fact, it's kind of confusing to do so, since that info isn't really available further down in the call stack, I assume)

robskillington · 2019-04-02T20:45:30Z

src/dbnode/network/server/tchannelthrift/node/service.go

+			opentracingutil.Time("end", time.Unix(0, req.RangeStart)),
+		)
+
+		ctx.SetGoContext(spCtx)


Hm, this seems racey if multiple goroutines have reference to the same ctx.

richardartoul · 2019-04-02T22:06:30Z

src/dbnode/server/server.go

+		// setup tracer
+		tracer, traceCloser, err = cfg.Tracing.NewTracer(serviceName, scope, zapLogger)
+		if err != nil {
+			logger.Fatalf("could not initialize tracing for m3dbnode: %v", err)


Change this to log an error, create a NOOP reporter, then continue.

robskillington · 2019-04-06T21:35:07Z

@benraskin92 let us know when this is good for another pass. This will be a great addition.

benraskin92 · 2019-04-08T15:42:49Z

glide.yaml

@@ -12,6 +12,9 @@ import:
  - package: github.com/m3db/stackadler32
    version: bfebcd73ef6ffe0ee30489227f0330c39064b674

+  - package: github.com/MichaelTJones/pcg


Can probably remove this dependency - will take a look.

nah, we use it in m3db/src/x/sync/pooled_worker_pool.go

prateek · 2019-04-08T15:45:07Z

docker/jaeger/Dockerfile

@@ -0,0 +1 @@
+FROM jaegertracing/all-in-one:1.9


do we need this file? as in, can we just refer to the upstream image in the docker-compose?

Maybe.. let me test.

prateek · 2019-04-08T15:48:36Z

scripts/development/m3_stack/README.md

+To start Jaeger, you need to set the environment variable `START_JAEGER` to `true` when you run `start_m3.sh`.
+
+```
+START_JAEGER=true ./start_m3.sh


nit: should this be USE_JAEGER instead? cause we do have to stop it in stop_m3.sh

prateek · 2019-04-08T15:56:58Z

scripts/development/m3_stack/docker-compose.yml

+  jaeger:
+    networks:
+      - backend
+    build:


more a question: can you skip the build: section here?

It appears so..

prateek · 2019-04-08T15:58:21Z

scripts/development/m3_stack/m3dbnode.yml

@@ -2,6 +2,15 @@ db:
  logging:
    level: info

+  tracing:


hm are you going to leave this in? if so, why does the readme need the instructions for config updates?

I changed the logic so that if it errors creating the tracer, we just log it and default to a noop - so it is safe to leave it in here.

In case someone wants to change the sampling rate, etc.

fair enough.

I changed the logic so that if it errors creating the tracer, we just log it and default to a noop - so it is safe to leave it in here.

I'm not sure I agree with that change; the most likely reason for that is invalid configuration, which it seems worthwhile to surface. Since it's analogous, do we handle failure to initialize tally etc in the same way? Would be good to be consistent.

So I definitely log the error, but it would be kind of bad if m3db couldn't start if your jaeger host was down for some reason.

Fair enough! My assumption was that the jaeger client would come up even if the host is down (and then just fail to log), and that it would error primarily in cases of misconfiguration. However, I could see the argument for not going down if jaeger for instance changes their configuration in a backwards incompatible way (though then the user upon upgrading will just have to notice that their traces are no longer working).

I'm good with this though; thanks for discussion!

prateek · 2019-04-08T15:59:01Z

scripts/development/m3_stack/start_m3.sh

 docker-compose -f docker-compose.yml up $DOCKER_ARGS m3coordinator01
+# docker-compose -f docker-compose.yml up --build -d --renew-anon-volumes m3db_seed


does this need to be removed?

Yes - sorry will remove before i land.

cool, please put a TODO(braskin) or something else in case you're going to keep it around.

prateek · 2019-04-08T15:59:37Z

src/dbnode/config/m3dbnode-local-etcd.yml

+  # Uncomment this to enable local jaeger tracing. See https://www.jaegertracing.io/docs/1.9/getting-started/
+  # for quick local setup (which this config will send data to).
+
+  tracing:


hm, should this be commented out?

also, should it be in all the config files?

Yes - but instead of this, I'd rather just have an annotated config section like http://m3db.github.io/m3/query_engine/config/annotated_config.yaml. I can add them to all of the configs but I think that's overkill.

prateek · 2019-04-08T16:05:30Z

src/dbnode/network/server/tchannelthrift/node/service.go

@@ -66,6 +69,9 @@ const (
 	maxSegmentArrayPooledLength = 32
 	// Any pooled error slices that grow beyond this capcity will be thrown away.
 	writeBatchPooledReqPoolMaxErrorsSliceSize = 4096
+
+	fetchTaggedSpanID = "rpc_fetch_tagged"


We should do some standardisation of the names we use when creating spans. The ideal is to have a easy 1-1 mapping from name to the code, without requiring users to look at the code itself (to make it easy to read the trace output in jaeger).

I'd suggest a convention like: packageName.objectName.Method or packageName.Method in case there isn't an object. where packageName is abbreviated enough to make it clear without being too verbose. So in this case, it'd be tchannelthrift/node.Service.Query

What do you think?

let me read a bit about opentracing/jaeger naming conventions.

how about keeping these names, but including tags? for example, this one would still be called rpc_fetch_tagged, but we could have a series of tags like:

method:query package:node component:tchannelthrift

prateek · 2019-04-08T16:07:23Z

src/dbnode/network/server/tchannelthrift/node/service.go

@@ -66,6 +69,9 @@ const (
 	maxSegmentArrayPooledLength = 32
 	// Any pooled error slices that grow beyond this capcity will be thrown away.
 	writeBatchPooledReqPoolMaxErrorsSliceSize = 4096
+
+	fetchTaggedSpanID = "rpc_fetch_tagged"
+	querySpanID       = "rpc_query"


another point (this one I'm not as sure of) - how would you feel about moving all these constants as exported in a separate package (say src/dbnode/tracepoint). Would help discoverability. And also provide a nice single place to document the convention of naming these things.

If we go with the tags approach, i'd say leave it in the individual files.

prateek · 2019-04-08T16:09:08Z

src/dbnode/network/server/tchannelthrift/node/service.go

+	var sp opentracing.Span
+	result, sp, err := s.query(tctx, req)
+
+	if sp != nil {


hm, can you rewire the span creation to give you back a NoOpSpan when we're not tracing? that way we'd be able to avoid if sp != nil everywhere

prateek · 2019-04-08T16:10:33Z

src/dbnode/network/server/tchannelthrift/node/service.go

+
+	ctx = tchannelthrift.Context(tctx)
+
+	ctx, sp, ok = ctx.StartTraceSpan(querySpanID)


hm, how about starting the span in the Query() method. just to minimise the amount of observability code we're putting side by side with execution logic

hm why do we need to return ok from StartTraceSpan ?

We technically don't but I guess it's a little cleaner than doing a sp != nil

prateek · 2019-04-09T01:14:16Z

src/dbnode/network/server/tchannelthrift/node/service.go

+	if err != nil {
+		sp.LogFields(opentracinglog.Error(err))
+	}
+	sp.Finish()


nit: probably easiest to follow the defer sp.Finish() pattern. i.e. issue the defer right after constructing the span.

In this method since it is not in the super hot path that makes sense. If this was a hot high frequency path I would say avoiding defer is a good idea.

I'll leave without the defer to establish a pattern.

prateek · 2019-04-09T01:14:29Z

src/dbnode/network/server/tchannelthrift/node/service.go

+	)
+
+	ctx = tchannelthrift.Context(tctx)
+	ctx, sp, ok = ctx.StartTraceSpan(tracepoint.FetchTaggedSpanID)


Just wanted to leave comment about avoiding the string casts and varargs slice alloc for the call to LogFields(...) when trace is not being sampled.

In low frequency methods it probably doesn't matter all that much, but we may as well establish the pattern so that in high frequency hot methods we're avoiding allocs by default with the pattern we establish.

Will leave this as is.

prateek · 2019-04-09T01:15:53Z

src/x/context/context.go

@@ -316,7 +320,7 @@ func (c *ctx) spanIsSampled(sp opentracing.Span) bool {
 func (c *ctx) StartTraceSpan(name string) (Context, opentracing.Span, bool) {
 	goCtx, exists := c.GoContext()
 	if !exists || c.checkedAndNotSampled {
-		return c, nil, false
+		return c, noopTracer.StartSpan(name), false


I think you can change the signature to StartTraceSpan(name string) (Context, opentracing.Span) to make this easier to use

Hey, as per earlier comment I left - knowing whether it's being sampled or not is important so for hot path code we can avoid doing anything that might cause an allocation unless the trace is absolutely being sampled. Hence the bool is important.

prateek · 2019-04-09T01:18:56Z

src/dbnode/tracepoint/tracepoint.go

+	QuerySpanID = "tchannelthrift/node.service.Query"
+
+	// DBQueryIDs is the operation name for the db QueryIDs path
+	DBQueryIDs = "storage.db.QueryIDs"


nit: first two vars in this file end with ...SpanID, the last two don't. Mind sticking to one convention.

Probably remove the ...SpanID suffix, since someone will forget it at some point hah.

prateek · 2019-04-09T01:19:06Z

src/dbnode/tracepoint/tracepoint.go

+// If there isn't an object, use `packageName.method`.
+
+const (
+	// FetchTaggedSpanID is the operation name for the tchannelthrift FetchTagged path


super nit: end comments with a period.

robskillington · 2019-04-09T06:26:15Z

src/dbnode/network/server/tchannelthrift/node/service_test.go

 	"fmt"
 	"sort"
 	"testing"
 	"time"

+	"github.com/m3db/m3/src/dbnode/tracepoint"
+


nit: Unnecessary empty endline?

robskillington · 2019-04-09T06:31:02Z

src/dbnode/storage/namespace.go

 		return index.QueryResult{},
-			xerrors.NewRetryableError(errIndexNotBootstrappedToRead)
+			xerrors.NewRetryableError(err)
 	}

 	res, err := n.reverseIndex.Query(ctx, query, opts)


Hm, maybe should also if err != nil && sp != nil { sp.LogFields(opentracinglog.Error(err)) } here?

Updates Docker Add docker compose Update tests Update docs Log more errors Make tracing config a pointer Remove config Fix test Add to start_m3 Return 1 Comments Updates Fix tests Update m3x package Pin pcg package version Remove comments Update glide Address comments Update naming convention Address more comments remove build

prateek · 2019-04-10T16:28:32Z

src/dbnode/network/server/tchannelthrift/node/service.go

@@ -255,13 +259,35 @@ func (s *service) Bootstrapped(ctx thrift.Context) (*rpc.NodeBootstrappedResult_
 }

 func (s *service) Query(tctx thrift.Context, req *rpc.QueryRequest) (*rpc.QueryResult_, error) {
+	var (


don't need these allocs ahead of time now?

prateek · 2019-04-10T16:29:34Z

src/dbnode/network/server/tchannelthrift/node/service.go

+		sp  opentracing.Span
+	)
+
+	ctx = tchannelthrift.Context(tctx)


could this be:

ctx, sp := tchannelthrift.Context(tctx).StartTraceSpan(tracepoint.Query)

prateek · 2019-04-10T16:29:50Z

src/dbnode/network/server/tchannelthrift/node/service.go

+	)
+
+	ctx = tchannelthrift.Context(tctx)
+	ctx, sp = ctx.StartTraceSpan(tracepoint.FetchTagged)


prateek · 2019-04-10T16:30:28Z

src/dbnode/server/server.go

@@ -21,6 +21,7 @@
 package server

 import (
+	stdlib "context"


nit: need a name that's clearer about defining the import "stdcontext" or "stdlibcontext" or "context" instead of "stdlib" please

prateek · 2019-04-10T16:31:11Z

src/dbnode/storage/database.go

@@ -708,8 +712,21 @@ func (d *db) QueryIDs(
 	query index.Query,
 	opts index.QueryOptions,
 ) (index.QueryResult, error) {
+	var sp opentracing.Span
+	ctx, sp = ctx.StartTraceSpan(tracepoint.DBQueryIDs)


does ctx, sp := ... get :( here?

prateek · 2019-04-10T16:31:25Z

src/dbnode/storage/database_test.go

@@ -21,13 +21,16 @@
 package storage

 import (
+	stdlib "context"


prateek · 2019-04-10T16:31:44Z

src/dbnode/storage/database_test.go

 	"errors"
 	"fmt"
 	"sort"
 	"sync"
 	"testing"
 	"time"

+	"github.com/m3db/m3/src/dbnode/tracepoint"


import order. interestingly, did metalint not pick this up?

weird - ill look into it

prateek · 2019-04-10T16:32:18Z

src/dbnode/storage/namespace_test.go

@@ -21,12 +21,15 @@
 package storage

 import (
+	stdlib "context"


prateek · 2019-04-10T16:32:21Z

src/dbnode/storage/namespace_test.go

 	"errors"
 	"fmt"
 	"sync"
 	"testing"
 	"time"

+	"github.com/m3db/m3/src/dbnode/tracepoint"


prateek · 2019-04-10T16:33:19Z

src/x/context/context.go

@@ -313,10 +317,10 @@ func (c *ctx) spanIsSampled(sp opentracing.Span) bool {
 	return true
 }

-func (c *ctx) StartTraceSpan(name string) (Context, opentracing.Span, bool) {
+func (c *ctx) StartSampledTraceSpan(name string) (Context, opentracing.Span, bool) {


nit: add comments to exported functions. write a blurb bout using StartSampledTraceSpan v the other one.

oh nvm, saw you have them in the interface def

I thought the convention was to just comment on the interface?

prateek · 2019-04-10T19:20:40Z

src/x/context/types.go

@@ -75,8 +75,12 @@ type Context interface {
 	SetGoContext(stdctx.Context)

 	// StartTraceSpan starts a new span and returns a child ctx
-	// if the span is being sampled.
-	StartTraceSpan(name string) (Context, opentracing.Span, bool)
+	StartTraceSpan(string) (Context, opentracing.Span)


nit: period after comment.

prateek

LGTM

prateek reviewed Mar 28, 2019

View reviewed changes

benraskin92 commented Mar 29, 2019

View reviewed changes

benraskin92 force-pushed the braskin/m3db_jaeger branch from 5cb48e6 to 2af7e0d Compare March 29, 2019 17:32

benraskin92 commented Mar 29, 2019

View reviewed changes

benraskin92 marked this pull request as ready for review March 29, 2019 21:55

benraskin92 changed the title ~~[WIP] Add Jaeger tracing to m3db~~ [m3db] Add Jaeger tracing to m3db Mar 29, 2019

benraskin92 commented Apr 2, 2019

View reviewed changes

richardartoul reviewed Apr 2, 2019

View reviewed changes

robskillington reviewed Apr 2, 2019

View reviewed changes

richardartoul reviewed Apr 2, 2019

View reviewed changes

benraskin92 force-pushed the braskin/m3db_jaeger branch from 6f827ca to 86a9183 Compare April 6, 2019 00:13

benraskin92 force-pushed the braskin/m3db_jaeger branch from 0df089d to 8ca8d2f Compare April 8, 2019 15:22

benraskin92 commented Apr 8, 2019

View reviewed changes

prateek reviewed Apr 8, 2019

View reviewed changes

prateek reviewed Apr 9, 2019

View reviewed changes

robskillington reviewed Apr 9, 2019

View reviewed changes

Benjamin Raskin added 2 commits April 9, 2019 09:44

Update glide

2e4b90b

benraskin92 force-pushed the braskin/m3db_jaeger branch from a8b2713 to 2e4b90b Compare April 9, 2019 14:08

Two start span methods

575fbec

prateek reviewed Apr 10, 2019

View reviewed changes

src/dbnode/storage/database_test.go Outdated

@@ -21,13 +21,16 @@

package storage

import (

stdlib "context"

Copy link

Collaborator

prateek Apr 10, 2019

Choose a reason for hiding this comment

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

same

prateek reviewed Apr 10, 2019

View reviewed changes

src/dbnode/storage/namespace_test.go Outdated

@@ -21,12 +21,15 @@

package storage

import (

stdlib "context"

Copy link

Collaborator

prateek Apr 10, 2019

Choose a reason for hiding this comment

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

here too

prateek reviewed Apr 10, 2019

View reviewed changes

Benjamin Raskin added 2 commits April 10, 2019 15:13

Address nits

6f6b0f7

Remove comments

8a2de85

prateek reviewed Apr 10, 2019

View reviewed changes

Add dots

77b1994

prateek approved these changes Apr 10, 2019

View reviewed changes

Merge branch 'master' into braskin/m3db_jaeger

0eb3104

benraskin92 merged commit 08cbbef into master Apr 10, 2019

justinjc deleted the braskin/m3db_jaeger branch November 27, 2019 20:54

		@@ -0,0 +1,82 @@
		// Copyright (c) 2019 Uber Technologies, Inc.

		@@ -0,0 +1,106 @@
		// Copyright (c) 2019 Uber Technologies, Inc.


		To start Jaeger, add the following to your `m3dbnode.yml` file under `db`:

		```yaml

		docker-compose -f docker-compose.yml up $DOCKER_ARGS m3coordinator01
		# docker-compose -f docker-compose.yml up --build -d --renew-anon-volumes m3db_seed


		ctx = tchannelthrift.Context(tctx)

		ctx, sp, ok = ctx.StartTraceSpan(querySpanID)

[m3db] Add Jaeger tracing to m3db #1506

[m3db] Add Jaeger tracing to m3db #1506

Conversation

benraskin92 commented Mar 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek Mar 28, 2019 • edited Loading

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

codecov bot commented Mar 29, 2019 • edited Loading

Codecov Report

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

robskillington Apr 9, 2019 • edited Loading

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

robskillington commented Apr 6, 2019

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

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

prateek Mar 28, 2019 •

edited

Loading

codecov bot commented Mar 29, 2019 •

edited

Loading

robskillington Apr 9, 2019 •

edited

Loading