Tcp long conn metrics and accesslogs (new) #1298

yp969803 · 2025-04-10T00:09:20Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #1211

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
Yes

Tcp long conn metrics and accesslogs

…p_probe_info Signed-off-by: Yash Patel <[email protected]>

codecov · 2025-04-10T00:25:25Z

Codecov Report

Attention: Patch coverage is 37.61905% with 131 lines in your changes missing coverage. Please review.

Project coverage is 45.50%. Comparing base (cff9c00) to head (3a7c6e8).
Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/controller/telemetry/metric.go	14.49%	59 Missing ⚠️
pkg/bpf/workload/skb.go	52.45%	39 Missing and 19 partials ⚠️
pkg/bpf/workload/loader.go	0.00%	8 Missing and 4 partials ⚠️
pkg/controller/telemetry/accesslog.go	71.42%	2 Missing ⚠️

Files with missing lines	Coverage Δ
pkg/controller/telemetry/accesslog.go	`90.32% <71.42%> (ø)`
pkg/bpf/workload/loader.go	`23.43% <0.00%> (-2.43%)`	⬇️
pkg/bpf/workload/skb.go	`52.45% <52.45%> (ø)`
pkg/controller/telemetry/metric.go	`48.55% <14.49%> (-2.87%)`	⬇️

... and 4 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 5745678...3a7c6e8. Read the comment docs.

🚀 New features to boost your workflow:

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

…probe_info Signed-off-by: Yash Patel <[email protected]>

yp969803 · 2025-04-10T01:20:04Z

@hzxuzhonghu can you look at this pr, I have used a more improved approach

hzxuzhonghu · 2025-04-10T01:25:09Z

Not sure why you open a new pr, basically it is more facilitating to update on the existing one. Which is more easy for reviewing

yp969803 · 2025-04-10T01:29:53Z

I have done a lot of experiments with the previous pr, it contains lots of unnecessary commits, also this time, I’ve taken a different and cleaner approach, which will make it much easier to review and merge.
Sorry for the inconvenience caused by me !!

bpf/include/bpf_common.h

hzxuzhonghu · 2025-04-10T01:32:31Z

bpf/kmesh/probes/probe.h

@@ -3,6 +3,7 @@

 #ifndef __KMESH_BPF_PROBE_H__
 #define __KMESH_BPF_PROBE_H__
+#define LONG_CONN_THRESHOLD_TIME (5 * 1000000000ULL) // 5s


please move down after the include

Ok, i have misunderstood it to move down one line !!

hzxuzhonghu · 2025-04-10T01:33:18Z

bpf/kmesh/probes/probe.h

+    bpf_sk_storage_delete(&map_of_sock_storage, sk);
+}
+
+static inline void observe_on_recv(struct bpf_sock *sk)


remove, i donot see it is used now. I guess you are writing for recv metric later.

Sorry for inconvience, it is different approach, it do not need different hooks for send and recv only one hook do the work, currently i have drafted that pr

I think only do in recv is not right, some tcp connections only have send no recv message

if that is the case i can use

SEC("cgroup_skb/egress") int sendmsg_cgrp_prog(struct __sk_buff *skb)

if i you want i can also use both the hooks for send and recv
SEC("cgroup_skb/ingress")
and
SEC("cgroup_skb/egress")

AFAK, we have sk_msg, we donont need cgroup_skb/egress now. cgroup_skb/ingress may still need to acquire the relatively realtime recv bytes

@yp969803 can you please explain about this confusion, could we duplicately calculate the send/recv bytes

hzxuzhonghu · 2025-04-10T01:34:29Z

bpf/kmesh/probes/tcp_probe.h

