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.

Sign up for GitHub

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

Jump to bottom

metrics: add db QPS metric #9151

Merged
merged 19 commits into from
Feb 13, 2019
Merged

metrics: add db QPS metric #9151

merged 19 commits into from
Feb 13, 2019

Conversation

iamzhoug37
Copy link
Contributor

@iamzhoug37 iamzhoug37 commented Jan 22, 2019 •
edited
Loading

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
Copy link

codecov-io commented Jan 22, 2019 •
edited
Loading

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
Copy link
Member

jackysp commented Jan 22, 2019

Great job!

jackysp
jackysp reviewed Jan 24, 2019
View reviewed changes
config/config.go Outdated
@@ -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"`
Copy link
Member

@jackysp jackysp Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Why not define it as a bool?
  2. It is better to move it to

    tidb/config/config.go

    Line 143 in 41838ce

    type Status struct {
  3. How about rename it to RecordQPSbyDB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@winoros
Copy link
Member

winoros commented Jan 24, 2019

@qazbnm456
Please fill the pr template.

@jackysp jackysp added the contribution This PR is from a community contributor. label Jan 24, 2019
@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
jackysp
jackysp reviewed Jan 25, 2019
View reviewed changes
executor/compiler.go Outdated
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

jackysp
jackysp reviewed Feb 12, 2019
View reviewed changes
metrics/executor.go Outdated Show resolved Hide resolved
jackysp
jackysp reviewed Feb 12, 2019
View reviewed changes
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
Copy link
Member

Choose a reason for hiding this comment

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

I think it is not a related change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is go project auto update.

jackysp
jackysp reviewed Feb 12, 2019
View reviewed changes
executor/compiler.go Outdated Show resolved Hide resolved
jackysp
jackysp reviewed Feb 12, 2019
View reviewed changes
executor/compiler.go Outdated Show resolved Hide resolved
jackysp
jackysp reviewed Feb 12, 2019
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

rest LGTM
PTAL @lysu

lysu
lysu reviewed Feb 13, 2019
View reviewed changes
Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

rest LGTM

executor/compiler.go Show resolved Hide resolved
executor/compiler.go Outdated Show resolved Hide resolved
jackysp
jackysp reviewed Feb 13, 2019
View reviewed changes
config/config.go Outdated Show resolved Hide resolved
jackysp
jackysp reviewed Feb 13, 2019
View reviewed changes
config/config.toml.example 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

jackysp
jackysp reviewed Feb 13, 2019
View reviewed changes
config/config.toml.example Outdated Show resolved Hide resolved
lysu
lysu approved these changes Feb 13, 2019
View reviewed changes
Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

@lysu
Copy link
Contributor

lysu commented Feb 13, 2019

/run-all-tests

@lysu lysu added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 13, 2019
@jackysp jackysp merged commit 357f9d7 into pingcap:master Feb 13, 2019
@jackysp
Copy link
Member

jackysp commented Feb 13, 2019

Thanks for your contribution, @iamzhoug37 !

@breezewish breezewish mentioned this pull request May 28, 2020
Closed
@breezewish breezewish mentioned this pull request Jun 4, 2020
Open
@Reminiscent Reminiscent mentioned this pull request Nov 5, 2021
Closed
@srstack srstack mentioned this pull request Feb 17, 2023
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackysp jackysp jackysp left review comments

@lysu lysu lysu approved these changes

Assignees
No one assigned
Labels
component/metrics contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@iamzhoug37 @codecov-io @jackysp @winoros @lysu