Add Performance Monitoring #887

skwwwwww · 2024-09-23T05:31:14Z

What type of PR is this?

/kind feature

codecov · 2024-09-23T05:44:56Z

Codecov Report

Attention: Patch coverage is 1.09290% with 181 lines in your changes missing coverage. Please review.

Project coverage is 48.45%. Comparing base (1902392) to head (efb1588).
Report is 75 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/controller/telemetry/map_metric.go	0.00%	94 Missing ⚠️
pkg/controller/telemetry/operation_metric.go	0.00%	78 Missing ⚠️
pkg/controller/workload/workload_controller.go	0.00%	7 Missing ⚠️
pkg/controller/controller.go	0.00%	2 Missing ⚠️

Files with missing lines	Coverage Δ
pkg/controller/telemetry/utils.go	`64.81% <100.00%> (+1.35%)`	⬆️
pkg/controller/controller.go	`0.00% <0.00%> (ø)`
pkg/controller/workload/workload_controller.go	`53.84% <0.00%> (-4.49%)`	⬇️
pkg/controller/telemetry/operation_metric.go	`0.00% <0.00%> (ø)`
pkg/controller/telemetry/map_metric.go	`0.00% <0.00%> (ø)`

... and 24 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4ca182...efb1588. Read the comment docs.

LiZhenCheng9527

When to delete outdated map metrics.
Refactor the struct2map function.
The judgement of Error needs to specify the source of the error, if not necessary, you can not define the error first.

bpf/kmesh/probes/performance_probe.h

LiZhenCheng9527 · 2024-09-23T06:54:13Z

bpf/kmesh/probes/performance_probe.h

+{
+    struct operation_usage_data data = {};
+    struct operation_usage_key key = {};
+    __u32 tid = bpf_get_current_pid_tgid() & 0xFFFFFFFF;


Why do & operation

low 32 bits returned by the bpf_get_current_pid_tgid() function represent the PID of the thread, the high 32 bits represent the TGID of the thread group

pkg/controller/telemetry/map_metric.go

pkg/controller/telemetry/operation_metric.go

docs/proposal/performance_monitoring.md

samples/addons/kmesh_performance-monitoring.json

pkg/controller/telemetry/map_metric.go

docs/proposal/performance_monitoring.md

hzxuzhonghu · 2024-09-24T08:33:36Z

docs/proposal/performance_monitoring.md

+struct operation_usage_data {
+    __u64 start_time;
+    __u64 end_time;
+    __u32 operation_type;


What is operation_type? And why it is in both key and value

Operation_type refers to a function, such as sock_traffic_control.

hzxuzhonghu · 2024-09-24T08:37:42Z

docs/proposal/performance_monitoring.md

+};
+
+struct operation_usage_key {
+    __u32 tid;


is this pid?

how can we map it to a pod

The tid is the unique identifier of a thread obtained through bpf_get_current_pid_tgid().

A unique kmesh-daemon runs on each node, and kmesh collects data from different nodes, allowing the data to be associated with the respective pods.

docs/proposal/performance_monitoring.md

weli-l · 2024-10-24T01:58:26Z

pkg/controller/telemetry/map_metric.go

+			continue
+		}
+		memLock := (info.KeySize + info.ValueSize) * info.MaxEntries
+		if memLock%4096 != 0 {


better to use macro

weli-l · 2024-10-24T02:01:48Z

pkg/controller/telemetry/map_metric.go

+		}
+		startID = mapID
+		count++
+		mapData.mapId = uint32(mapID)


better to split this function to ensure logical independence

Signed-off-by: skw <2438567342@qq.com>

skwwwwww · 2024-10-24T15:56:09Z

@hzxuzhonghu
The code has been improved and conflicts have been resolved

hzxuzhonghu

Not quite sure how this will influence the data plane performance.

hzxuzhonghu · 2024-09-25T06:54:19Z

pkg/controller/telemetry/map_metric.go

+	mapMetricCache map[mapMetricLabels]*mapUsageInfo
+}
+
+type mapUsageMetric struct {


please find a more proper name

hzxuzhonghu · 2024-10-28T02:34:16Z

bpf/kmesh/probes/performance_probe.h

+    __type(key, struct operation_usage_key);
+    __type(value, struct operation_usage_data);
+    __uint(max_entries, 1024);
+} performance_data_map SEC(".maps");


Please update the map name with kmesh prefix

hzxuzhonghu · 2024-10-28T02:34:55Z

bpf/kmesh/probes/performance_probe.h

