Stats inner/inter zone network traffic for mpp tasks by yibin87 · Pull Request #9747 · pingcap/tiflash

yibin87 · 2024-12-27T03:06:59Z

What problem does this PR solve?

Issue Number: close #9748

Problem Summary:

What is changed and how it works?

Inner/inter zone network traffic is priced differently in cloud, thus trace each mpp query's such info will help users to locate the most network-expensive query.
This PR stats thress kinds of network traffic:

Exchange sender
Exchange receiver
Remote table reader using coprocessor request

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
Currently, set same and different zone labels for tiflash proxy config, and hack DAGStorageInterpreter to always use remote coprocessor read. Then check the MPPTaskStatistics.cpp output to see if local/inner/inter zone label is set correctly.
When tidb side change is merged, will check slow log and statements summary.
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

yibin87 · 2024-12-27T03:17:04Z

/hold

yibin87 · 2025-01-02T02:24:06Z

/cc @SeaRise @JinheLin @windtalker

ti-chi-bot · 2025-01-02T02:24:10Z

@yibin87: GitHub didn't allow me to request PR reviews from the following users: SeaRise.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @SeaRise @JinheLin @windtalker

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp

yibin87 · 2025-01-03T03:43:38Z

/unhold

yibin87 · 2025-01-03T04:59:13Z

/cc @xzhangxian1008

xzhangxian1008

Other LGTM

xzhangxian1008 · 2025-01-06T02:26:52Z

dbms/src/Flash/Statistics/BaseRuntimeStatistics.cpp

+        inter_zone_receive_bytes += bytes;
+        break;
+    default:
+        break;


Maybe we can throw exception if encountering an unknown type?

Ok, updated.

xzhangxian1008 · 2025-01-06T02:27:01Z

dbms/src/Flash/Statistics/BaseRuntimeStatistics.cpp

+        inter_zone_send_bytes += bytes;
+        break;
+    default:
+        break;


Ok, updated

Signed-off-by: yibin <huyibin@pingcap.com>

windtalker · 2025-01-06T10:12:12Z

dbms/src/Flash/Coprocessor/CoprocessorReader.h

    }

-    static size_t getSourceNum() { return 1; }
+    static size_t getSourceNum() { return 2; }


I‘m not sure if the change will bring some side affects

Checked that it's only used in CoprocessorReaderSourceOp as the IOProfile's connection info num.

windtalker · 2025-01-06T11:49:32Z

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp

-
+    String store_zone_label;
+    auto kv_store = tmt.getKVStore();
+    if likely (kv_store)


does this work for the compute node in disagg-arch?

No, for disagg-arch, compute node, kv_store is nullptr.

windtalker · 2025-01-07T01:17:31Z

dbms/src/Flash/Statistics/TableScanImpl.cpp

+}
+String LocalTableScanDetail::toJson() const
+{
+    return fmt::format(R"({{"is_local":false,"bytes":{},{}}})", bytes, time_detail.toJson());


why is_local is false for LocalTableScanDetail?

Good catch, updated.

Signed-off-by: yibin <huyibin@pingcap.com>

windtalker

lgtm

ti-chi-bot · 2025-01-07T01:59:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: windtalker, xzhangxian1008

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [windtalker]

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

ti-chi-bot · 2025-01-07T01:59:48Z

[LGTM Timeline notifier]

Timeline:

2025-01-06 02:56:50.626658828 +0000 UTC m=+149553.915490532: ☑️ agreed by xzhangxian1008.
2025-01-07 01:59:47.836122279 +0000 UTC m=+232531.124953983: ☑️ agreed by windtalker.

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/needs-linked-issue labels Dec 27, 2024

ti-chi-bot bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 27, 2024

ti-chi-bot bot requested review from JinheLin and windtalker January 2, 2025 02:24

JinheLin reviewed Jan 2, 2025

View reviewed changes

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp Show resolved Hide resolved

yibin87 requested a review from JinheLin January 2, 2025 09:53

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 3, 2025

yibin87 force-pushed the network_stats branch from 46b7987 to dc13838 Compare January 3, 2025 03:51

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 3, 2025

yibin87 force-pushed the network_stats branch 2 times, most recently from 5b96f79 to e1de416 Compare January 3, 2025 03:59

ti-chi-bot bot requested a review from xzhangxian1008 January 3, 2025 04:59

xzhangxian1008 approved these changes Jan 6, 2025

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jan 6, 2025

yibin87 added 3 commits January 6, 2025 17:37

update tipb protocol

331a274

Signed-off-by: yibin <huyibin@pingcap.com>

update tipb code

944d2cf

Signed-off-by: yibin <huyibin@pingcap.com>

update tipb commit

6c836bc

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 added 16 commits January 6, 2025 17:37

Stats exchange sender network traffic

a2bd6a3

Signed-off-by: yibin <huyibin@pingcap.com>

Save work

17067f5

Signed-off-by: yibin <huyibin@pingcap.com>

Handle exchange receiver network traffic

39bd95c

Signed-off-by: yibin <huyibin@pingcap.com>

Little refact

6ed8c92

Signed-off-by: yibin <huyibin@pingcap.com>

Little refact

0c11996

Signed-off-by: yibin <huyibin@pingcap.com>

Add coprocessor remote read connection network stats

9930677

Signed-off-by: yibin <huyibin@pingcap.com>

Get store zone label using basic way

111b950

Signed-off-by: yibin <huyibin@pingcap.com>

Save work

0996b36

Signed-off-by: yibin <huyibin@pingcap.com>

refine tableScan statistics

c0cd583

Signed-off-by: yibin <huyibin@pingcap.com>

Little refact

1f77702

Signed-off-by: yibin <huyibin@pingcap.com>

Fix incorrect update issue

cc108c9

Signed-off-by: yibin <huyibin@pingcap.com>

Little fix and refact

10c14e1

Signed-off-by: yibin <huyibin@pingcap.com>

Address comments

fb6d9a7

Signed-off-by: yibin <huyibin@pingcap.com>

Update client-c version

bf54022

Signed-off-by: yibin <huyibin@pingcap.com>

Address comments

4bfcaf6

Signed-off-by: yibin <huyibin@pingcap.com>

Little update

5254695

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 force-pushed the network_stats branch from 4e7ea8a to 5254695 Compare January 6, 2025 09:38

windtalker reviewed Jan 7, 2025

View reviewed changes

yibin87 requested a review from windtalker January 7, 2025 01:43

address comments

9d521d3

Signed-off-by: yibin <huyibin@pingcap.com>

windtalker approved these changes Jan 7, 2025

View reviewed changes

ti-chi-bot bot added the lgtm label Jan 7, 2025

ti-chi-bot bot added approved and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 7, 2025

ti-chi-bot bot merged commit 330a709 into pingcap:master Jan 7, 2025

JaySon-Huang mentioned this pull request Mar 26, 2025

*: Fix invalid json in ExchangeReceiveDetail #10040

Merged

12 tasks

Conversation

yibin87 commented Dec 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

yibin87 commented Dec 27, 2024

Uh oh!

yibin87 commented Jan 2, 2025

Uh oh!

ti-chi-bot bot commented Jan 2, 2025

Uh oh!

Uh oh!

yibin87 commented Jan 3, 2025

Uh oh!

yibin87 commented Jan 3, 2025

Uh oh!

xzhangxian1008 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yibin87 Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Jan 7, 2025

Uh oh!

ti-chi-bot bot commented Jan 7, 2025

[LGTM Timeline notifier]

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yibin87 commented Dec 27, 2024 •

edited

Loading

yibin87 Jan 7, 2025 •

edited

Loading