metrics: add region request metrics which record caller info #9117

okJiang · 2025-03-05T10:02:49Z

What problem does this PR solve?

Issue Number: Ref #8593

What is changed and how does it work?

Check List

Tests

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

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

Signed-off-by: okJiang <[email protected]>

ti-chi-bot · 2025-03-05T10:02:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign binshi-bing for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2025-03-05T10:11:03Z

Codecov Report

Attention: Patch coverage is 65.95745% with 16 lines in your changes missing coverage. Please review.

Project coverage is 75.73%. Comparing base (3b505f5) to head (66e8344).
Report is 19 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9117      +/-   ##
==========================================
- Coverage   76.36%   75.73%   -0.64%     
==========================================
  Files         473      477       +4     
  Lines       71663    73269    +1606     
==========================================
+ Hits        54728    55487     +759     
- Misses      13514    14308     +794     
- Partials     3421     3474      +53

Flag	Coverage Δ
unittests	`75.73% <65.95%> (∅)`

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

rleungx · 2025-03-05T10:20:29Z

server/metrics.go

+			Subsystem: "server",
+			Name:      "region_request_cnt",
+			Help:      "Counter of region request.",
+		}, []string{"request", "caller_id", "caller_component", "err_msg"})


Can we make it more simple, e.g., request-component-module?

ScanRegions-tidb-server->tidb-tikv-driver is kind of confusing and not readable.

request-component-module LGTM

However, there are caller_id and caller_component in the current code. If we use request-component-module here, it will cause confusion with the term component.

ScanRegions-tidb-server->tidb-tikv-driver is kind of confusing and not readable.

It is indeed easy to get confused, and I am still thinking of a better format. Since we chose to use a hyphen as a connector in the component, continuing to use a hyphen in Grafana might look a bit strange.

Maybe we should name the component using camel case? Such as RegionCache

What do you think? cc @lhy1024 @JmPotato @bufferflies

Signed-off-by: okJiang <[email protected]>

okJiang · 2025-03-06T05:32:00Z

client/pkg/caller/caller.go

+	// client-go component
+	CodecPDClient       Component = "codec-pd-client"
+	RegionCache         Component = "region-cache"
+	StoreCache          Component = "store-cache"
+	Oracles             Component = "oracle"
+	Rawkv               Component = "rawkv"
+	KvStore             Component = "kv-store"
+	InterceptedPDClient Component = "intercepted-pd-client"
+
+	// TiDB component
+	Pitr             Component = "pitr"
+	Ddl              Component = "ddl"
+	ImportInto       Component = "import-into"
+	TikvHandler      Component = "tikv-handler"
+	GcWorker         Component = "gc-worker"
+	GcJob            Component = "gc-job"
+	DistributedGcJob Component = "distributed-gc-job"


These names can be reviewed with these two PR https://github.com/pingcap/tidb/pull/59911/files and tikv/client-go#1516

bufferflies · 2025-03-07T06:56:40Z

client/pkg/caller/caller.go

+	CodecPDClient       Component = "codec-pd-client"
+	RegionCache         Component = "region-cache"
+	StoreCache          Component = "store-cache"
+	Oracles             Component = "oracle"


Suggested change

Oracles Component = "oracle"

Oracle Component = "oracle"

where is it used?

It is used in other repo https://github.com/tikv/pd/pull/9117/files/ebd66e5b0042353ad6402f6239ab049a438c21a0#r1982716822

Did I misunderstand your comment? I thought what you needed was the current PR like. tikv/client-go#1516 (comment)

bufferflies · 2025-03-07T06:59:18Z

server/grpc_service.go

@@ -1440,6 +1443,9 @@ func (s *GrpcServer) GetPrevRegion(ctx context.Context, request *pdpb.GetRegionR
 		return rsp.(*pdpb.GetRegionResponse), err
 	}

