Skip to content
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

metrics: add db QPS metric #9151

Merged
merged 19 commits into from Feb 13, 2019

Conversation

@iamzhoug37
Copy link
Contributor

commented Jan 22, 2019

What is changed and how it works?

  1. Add a new metric with DB、type label.
  2. Add new configuration "tidb_enable_db_qps_metric" to control whether to open the metric.
    • the configuration can be configured in the tidb configuration file, and load once when the tidb startup
    • support session scope close or open the metric

metric result like this:
image

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to update the tidb-ansible repository
  • Need to be included in the release note
@codecov-io

This comment has been minimized.

Copy link

commented Jan 22, 2019

Codecov Report

Merging #9151 into master will decrease coverage by 0.05%.
The diff coverage is 5.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9151      +/-   ##
==========================================
- Coverage    67.2%   67.14%   -0.06%     
==========================================
  Files         371      371              
  Lines       77353    77427      +74     
==========================================
+ Hits        51985    51991       +6     
- Misses      20720    20791      +71     
+ Partials     4648     4645       -3
Impacted Files Coverage Δ
config/config.go 14.54% <ø> (ø) ⬆️
metrics/metrics.go 0% <0%> (ø) ⬆️
executor/compiler.go 63.36% <5.4%> (-33.54%) ⬇️
executor/join.go 78.38% <0%> (ø) ⬆️
executor/executor.go 67.03% <0%> (+0.13%) ⬆️
store/tikv/lock_resolver.go 42.65% <0%> (+0.94%) ⬆️

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 18ff4f9...bc6cf35. Read the comment docs.

@jackysp

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Great job!

@@ -61,7 +61,8 @@ type Config struct {
TxnLocalLatches TxnLocalLatches `toml:"txn-local-latches" json:"txn-local-latches"`
// Set sys variable lower-case-table-names, ref: https://dev.mysql.com/doc/refman/5.7/en/identifier-case-sensitivity.html.
// TODO: We actually only support mode 2, which keeps the original case, but the comparison is case-insensitive.
LowerCaseTableNames int `toml:"lower-case-table-names" json:"lower-case-table-names"`
LowerCaseTableNames int `toml:"lower-case-table-names" json:"lower-case-table-names"`
DbQPSMetricSwitch int64 `toml:"db-qps-metric-switch" json:"db-qps-metric-switch"`

This comment has been minimized.

Copy link
@jackysp

jackysp Jan 24, 2019

Member
  1. Why not define it as a bool?
  2. It is better to move it to
    type Status struct {
  3. How about rename it to RecordQPSbyDB

This comment has been minimized.

Copy link
@iamzhoug37

iamzhoug37 Jan 28, 2019

Author Contributor

ok

@winoros

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

@qazbnm456
Please fill the pr template.

@jackysp jackysp changed the title tidb add db QPS metric metrics: tidb add db QPS metric Jan 24, 2019

@jackysp jackysp changed the title metrics: tidb add db QPS metric metrics: add db QPS metric Jan 24, 2019

@@ -125,6 +126,120 @@ func CountStmtNode(stmtNode ast.StmtNode, inRestrictedSQL bool) {
metrics.StmtNodeCounter.WithLabelValues(GetStmtLabel(stmtNode)).Inc()
}

// DbCountStmtNode records the number of statements with the same type and db.
func DbCountStmtNode(stmtNode ast.StmtNode, inRestrictedSQL bool) {

This comment has been minimized.

Copy link
@jackysp

jackysp Jan 25, 2019

Member

I think it is better to refactor CountStmtNode instead of writing a new function.

This comment has been minimized.

Copy link
@iamzhoug37

iamzhoug37 Jan 28, 2019

Author Contributor

ok

Show resolved Hide resolved sessionctx/variable/sysvar.go Outdated

iamzhoug37 added some commits Jan 28, 2019

Show resolved Hide resolved metrics/executor.go Outdated
go.mod Outdated
@@ -84,7 +84,6 @@ require (
google.golang.org/genproto v0.0.0-20190108161440-ae2f86662275 // indirect
google.golang.org/grpc v1.17.0
gopkg.in/natefinch/lumberjack.v2 v2.0.0
gopkg.in/stretchr/testify.v1 v1.2.2 // indirect

This comment has been minimized.

Copy link
@jackysp

jackysp Feb 12, 2019

Member

I think it is not a related change.

This comment has been minimized.

Copy link
@iamzhoug37

iamzhoug37 Feb 12, 2019

Author Contributor

This is go project auto update.

Show resolved Hide resolved executor/compiler.go Outdated
Show resolved Hide resolved executor/compiler.go Outdated
@jackysp
Copy link
Member

left a comment

rest LGTM
PTAL @lysu

@lysu
Copy link
Member

left a comment

rest LGTM

Show resolved Hide resolved executor/compiler.go
Show resolved Hide resolved executor/compiler.go Outdated
Show resolved Hide resolved config/config.go Outdated
@@ -119,6 +119,9 @@ metrics-addr = ""
# Prometheus client push interval in second, set \"0\" to disable prometheus push.
metrics-interval = 15

# if enable tidb record db qps
record-db-qps = true

This comment has been minimized.

Copy link
@jackysp

jackysp Feb 13, 2019

Member

It is better to set it to false as default. Because it may make the server starting failure.

This comment has been minimized.

Copy link
@iamzhoug37

iamzhoug37 Feb 13, 2019

Author Contributor

OK

This comment has been minimized.

Copy link
@iamzhoug37

iamzhoug37 Feb 13, 2019

Author Contributor

ok

Show resolved Hide resolved config/config.toml.example Outdated
@lysu

lysu approved these changes Feb 13, 2019

Copy link
Member

left a comment

LGTM

@lysu

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

/run-all-tests

@lysu lysu added the status/LGT2 label Feb 13, 2019

@jackysp jackysp merged commit 357f9d7 into pingcap:master Feb 13, 2019

13 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@jackysp

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Thanks for your contribution, @iamzhoug37 !

yu34po added a commit to yu34po/tidb that referenced this pull request Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.