Stats inner/inter zone network traffic for mpp tasks #9747

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.

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 <[email protected]>

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

dbms/src/Flash/Coprocessor/CoprocessorReader.h

@@ -311,7 +322,7 @@ class CoprocessorReader
        return toResult(result_pair, block_queue, header);
    }

-    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 <[email protected]>

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

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 <[email protected]>

update tipb code

944d2cf

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

update tipb commit

6c836bc

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

yibin87 added 16 commits January 6, 2025 17:37

Stats exchange sender network traffic

a2bd6a3

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

Save work

17067f5

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

Handle exchange receiver network traffic

39bd95c

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

Little refact

6ed8c92

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

Little refact

0c11996

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

Add coprocessor remote read connection network stats

9930677

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

Get store zone label using basic way

111b950

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

Save work

0996b36

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

refine tableScan statistics

c0cd583

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

Little refact

1f77702

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

Fix incorrect update issue

cc108c9

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

Little fix and refact

10c14e1

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

Address comments

fb6d9a7

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

Update client-c version

bf54022

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

Address comments

4bfcaf6

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

Little update

5254695

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

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 <[email protected]>

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
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stats inner/inter zone network traffic for mpp tasks #9747

Stats inner/inter zone network traffic for mpp tasks #9747

yibin87 commented Dec 27, 2024 •

edited

Loading

yibin87 commented Dec 27, 2024

yibin87 commented Jan 2, 2025

ti-chi-bot bot commented Jan 2, 2025

yibin87 commented Jan 3, 2025

yibin87 commented Jan 3, 2025

xzhangxian1008 left a comment

xzhangxian1008 Jan 6, 2025

yibin87 Jan 6, 2025

xzhangxian1008 Jan 6, 2025

yibin87 Jan 6, 2025

windtalker Jan 6, 2025

yibin87 Jan 7, 2025

windtalker Jan 6, 2025

yibin87 Jan 7, 2025

windtalker Jan 7, 2025

yibin87 Jan 7, 2025 •

edited

Loading

windtalker left a comment

ti-chi-bot bot commented Jan 7, 2025

ti-chi-bot bot commented Jan 7, 2025

Stats inner/inter zone network traffic for mpp tasks #9747

Stats inner/inter zone network traffic for mpp tasks #9747

Conversation

yibin87 commented Dec 27, 2024 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

yibin87 commented Dec 27, 2024

yibin87 commented Jan 2, 2025

ti-chi-bot bot commented Jan 2, 2025

yibin87 commented Jan 3, 2025

yibin87 commented Jan 3, 2025

xzhangxian1008 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yibin87 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

windtalker left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jan 7, 2025

ti-chi-bot bot commented Jan 7, 2025

[LGTM Timeline notifier]

yibin87 commented Dec 27, 2024 •

edited

Loading

yibin87 Jan 7, 2025 •

edited

Loading