util/tracing: adding tracing package #7063

zhexuany · 2018-07-16T10:41:57Z

What have you changed? (mandatory)

adding a tracing package. #7016 is too large, so I split that PR into several PR in hope of speeding up the review process.

What is the type of the changes? (mandatory)

New feature (non-breaking change which adds functionality)

How has this PR been tested? (mandatory)

unit test

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Nope

Does this PR affect tidb-ansible update? (mandatory)

Nope

Does this PR need to be added to the release notes? (mandatory)

gogo protobuf version is updated!

zz-jason · 2018-07-16T10:43:15Z

why vendor is updated?

alivxxx · 2018-07-16T11:50:35Z

Gopkg.toml

@@ -19,6 +19,7 @@ required = ["github.com/golang/protobuf/jsonpb"]
  name = "github.com/coreos/etcd"
  version = "=3.2.18"

+# lock grpc version at 1.7.5 because etcd depdens on this version


Version 1.12.0?

Yup. This should be removed.

zhexuany · 2018-07-16T12:31:50Z

Becuase tracer.go depends on opentracing-go and basicTracer. #7016 is too large hence I split it into small PR which could be easier to review. ;)

zimulala · 2018-07-17T02:13:28Z

util/tracing/tracer_test.go

+	"testing"
+
+	. "github.com/pingcap/check"
+


Why add this blank line?

zimulala · 2018-07-17T02:16:16Z

util/tracing/tracer.go

+	return sp
+}
+
+// ChildSpanFromContxt return a non-nil span. If span can be got from ctx, then returned span is


s/return/returns

Do we need to unify the span? In line 46 and 34, it's Span.

tiancaiamao · 2018-07-17T02:22:42Z

util/tracing/tracer.go

+			return child
+		}
+	}
+	return NoopSpan()


Noop span will not be chained ? If so, we can define a constant to avoid allocation.

Nope. Any operation on noop results in returning itself.

tiancaiamao · 2018-07-17T02:23:59Z

util/tracing/tracer_test.go

+	"testing"
+
+	. "github.com/pingcap/check"
+


remove empty line

tiancaiamao · 2018-07-17T02:26:18Z

util/tracing/tracer_test.go

