-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
metric: replace QueryDurationHistogram's "general" type to more detail stmt type #8819
Conversation
/run-all-tests |
server/conn.go
Outdated
cc.ctx.SetProcessInfo("", t, mysql.ComSleep) | ||
stmtType := cc.ctx.ShowProcess().StmtType | ||
if stmtType != "" { | ||
metrics.StmtDurationHistogram.WithLabelValues(stmtType).Observe(time.Since(t).Seconds()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we handle the metrics at the same place as QueryDurationHistogram?
/run-unit-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: lysu <sulifx@gmail.com>
Signed-off-by: lysu <sulifx@gmail.com>
Signed-off-by: lysu <sulifx@gmail.com>
if stmtType != "" { | ||
sqlType = stmtType | ||
} | ||
metrics.QueryDurationHistogram.WithLabelValues(sqlType).Observe(time.Since(startTime).Seconds()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do QueryDurationHistogram
and QueryTotalCounter
use the different label
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a "historical reasons", old code choose COMMAND as count label and none label for the duration,
To add duration label, IMO stmtType(like insert/select) is more useful than command(like COM_QUERY).
why don't change QueryTotalCounter is maybe sometime we need that, and we also have another StmtNodeCounter
to cover "count by stmtType" although has some litte different.
so after this we can see:
- count of command -> QueryTotalCounter
- count of stmt -> StmtNodeCounter
- duration of query -> QueryDurationHistogram and optional switch to stmtType view
:D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
1 similar comment
/run-all-tests |
/rebuild |
/run-all-tests |
/run-unit-test |
/run-unit-test |
What problem does this PR solve?
current Prometheus only display QueryDurationHistogram with internal and general, for general we often need more detail about which kind of SQL become slow.
What is changed and how it works?
replace QueryDurationHistogram's "general" type to more detail stmt type
why choose to replace general? add new dimensions will make time serial x2 per host, +n per host
corner case:
insert into tbl select a, b from tbl2
will be recorded as root stmt type ----Insert
sql1;sql2
will use last stmt's typeCheck List
Tests
Code changes
Side effects
Related changes
This change is