+struct {
+    __uint(type, BPF_MAP_TYPE_RINGBUF);
+    __uint(max_entries, RINGBUF_SIZE);
+} map_perf_info SEC(".maps");


why not remove performance_data_map map and make it inlined into the ringbuffer?

pkg/controller/telemetry/map_metric.go

hzxuzhonghu · 2024-10-28T02:41:55Z

pkg/controller/telemetry/map_metric.go

+	keySize    uint32
+	valueSize  uint32
+	memLock    uint64
+	entryCount uint32


should we export those attributes like bpftool -j?

hzxuzhonghu · 2024-10-28T02:45:05Z

pkg/controller/telemetry/operation_metric.go

+func (m *OperationMetricController) buildOperationMetric(data *operationUsageMetric) operationMetricLabels {
+	labels := operationMetricLabels{}
+	// Get the actual pod name
+	podName := os.Getenv("HOSTNAME")


This is not the pod name

hzxuzhonghu · 2024-10-30T06:35:26Z

bpf/kmesh/probes/performance_probe.h

+    key.tid = tid;
+    data.start_time = bpf_ktime_get_ns();
+    data.operation_type = operation_type;
+    bpf_map_update_elem(&kmesh_perf_map, &key, &data, BPF_ANY);


It does not support concurrent connect

Use pid_tgid as part of the key to solve the concurrency issue.

hzxuzhonghu · 2024-10-30T06:35:46Z

bpf/kmesh/probes/performance_probe.h

+};
+
+struct operation_usage_key {
+    __u32 tid;


pid, right?

hzxuzhonghu · 2024-10-30T06:38:20Z

bpf/kmesh/probes/performance_probe.h

+    info->start_time = data->start_time;
+    info->end_time = data->end_time;
+    info->operation_type = data->operation_type;
+    bpf_ringbuf_submit(info, 0);


you only report thes info to userspace, donot we care about which pid is operating on this?

The pid_tgid has been added to the labels.

pkg/controller/telemetry/map_metric.go

hzxuzhonghu · 2024-10-30T07:06:46Z

pkg/controller/telemetry/operation_metric.go

+		nodeName = "unknown-node-name"
+	}
+	labels.nodeName = nodeName
+	labels.operationType = fmt.Sprintf("%d", data.operationType)


use string instead of int， this is exposed to user

hzxuzhonghu · 2024-10-30T07:08:58Z

bpf/kmesh/probes/performance_probe.h

+    __uint(type, BPF_MAP_TYPE_HASH);
+    __type(key, struct operation_usage_key);
+    __type(value, struct operation_usage_data);
+    __uint(max_entries, 1024);


This value is too small

hzxuzhonghu · 2024-10-30T09:27:27Z

pkg/controller/telemetry/map_metric.go