+	TestingT(t)
+}
+
+type testTraceSuite struct {


Just cover the noop span case?

Friendly ping

tiancaiamao · 2018-07-17T02:29:03Z

Gopkg.lock

@@ -86,6 +86,12 @@
  packages = ["."]
  revision = "3955978caca48c1658a4bb7a9c6a0f084e326af3"

+[[projects]]


We have to make sure other project doesn't use this, or use the same version of this package, or at least compatible to this package version.

In general, you should not review changes made in Gopkg.lock because such file is generated by dep.

lysu · 2018-07-17T03:23:33Z

util/tracing/tracer.go

+// otherwise, the created Span is a child.
+func NewSnowball(opName string, callback func(sp basictracer.RawSpan)) opentracing.Span {
+	tr := basictracer.New(CallbackRecorder(callback))
+	opentracing.SetGlobalTracer(tr)


It seems NewSnowBall() will be called in TraceExecutor.Open which will be called per statement.

So If two people do trace command at the same time via two connections, this global variable will be a race condition and be modified in two different goroutines?

It is by design. Only TracExec calls NewSnowBall(). It will be evolving quickly, so I prefer to keeping it like this.

plz explain more about the correctness of this by design~~~?

multi goroutines(os threads underneath) write/read same gobal variable seems be a well-known bug. (ref: https://golang.org/ref/mem#tmp_1).

I still can not understand how can u got right value without any memory corruption in multi trace stmt exec.

By design means there will be only one TracExec running. In general, this feature is executed from MySQL-shell and usually done by ops engineer or client. We just need to collect useful information.

coocood · 2018-07-17T05:09:56Z

@zhexuany please only update the vendor first.

XuHuaiyu · 2018-07-18T07:21:51Z

util/tracing/tracer.go

+}
+
+// NewSnowball returns a span which records directly via the specified
+// callback. If the given WireSpan is the zero value, a new trace is created;


I do not see any WireSpan?

XuHuaiyu · 2018-07-18T07:22:09Z

util/tracing/tracer.go

+// NewSnowball returns a span which records directly via the specified
+// callback. If the given WireSpan is the zero value, a new trace is created;
+// otherwise, the created Span is a child.
+func NewSnowball(opName string, callback func(sp basictracer.RawSpan)) opentracing.Span {


add comments for the input parameters.

XuHuaiyu · 2018-07-18T07:25:28Z

util/tracing/tracer.go

+}
+
+// NoopSpan returns a span which discards all operations.
+func NoopSpan() opentracing.Span {


We do not need to make this an exported function?

Yes. Good suggestion~

So any change here?

XuHuaiyu · 2018-07-18T07:26:52Z

util/tracing/tracer.go

+// ChildSpanFromContxt returns a non-nil span. If span can be got from ctx, then returned span is
+// a child of such span. Otherwise, returned span is a noop span.
+func ChildSpanFromContxt(ctx context.Context, opName string) (sp opentracing.Span) {
+	if sp := opentracing.SpanFromContext(ctx); sp != nil {


It seems that SpanFromContext will not return nil.

It will if there is no span in Context

if sp = opentracing.SpanFromContext(ctx); sp == nil { return NoopSpan() }

But it will not return nil?

tiancaiamao · 2018-07-19T03:19:25Z

util/tracing/tracer.go

+}
+
+// NoopSpan returns a span which discards all operations.
+func NoopSpan() opentracing.Span {


So any change here?

tiancaiamao · 2018-07-19T03:20:57Z

util/tracing/tracer.go

+// callback.
+func NewSnowball(opName string, callback func(sp basictracer.RawSpan)) opentracing.Span {
+	tr := basictracer.New(CallbackRecorder(callback))
+	opentracing.SetGlobalTracer(tr)


When will we reset the global tracer? or once NewSnowball function is called, we'll always have the tracing on?

tiancaiamao · 2018-07-19T03:22:36Z

util/tracing/tracer.go

+package tracing
+
+import (
+	"context"


You can probably use this one:

"golang.org/x/net/context"

Or, change all the repo to use

"context"

Yeah, "golang.org/x/net/context" is definitely correct choice.

zhexuany · 2018-07-19T15:37:03Z

Closing this PR since changes are already small and can be reviewed in original PR.

zhexuany requested review from zz-jason and alivxxx July 16, 2018 10:42

alivxxx reviewed Jul 16, 2018

View reviewed changes

zhexuany changed the title ~~adding tracing package~~ util/tracing: adding tracing package Jul 16, 2018

zhexuany force-pushed the adding_tracing_package branch 3 times, most recently from 9a1982f to cd88645 Compare July 16, 2018 12:59

coocood added the status/DNM label Jul 16, 2018

zhexuany force-pushed the adding_tracing_package branch from 0e734be to 8747b3c Compare July 16, 2018 13:08

coocood removed the status/DNM label Jul 16, 2018

zhexuany mentioned this pull request Jul 16, 2018

*: tidb tracing prototype #7016

Merged

3 tasks

zimulala reviewed Jul 17, 2018

View reviewed changes

tiancaiamao reviewed Jul 17, 2018

View reviewed changes

lysu reviewed Jul 17, 2018

View reviewed changes

zhexuany force-pushed the adding_tracing_package branch from a832e5a to f2a068c Compare July 17, 2018 07:52

zhexuany mentioned this pull request Jul 17, 2018

vendor:update vendor for tracing #7067

Merged

zhexuany added 2 commits July 17, 2018 18:49

adding tracing package

559fa2b

address reviewer's comment

615e243

zhexuany force-pushed the adding_tracing_package branch from f2a068c to 615e243 Compare July 17, 2018 10:49

XuHuaiyu reviewed Jul 18, 2018

View reviewed changes

more work on tracing

4527131

tiancaiamao reviewed Jul 19, 2018

View reviewed changes

zhexuany closed this Jul 19, 2018

zhexuany deleted the adding_tracing_package branch July 19, 2018 15:37

zhexuany added this to In Progress in tracing system for tidb Aug 15, 2018

zz-jason moved this from In Progress to Done in tracing system for tidb Mar 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util/tracing: adding tracing package #7063

util/tracing: adding tracing package #7063

zhexuany commented Jul 16, 2018 •

edited by tiancaiamao

zz-jason commented Jul 16, 2018

alivxxx Jul 16, 2018

zhexuany Jul 16, 2018

zhexuany commented Jul 16, 2018

zimulala Jul 17, 2018

zimulala Jul 17, 2018

zimulala Jul 17, 2018

tiancaiamao Jul 17, 2018 •

edited

zhexuany Jul 17, 2018

tiancaiamao Jul 17, 2018

tiancaiamao Jul 17, 2018

tiancaiamao Jul 17, 2018 •

edited

tiancaiamao Jul 17, 2018

zhexuany Jul 17, 2018

lysu Jul 17, 2018 •

edited

zhexuany Jul 17, 2018

lysu Jul 17, 2018

zhexuany Jul 17, 2018

lysu Jul 17, 2018

coocood commented Jul 17, 2018

XuHuaiyu Jul 18, 2018

XuHuaiyu Jul 18, 2018

XuHuaiyu Jul 18, 2018

zhexuany Jul 18, 2018

tiancaiamao Jul 19, 2018

XuHuaiyu Jul 18, 2018

zhexuany Jul 18, 2018

XuHuaiyu Jul 19, 2018

tiancaiamao Jul 19, 2018

tiancaiamao Jul 19, 2018

tiancaiamao Jul 19, 2018

zhexuany Jul 19, 2018

zhexuany commented Jul 19, 2018

util/tracing: adding tracing package #7063

util/tracing: adding tracing package #7063

Conversation

zhexuany commented Jul 16, 2018 • edited by tiancaiamao

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

zz-jason commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhexuany commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao Jul 17, 2018 • edited

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

tiancaiamao Jul 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu Jul 17, 2018 • edited

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

coocood commented Jul 17, 2018

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

zhexuany commented Jul 19, 2018

zhexuany commented Jul 16, 2018 •

edited by tiancaiamao

tiancaiamao Jul 17, 2018 •

edited

tiancaiamao Jul 17, 2018 •

edited

lysu Jul 17, 2018 •

edited