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 metrics for bind info #10921

Merged
merged 9 commits into from
Jul 26, 2019
Merged

metrics: add metrics for bind info #10921

merged 9 commits into from
Jul 26, 2019

Conversation

alivxxx
Copy link
Contributor

@alivxxx alivxxx commented Jun 24, 2019

What problem does this PR solve?

Add metrics for bind info.

What is changed and how it works?

It adds metrics for

  • Number of queries that use sql bind
  • The current number of sql bind
  • The memory usage of sql bind.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

@alivxxx alivxxx added type/enhancement The issue or PR belongs to an enhancement. component/metrics labels Jun 24, 2019
@alivxxx alivxxx requested review from zz-jason, qw4990 and XuHuaiyu June 24, 2019 06:57
@codecov
Copy link

codecov bot commented Jun 24, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@d135ff1). Click here to learn what that means.
The diff coverage is 85.2941%.

@@            Coverage Diff             @@
##             master    #10921   +/-   ##
==========================================
  Coverage          ?   81.257%           
==========================================
  Files             ?       423           
  Lines             ?     90082           
  Branches          ?         0           
==========================================
  Hits              ?     73198           
  Misses            ?     11583           
  Partials          ?      5301

qw4990
qw4990 reviewed Jul 2, 2019
View reviewed changes
bindinfo/session_handle.go Outdated Show resolved Hide resolved
qw4990
qw4990 reviewed Jul 2, 2019
View reviewed changes
bindinfo/session_handle.go Outdated Show resolved Hide resolved
zz-jason
zz-jason reviewed Jul 8, 2019
View reviewed changes
bindinfo/cache.go Outdated Show resolved Hide resolved
@alivxxx alivxxx requested review from zz-jason, qw4990 and eurekaka July 12, 2019 08:15
zz-jason
zz-jason reviewed Jul 16, 2019
View reviewed changes
bindinfo/handle.go Outdated
if meta.Status == Using {
newCache[hash] = append(newCache[hash], meta)
metrics.BindMemoryUsage.WithLabelValues(metrics.ScopeGlobal).Add(meta.size())
Copy link
Member

Choose a reason for hiding this comment

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

It's better to add a comment to explain why we don't decrease the memory size for the old data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is decreased in removeStaleBindMetas, this line directly manipulates the cache, so it is handled here.

Copy link
Member

Choose a reason for hiding this comment

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

should we also increase BindTotalGauge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think no, or if we have multiply tidb, each of them will increase one for the same bindings, it will look like there are many bindings.

bindinfo/handle.go Outdated
for i := len(metas) - 1; i >= 0; i-- {
if metas[i].isStale(meta) {
metrics.BindMemoryUsage.WithLabelValues(scope).Sub(metas[i].size())
if metas[i].Status == Using {
Copy link
Member

Choose a reason for hiding this comment

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

should we decrease the counter no matter whether it is Using?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, currently it only means valid binds, and for global handles, it could only be using.

eurekaka
eurekaka reviewed Jul 23, 2019
View reviewed changes
metrics/bindinfo.go Outdated
Subsystem: "bindinfo",
Name: "bind_total_gauge",
Help: "Total number of sql bind",
}, []string{LblType})
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this value be retrieved directly from system tables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it can, but the system table only contains global level bindings.

eurekaka
eurekaka reviewed Jul 23, 2019
View reviewed changes
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@eurekaka eurekaka added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 23, 2019
@alivxxx alivxxx requested a review from zz-jason July 23, 2019 09:14
@alivxxx
Copy link
Contributor Author

alivxxx commented Jul 25, 2019

PTAL @zz-jason

zz-jason
zz-jason reviewed Jul 25, 2019
View reviewed changes
session/session.go Show resolved Hide resolved
bindinfo/handle.go Outdated
if meta.Status == Using {
newCache[hash] = append(newCache[hash], meta)
metrics.BindMemoryUsage.WithLabelValues(metrics.ScopeGlobal).Add(meta.size())
Copy link
Member

Choose a reason for hiding this comment

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

should we also increase BindTotalGauge?

zz-jason
zz-jason reviewed Jul 26, 2019
View reviewed changes
bindinfo/handle.go Outdated
@@ -255,6 +257,7 @@ func (h *BindHandle) DropInvalidBindRecord() {
if time.Since(invalidBindRecord.droppedTime) > 6*time.Second {
delete(invalidBindRecordMap, key)
}
invalidBindRecord.bindRecord.updateMetrics(metrics.ScopeGlobal, false)
Copy link
Member

Choose a reason for hiding this comment

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

should we only update the metrics when the delete operation is performed?

@qw4990 qw4990 removed their request for review July 26, 2019 05:05
zz-jason
zz-jason approved these changes Jul 26, 2019
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 26, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Jul 26, 2019

/run-all-tests

@sre-bot sre-bot merged commit e09745b into pingcap:master Jul 26, 2019
@sre-bot sre-bot mentioned this pull request Jul 26, 2019
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Jul 26, 2019

cherry pick to release-3.0 in PR #11467

@alivxxx alivxxx deleted the bind-metrics branch July 29, 2019 02:59
zz-jason pushed a commit that referenced this pull request Jul 29, 2019
@sre-bot @zz-jason
metrics: add metrics for bind info (#10921) (#11467)
58aae12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eurekaka eurekaka eurekaka left review comments

@qw4990 qw4990 qw4990 left review comments

@zz-jason zz-jason zz-jason approved these changes

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

Assignees
No one assigned
Labels
component/metrics status/can-merge Indicates a PR has been approved by a committer. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alivxxx @sre-bot @zz-jason @eurekaka @qw4990