+)
+
+const (
+	mapMetricFlushInterval = 5 * time.Second


make it align with previous metrics, 15s iirc

hzxuzhonghu · 2024-10-30T09:29:30Z

pkg/controller/telemetry/operation_metric.go

+	labels := operationMetricLabels{}
+	nodeName := os.Getenv("NODE_NAME")
+	if nodeName == "" {
+		nodeName = "unknown-node-name"


Suggested change

nodeName = "unknown-node-name"

nodeName = "unknown"

hzxuzhonghu · 2024-10-30T09:30:30Z

pkg/controller/telemetry/operation_metric.go

+	}
+	labels.nodeName = nodeName
+	labels.operationType = operationTypeMap[data.operationType]
+	labels.pidTgid = fmt.Sprintf("%d", data.pidTgid)


pure pid does not make sense, ideally we should have correlate with pod name/namespace

So i suggest first leave it unset

The ability to retrieve pidTgid is retained, but it is not added to the labels.

hzxuzhonghu · 2024-10-30T09:31:57Z

bpf/kmesh/probes/performance_probe.h

+struct operation_usage_data {
+    __u64 start_time;
+    __u64 end_time;
+    __u64 pid_tgid;


nit: we donot need to have this in both key and value

hzxuzhonghu

BTW, we should have a feature flag to control whether to enable it

hzxuzhonghu · 2024-10-31T02:05:18Z

pkg/controller/telemetry/map_metric.go

+type MapMetricController struct {
+}
+
+type mapEntrycountMetric struct {


nit: MapInfo

hzxuzhonghu · 2024-10-31T02:07:41Z

pkg/controller/telemetry/map_metric.go

+}
+
+type totalMapMetricLabels struct {
+	nodeName string


can you remove this sturct, so similar with mapMetricLabels

ping @skwwwwww

Considering code reuse and style before, I think there is no need to delete it

I doubt isn't this duplicate with mapMetricLabels

One is a label for a single map, and the other is a label for the total number of maps, requiring different struct to label different data

hzxuzhonghu · 2024-10-31T02:10:45Z

pkg/controller/telemetry/map_metric.go

+		if err != nil {
+			break
+		}
+		defer mapInfo.Close()


It is not good practice to call defer within a loop

hzxuzhonghu · 2024-10-31T02:12:07Z

pkg/controller/telemetry/map_metric.go

+		defer mapInfo.Close()
+
+		if info.Name == "" {
+			startID = mapID


can this be always set

Signed-off-by: skw <2438567342@qq.com>

hzxuzhonghu

/lgtm

kmesh-bot · 2024-10-31T10:35:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

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~~ [hzxuzhonghu]

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

kmesh-bot added the kind/feature label Sep 23, 2024

kmesh-bot requested review from YaoZengzeng and supercharge-xsy September 23, 2024 05:31

kmesh-bot added the size/XXL label Sep 23, 2024

LiZhenCheng9527 reviewed Sep 23, 2024

View reviewed changes

YaoZengzeng reviewed Sep 24, 2024

View reviewed changes

docs/proposal/performance_monitoring.md Outdated Show resolved Hide resolved

YaoZengzeng reviewed Sep 24, 2024

View reviewed changes

docs/proposal/performance_monitoring.md Outdated Show resolved Hide resolved

YaoZengzeng reviewed Sep 24, 2024

View reviewed changes

samples/addons/kmesh_performance-monitoring.json Show resolved Hide resolved

YaoZengzeng reviewed Sep 24, 2024

View reviewed changes

pkg/controller/telemetry/map_metric.go Outdated Show resolved Hide resolved

hzxuzhonghu reviewed Sep 24, 2024

View reviewed changes

hzxuzhonghu mentioned this pull request Sep 24, 2024

Add Use Grafana to visualize kmesh perofrmance monitoring kmesh-net/website#77

Merged

skwwwwww requested review from LiZhenCheng9527 and hzxuzhonghu October 15, 2024 05:48

kmesh-bot added the do-not-merge/contains-merge-commits label Oct 22, 2024

weli-l reviewed Oct 24, 2024

View reviewed changes

kmesh-bot added do-not-merge/contains-merge-commits and removed do-not-merge/contains-merge-commits labels Oct 24, 2024

Add Performance Monitoring

be94d50

Signed-off-by: skw <2438567342@qq.com>

kmesh-bot added do-not-merge/contains-merge-commits and removed do-not-merge/contains-merge-commits labels Oct 24, 2024

skwwwwww requested a review from YaoZengzeng October 27, 2024 14:01

hzxuzhonghu reviewed Oct 28, 2024

View reviewed changes

hzxuzhonghu reviewed Oct 30, 2024

View reviewed changes

hzxuzhonghu reviewed Oct 31, 2024

View reviewed changes

fix

008e9b5

Signed-off-by: skw <2438567342@qq.com>

hzxuzhonghu previously approved these changes Oct 31, 2024

View reviewed changes

kmesh-bot added the approved label Oct 31, 2024

skwwwwww dismissed hzxuzhonghu’s stale review via 083b299 October 31, 2024 09:36

kmesh-bot removed the approved label Oct 31, 2024

Added a kernel-mode switch for the feature.

efb1588

Signed-off-by: skw <2438567342@qq.com>

skwwwwww requested review from hzxuzhonghu and weli-l October 31, 2024 10:21

hzxuzhonghu approved these changes Oct 31, 2024

View reviewed changes

kmesh-bot assigned hzxuzhonghu Oct 31, 2024

kmesh-bot added the lgtm label Oct 31, 2024

kmesh-bot added the approved label Oct 31, 2024

kmesh-bot merged commit 4b403b2 into kmesh-net:main Oct 31, 2024
9 checks passed

LiZhenCheng9527 mentioned this pull request Dec 30, 2024

Add Performance Monitoring user guide kmesh-net/website#104

Closed

Add Performance Monitoring #887

Add Performance Monitoring #887

Uh oh!

Conversation

skwwwwww commented Sep 23, 2024

Uh oh!

codecov bot commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LiZhenCheng9527 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skwwwwww commented Oct 24, 2024

Uh oh!

hzxuzhonghu 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!

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!

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!

codecov bot commented Sep 23, 2024 •

edited

Loading