@@ -35,18 +35,20 @@ struct tcp_probe_info {
    __u32 type;
    struct bpf_sock_tuple tuple;
    struct orig_dst_info orig_dst;
-    __u32 sent_bytes;
-    __u32 received_bytes;
+    __u64 conn_id;        /* sock_cookie */


we donot need this

I am using conn_id in the userspace as key for the map

tcp_conns := make(map[uint64]connMetric)

We can use tuple as key, sock cookie is meaningless in userspace

different connections with same src and dest address has same tuple keys,

The metrics actually are accumulated at application to application level. So it does not matter at all

If we want, for long connections, we can also accumulate metrics at connection level

We should and we must

hzxuzhonghu · 2025-04-10T01:36:46Z

bpf/kmesh/workload/recvmsg.c

+#include "probe.h"
+
+SEC("cgroup_skb/ingress")
+int recvmsg_prog(struct __sk_buff *skb)


Please remove and handle recv in the other pr. And i do not see it in the proposal?

hzxuzhonghu

I donot see the send msg logic, do you miss it?

yp969803 · 2025-04-10T01:41:35Z

@hzxuzhonghu I am using a different approach here, which does not require send_msg logic, I will make changes to the proposal for this pr to make the understanding of the approach clear, also I will explain the approach in today's meet

hzxuzhonghu · 2025-04-10T01:42:49Z

#1298 (comment)

yp969803 · 2025-04-10T14:21:30Z

These are the final logs

accesslog: 2025-04-09 15:05:58.511324246 +0000 UTC src.addr=10.244.2.10:39826, src.workload=busybox-telnet, src.namespace=default, dst.addr=10.244.2.7:23, dst.service=telnet-service.default.svc.cluster.local, dst.workload=busybox-telnet-server, dst.namespace=default, connection_id=712410, start_time=2025-04-09 15:05:58.511169295 +0000 UTC, direction=OUTBOUND, status=BPF_TCP_ESTABLISHED, sent_bytes=1, received_bytes=0, packet_loss=0, retransmissions=0, srtt=760us, min_rtt=95us, duration=0.154951ms
accesslog: 2025-04-09 15:05:58.511381102 +0000 UTC src.addr=10.244.2.10:39826, src.workload=busybox-telnet, src.namespace=default, dst.addr=10.244.2.7:23, dst.service=telnet-service.default.svc.cluster.local, dst.workload=busybox-telnet-server, dst.namespace=default, connection_id=712411, start_time=2025-04-09 15:05:58.511378715 +0000 UTC, direction=INBOUND, status=BPF_TCP_ESTABLISHED, sent_bytes=1, received_bytes=0, packet_loss=0, retransmissions=0, srtt=808us, min_rtt=101us, duration=0.002387ms
accesslog: 2025-04-09 15:06:03.676574254 +0000 UTC src.addr=10.244.2.10:39826, src.workload=busybox-telnet, src.namespace=default, dst.addr=10.244.2.7:23, dst.service=telnet-service.default.svc.cluster.local, dst.workload=busybox-telnet-server, dst.namespace=default, connection_id=712410, start_time=2025-04-09 15:05:58.511169295 +0000 UTC, direction=OUTBOUND, status=BPF_TCP_ESTABLISHED, sent_bytes=10, received_bytes=55, packet_loss=0, retransmissions=0, srtt=566us, min_rtt=52us, duration=5165.404959ms
accesslog: 2025-04-09 15:06:03.676628152 +0000 UTC src.addr=10.244.2.10:39826, src.workload=busybox-telnet, src.namespace=default, dst.addr=10.244.2.7:23, dst.service=telnet-service.default.svc.cluster.local, dst.workload=busybox-telnet-server, dst.namespace=default, connection_id=712411, start_time=2025-04-09 15:05:58.511378715 +0000 UTC, direction=INBOUND, status=BPF_TCP_ESTABLISHED, sent_bytes=11, received_bytes=29, packet_loss=0, retransmissions=0, srtt=14527us, min_rtt=12us, duration=5165.249437ms
accesslog: 2025-04-09 15:06:08.706203481 +0000 UTC src.addr=10.244.2.10:39826, src.workload=busybox-telnet, src.namespace=default, dst.addr=10.244.2.7:23, dst.service=telnet-service.default.svc.cluster.local, dst.workload=busybox-telnet-server, dst.namespace=default, connection_id=712410, start_time=2025-04-09 15:05:58.511169295 +0000 UTC, direction=OUTBOUND, status=BPF_TCP_ESTABLISHED, sent_bytes=30, received_bytes=124, packet_loss=0, retransmissions=0, srtt=5704us, min_rtt=43us, duration=10195.034186ms
accesslog: 2025-04-09 15:06:08.706228193 +0000 UTC src.addr=10.244.2.10:39826, src.workload=busybox-telnet, src.namespace=default, dst.addr=10.244.2.7:23, dst.service=telnet-service.default.svc.cluster.local, dst.workload=busybox-telnet-server, dst.namespace=default, connection_id=712411, start_time=2025-04-09 15:05:58.511378715 +0000 UTC, direction=INBOUND, status=BPF_TCP_ESTABLISHED, sent_bytes=32, received_bytes=55, packet_loss=0, retransmissions=0, srtt=41995us, min_rtt=8us, duration=10194.849478ms

Signed-off-by: Yash Patel <[email protected]> rfac: added workload include to performance_probe.h Signed-off-by: Yash Patel <[email protected]> rfac: moved is_managed_bt_kmesh_skb to bpf_common.h Signed-off-by: Yash Patel <[email protected]> chore: run make gen Signed-off-by: Yash Patel <[email protected]> rfac: changed recv.c to cgrp_skb.c Signed-off-by: Yash Patel <[email protected]> rfac: created skb.go to load and attach cgroup_skb Signed-off-by: Yash Patel <[email protected]> rfac: tcp_probe.h Signed-off-by: Yash Patel <[email protected]> rfac: skb.go Signed-off-by: Yash Patel <[email protected]>

hzxuzhonghu · 2025-04-11T00:54:14Z

pkg/controller/telemetry/accesslog.go

 	timeInfo := fmt.Sprintf("%v", uptime)
 	sourceInfo := fmt.Sprintf("src.addr=%s, src.workload=%s, src.namespace=%s", accesslog.sourceAddress, accesslog.sourceWorkload, accesslog.sourceNamespace)
 	destinationInfo := fmt.Sprintf("dst.addr=%s, dst.service=%s, dst.workload=%s, dst.namespace=%s", accesslog.destinationAddress, accesslog.destinationService, accesslog.destinationWorkload, accesslog.destinationNamespace)
-	connectionInfo := fmt.Sprintf("direction=%s, sent_bytes=%d, received_bytes=%d, srtt=%dus, min_rtt=%dus, duration=%vms", accesslog.direction, data.sentBytes, data.receivedBytes, data.srtt, data.minRtt, (float64(data.duration) / 1000000.0))
+	connectionInfo := fmt.Sprintf("connection_id=%d, start_time=%s, direction=%s, status=%s, sent_bytes=%d, received_bytes=%d, packet_loss=%d, retransmissions=%d, srtt=%dus, min_rtt=%dus, duration=%vms", conn_metrics.connId, startTimeInfo, accesslog.direction, accesslog.status, conn_metrics.sentBytes, conn_metrics.receivedBytes, conn_metrics.packetLost, conn_metrics.totalRetrans, data.srtt, data.minRtt, (float64(data.duration) / 1000000.0))


we donot need to build accesslog periodically.

hzxuzhonghu · 2025-04-11T00:54:57Z

pkg/controller/telemetry/metric.go

@@ -86,17 +102,18 @@ type statistics struct {
 	ConnectSuccess uint32
 	Direction      uint32
 	State          uint32
+	_              uint32


please add some comments

hzxuzhonghu · 2025-04-11T00:55:57Z

pkg/controller/telemetry/metric.go

@@ -106,11 +123,13 @@ type connectionDataV4 struct {
 	OriginalAddr uint32
 	OriginalPort uint16
 	_            uint16
-	_            [3]uint32
+	_            [4]uint32
+	ConnId       uint64


hzxuzhonghu · 2025-04-11T00:56:36Z

pkg/controller/telemetry/metric.go

@@ -383,9 +431,10 @@ func (m *MetricController) Run(ctx context.Context, mapOfTcpInfo *ebpf.Map) {
 	}
 }

-func buildV4Metric(buf *bytes.Buffer) (requestMetric, error) {
+func buildV4Metric(buf *bytes.Buffer, tcp_conns map[uint64]connMetric) (requestMetric, error) {


@LiZhenCheng9527 ptal

yp969803 · 2025-04-11T02:42:19Z

@hzxuzhonghu i have addressed all your comments, you can look into it when you get time

Signed-off-by: Yash Patel <[email protected]>

hzxuzhonghu · 2025-04-14T07:15:31Z

bpf/kmesh/probes/probe.h

@@ -81,5 +83,26 @@ static inline void observe_on_close(struct bpf_sock *sk)
    }

    tcp_report(sk, tcp_sock, storage, BPF_TCP_CLOSE);
+    bpf_sk_storage_delete(&map_of_sock_storage, sk);


remove, this kind of map does not need to delete manually

hzxuzhonghu · 2025-04-14T07:26:41Z

I think i have requested many many times, we donot need connection_id=712411,, which is meaningless from user view

yp969803 · 2025-04-14T10:25:33Z

I will remove the connection_id from the access logs
I want conn_id in userspace for the key of the map

tcp_conns := make(map[uint64]connMetric)

I can use tuple as a key, but then I have to declare a new struct in userspace, also this struct will have variable size for IP v4 and v6 , which is a problem, also the size of the key will also increase, I think there is no flaw in using conn_id in the userspace as a map key because it is much more smaller size than the tupple and have fix size.

@hzxuzhonghu

Signed-off-by: Yash Patel <[email protected]>

yp969803 · 2025-04-14T12:37:26Z

@nlgwcy can you review the ebpf part when you get time !!

hzxuzhonghu · 2025-04-15T01:19:19Z

I want conn_id in userspace for the key of the map

Can you use the src dst {ip: port}, it can not conflict during the small interval

yp969803 · 2025-04-15T11:12:07Z

@hzxuzhonghu removed conn_id from everywhere as per your request, you can merge the pr now

hzxuzhonghu · 2025-04-15T11:30:33Z

bpf/include/bpf_common.h

@@ -44,6 +44,8 @@ struct {

 struct sock_storage_data {
    __u64 connect_ns;
+    __u64 last_report_ns;
+    __u64 sock_cookie;


we can remove now, right?

…ric.go, also rmoved conn_id in tcp_probe_info Signed-off-by: Yash Patel <[email protected]> rfac: rmoved conn_id from tcp_probe_info Signed-off-by: Yash Patel <[email protected]>

hzxuzhonghu · 2025-04-15T11:51:09Z

@nlgwcy Is there any other comments

hzxuzhonghu · 2025-04-17T01:57:38Z

/retest

hzxuzhonghu

/lgtm

Some tests needed, but can be added later

kmesh-bot · 2025-04-17T02:07:39Z

[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

rfac: changed userspace metric and accesslog to add more fields of tc…

91457bc

…p_probe_info Signed-off-by: Yash Patel <[email protected]>

kmesh-bot added the kind/feature label Apr 10, 2025

kmesh-bot requested review from hzxuzhonghu and YaoZengzeng April 10, 2025 00:09

kmesh-bot added the size/L label Apr 10, 2025

rfac: changed tcp_probe.h functions to also report new fields of tcp_…

263a04d

…probe_info Signed-off-by: Yash Patel <[email protected]>

kmesh-bot added size/XL size/XXL and removed size/L size/XL labels Apr 10, 2025

yp969803 force-pushed the issue1211 branch from 2c5ba22 to 425fbee Compare April 10, 2025 01:18

hzxuzhonghu reviewed Apr 10, 2025

View reviewed changes

bpf/include/bpf_common.h Outdated Show resolved Hide resolved

hzxuzhonghu reviewed Apr 10, 2025

View reviewed changes

hzxuzhonghu requested changes Apr 10, 2025

View reviewed changes

LiZhenCheng9527 added this to the v1.1 milestone Apr 10, 2025

yp969803 force-pushed the issue1211 branch 3 times, most recently from e5b081c to 5b3a8ef Compare April 10, 2025 14:14

yp969803 force-pushed the issue1211 branch from 5b3a8ef to aa15f4f Compare April 10, 2025 14:26

hzxuzhonghu reviewed Apr 11, 2025

View reviewed changes

yp969803 force-pushed the issue1211 branch from 05b9cf8 to ab58fee Compare April 11, 2025 02:35

yp969803 requested a review from hzxuzhonghu April 11, 2025 02:38

yp969803 force-pushed the issue1211 branch from ab58fee to 69aaf5e Compare April 11, 2025 02:47

chore: added usefull comments

d1eb2db

Signed-off-by: Yash Patel <[email protected]>

yp969803 force-pushed the issue1211 branch from 69aaf5e to d1eb2db Compare April 11, 2025 02:49

hzxuzhonghu reviewed Apr 14, 2025

View reviewed changes

rfac: rmoved conn_id from accesslogs

d543960

Signed-off-by: Yash Patel <[email protected]>

hzxuzhonghu reviewed Apr 15, 2025

View reviewed changes

rfac: use connectionSrcDst in tcp_conn map and removed conn_id in met…

3a7c6e8

…ric.go, also rmoved conn_id in tcp_probe_info Signed-off-by: Yash Patel <[email protected]> rfac: rmoved conn_id from tcp_probe_info Signed-off-by: Yash Patel <[email protected]>

yp969803 force-pushed the issue1211 branch from 2373fbc to 3a7c6e8 Compare April 15, 2025 11:35

hzxuzhonghu approved these changes Apr 15, 2025

View reviewed changes

kmesh-bot added the approved label Apr 15, 2025

hzxuzhonghu approved these changes Apr 17, 2025

View reviewed changes

kmesh-bot assigned hzxuzhonghu Apr 17, 2025

kmesh-bot added the lgtm label Apr 17, 2025

kmesh-bot merged commit 578f3ee into kmesh-net:main Apr 17, 2025
12 of 13 checks passed

yp969803 mentioned this pull request Apr 17, 2025

metrics in Kernel Native Mode #1284

Open

Tcp long conn metrics and accesslogs (new) #1298

Tcp long conn metrics and accesslogs (new) #1298

Uh oh!

Conversation

yp969803 commented Apr 10, 2025

Uh oh!

codecov bot commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yp969803 commented Apr 10, 2025

Uh oh!

hzxuzhonghu commented Apr 10, 2025

Uh oh!

yp969803 commented Apr 10, 2025

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!

yp969803 Apr 10, 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!

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!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

yp969803 commented Apr 10, 2025

Uh oh!

hzxuzhonghu commented Apr 10, 2025

Uh oh!

yp969803 commented Apr 10, 2025

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!

yp969803 commented Apr 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu commented Apr 14, 2025

Uh oh!

yp969803 commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

codecov bot commented Apr 10, 2025 •

edited

Loading

yp969803 Apr 10, 2025 •

edited

Loading

yp969803 commented Apr 14, 2025 •

edited

Loading

yp969803 commented Apr 14, 2025 •

edited

Loading