+	defer func() {


Can we extract one function for this? and get the call name by another way.

Isn't there only one function right now?

Signed-off-by: okJiang <[email protected]>

bufferflies · 2025-03-13T10:06:08Z

server/metrics.go

+	prometheus.MustRegister(regionRequestCounter)
+}
+
+func incRegionRequestCounter(method string, header *pdpb.RequestHeader, err *pdpb.Error) {


Hash function is little heavy, and we don't need to report all the request just see the the source. So we can use sample to report the normal request to decrease the usage of hash function.

This can indeed lighten the load; for example, we can collect metrics with a probability of one percent. Even with tens of thousands of requests, we can still identify the sources of these large requests.

If there are no objections from others, I will make the changes today.

Done~ 7c00c9e

Signed-off-by: okJiang <[email protected]>

JmPotato · 2025-03-28T08:41:35Z

client/pkg/caller/caller.go

@@ -28,11 +28,31 @@ type (
 	Component string
 )

+// nolint: exported


Is this necessary?

Yes, otherwise it would require a comment for each variable

okJiang · 2025-04-01T08:58:27Z

ping @bufferflies @JmPotato @rleungx

bufferflies · 2025-04-01T10:27:53Z

server/metrics.go

+	if callerComponent == "" {
+		callerComponent = "unknown"
+	}
+	regionRequestCounter.WithLabelValues(method, callerID, callerComponent, errMsg).Inc()


if the error is nil, the inc one is wrong; it should multi * 100.

I separated successful and failed requests, marking a 1% sampling rate for successful requests.

bufferflies

rest lgtm

JmPotato · 2025-04-02T02:30:21Z

server/metrics.go

+	if err == nil && rand.Intn(100) != 0 {
+		// sample 1% region requests to avoid high cardinality
+		return
+	}


Instead of sampling with a specific probability, can we record the data into a counter and periodically flush it to Prometheus?

Recording to the counter also requires calculation via a hash map, and we need to ensure it's thread-safe, which likewise introduces considerable overhead.

Signed-off-by: okJiang <[email protected]>

add caller metrics

f1aa468

Signed-off-by: okJiang <[email protected]>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. labels Mar 5, 2025

ti-chi-bot bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 5, 2025

rleungx reviewed Mar 5, 2025

View reviewed changes

Add Component Name

ebd66e5

Signed-off-by: okJiang <[email protected]>

okJiang commented Mar 6, 2025

View reviewed changes

okJiang changed the title ~~client: add region request metrics which record caller info~~ service: add region request metrics which record caller info Mar 7, 2025

okJiang changed the title ~~service: add region request metrics which record caller info~~ metrics: add region request metrics which record caller info Mar 7, 2025

bufferflies reviewed Mar 7, 2025

View reviewed changes

fix lint

d943e9e

Signed-off-by: okJiang <[email protected]>

bufferflies reviewed Mar 13, 2025

View reviewed changes

okJiang added 3 commits March 27, 2025 18:10

fix comment

7c00c9e

Signed-off-by: okJiang <[email protected]>

fix comment

ac5d764

Signed-off-by: okJiang <[email protected]>

update grafana

66e8344

Signed-off-by: okJiang <[email protected]>

JmPotato reviewed Mar 28, 2025

View reviewed changes

bufferflies reviewed Apr 1, 2025

View reviewed changes

JmPotato reviewed Apr 2, 2025

View reviewed changes

rename panel name

1f027c0

Signed-off-by: okJiang <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metrics: add region request metrics which record caller info #9117

metrics: add region request metrics which record caller info #9117

okJiang commented Mar 5, 2025 •

edited

Loading

ti-chi-bot bot commented Mar 5, 2025

codecov bot commented Mar 5, 2025 •

edited

Loading

rleungx Mar 5, 2025 •

edited

Loading

rleungx Mar 5, 2025

okJiang Mar 6, 2025

okJiang Mar 6, 2025

okJiang Mar 6, 2025 •

edited

Loading

bufferflies Mar 7, 2025

bufferflies Mar 7, 2025

okJiang Mar 7, 2025 •

edited

Loading

bufferflies Mar 7, 2025

okJiang Mar 7, 2025

bufferflies Mar 13, 2025

okJiang Mar 14, 2025

okJiang Mar 27, 2025

JmPotato Mar 28, 2025

okJiang Mar 28, 2025

okJiang commented Apr 1, 2025

bufferflies Apr 1, 2025

okJiang Apr 2, 2025

bufferflies left a comment

JmPotato Apr 2, 2025

okJiang Apr 2, 2025

metrics: add region request metrics which record caller info #9117

Are you sure you want to change the base?

metrics: add region request metrics which record caller info #9117

Conversation

okJiang commented Mar 5, 2025 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Mar 5, 2025

codecov bot commented Mar 5, 2025 • edited Loading

Codecov Report

rleungx Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang commented Apr 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bufferflies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang commented Mar 5, 2025 •

edited

Loading

codecov bot commented Mar 5, 2025 •

edited

Loading

rleungx Mar 5, 2025 •

edited

Loading

okJiang Mar 6, 2025 •

edited

Loading

okJiang Mar 7, 2025 •

edited

Loading