New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: begin opentracing from dispatch() and change interface to Execute(ctx, sql) #5027

Merged
merged 4 commits into from Nov 7, 2017

Conversation

Projects
None yet
3 participants
@tiancaiamao
Contributor

tiancaiamao commented Nov 6, 2017

If we start trace from session.Execute(), some background session would make a lot of noise.
server.dispatch should be a better beginning for each trace.

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Nov 7, 2017

Member

LGTM

Member

shenli commented Nov 7, 2017

LGTM

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli
Member

shenli commented Nov 7, 2017

@coocood PTAL

@@ -492,12 +494,15 @@ func (cc *clientConn) addMetrics(cmd byte, startTime time.Time, err error) {
// It also gets a token from server which is used to limit the concurrently handling clients.
// The most frequently used command is ComQuery.
func (cc *clientConn) dispatch(data []byte) error {
span := opentracing.StartSpan("server.dispatch")

This comment has been minimized.

@coocood

coocood Nov 7, 2017

Member

There are commands we don't care which are noises too.
How about tracing only handleQuery and handleStmtExecute?

@coocood

coocood Nov 7, 2017

Member

There are commands we don't care which are noises too.
How about tracing only handleQuery and handleStmtExecute?

This comment has been minimized.

@tiancaiamao

tiancaiamao Nov 7, 2017

Contributor

There won't be any noises currently, because we haven't trace requests other than handleQuery...
Trace dispatch has a potential benefit that we can find whether requests are blocked by rate limiter:

	token := cc.server.getToken()
	defer func() {
		cc.server.releaseToken(token)
		span.Finish()
	}()
@tiancaiamao

tiancaiamao Nov 7, 2017

Contributor

There won't be any noises currently, because we haven't trace requests other than handleQuery...
Trace dispatch has a potential benefit that we can find whether requests are blocked by rate limiter:

	token := cc.server.getToken()
	defer func() {
		cc.server.releaseToken(token)
		span.Finish()
	}()
@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Nov 7, 2017

Contributor

/run-all-tests

Contributor

tiancaiamao commented Nov 7, 2017

/run-all-tests

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Nov 7, 2017

Contributor

/run-integration-test tidb-test=pr/397

Contributor

tiancaiamao commented Nov 7, 2017

/run-integration-test tidb-test=pr/397

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Nov 7, 2017

Contributor

/run-integration-tests tidb-test=pr/397

Contributor

tiancaiamao commented Nov 7, 2017

/run-integration-tests tidb-test=pr/397

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Nov 7, 2017

Contributor

/run-integration-tests tidb-test=pr/397

Contributor

tiancaiamao commented Nov 7, 2017

/run-integration-tests tidb-test=pr/397

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Nov 7, 2017

Contributor

/run-integration-tests tidb-test=pr/397

Contributor

tiancaiamao commented Nov 7, 2017

/run-integration-tests tidb-test=pr/397

@coocood

coocood approved these changes Nov 7, 2017

@tiancaiamao tiancaiamao merged commit 0977fd0 into pingcap:master Nov 7, 2017

8 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 73.591%
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@tiancaiamao tiancaiamao deleted the tiancaiamao:dispatch-context branch Nov 7, 2017

spongedu added a commit to spongedu/tidb that referenced this pull request Nov 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment