Don't read Go maps anymore #1413

grcevski · 2024-11-26T19:51:50Z

Since swiss maps are introduced in Go 1.24, our Go map key extraction will stop working. Please see issue #1369.

I took a look at implementing the swiss map lookup and I'm not sure it's possible to implement the double nested loop without hitting verifier issues, assuming we can reimplement all the bit masking required by the group logic.

So I took a different approach:

For gRPC and HTTP2 I found a place where the data is exposed as an array, so this was easy to traverse.
For HTTP I found a place where the headers are parsed before we add them to the map. This will be slower than our existing approach, but it's a stop gap solution which will work until (if) we decide to reimplement the swiss map logic.

TODO:

Offsets for the hardcoded grpc offset tracking.

Closes #1369

grcevski · 2024-11-26T20:34:22Z

test/integration/red_test_client.go

 	}, test.Interval(100*time.Millisecond))
-
-	spans := trace.FindByOperationName(method)


This removed test was a long shot anyway. Essentially, since we had the ability to parse Go maps we used them to parse the outgoing headers on client calls. This didn't quite fit Beyla's usecase, we auto instrument, we don't expect users to put 'Traceparent' in their outgoing headers. But since it was free (in terms of functionality) we did it anyway, even though if you were tracking incoming call and user manually added traceparent, we'd actually provide a wrong trace. Now however, it's no longer possible. The map is pre-constructed and we can't read it in Go 1.24.

rafaelroquetto

Good stuff! Mostly nits from my end, but I think there may be a potential bug involving the "Traceparent:" literal.

bpf/go_common.h

rafaelroquetto · 2024-11-26T20:25:39Z

bpf/go_common.h

@@ -401,4 +401,37 @@ static __always_inline void *unwrap_tls_conn_info(void *conn_ptr, void *tls_stat
    return conn_ptr;
 }

+static __always_inline void process_meta_frame_headers(void *frame, tp_info_t *tp) {
+    if (frame) {


nit: an early return may improve readability by reducing the indentation depth

rafaelroquetto · 2024-11-26T20:26:11Z

bpf/go_common.h

+        u64 fields_len = 0;
+        bpf_probe_read(&fields_len, sizeof(fields_len), (void *)(frame + 8 + 8));
+        bpf_dbg_printk("fields ptr %llx, len %d", fields, fields_len);
+        if (fields && fields_len > 0) {


Maybe my preference is otherwise :), indenting is a problem, but returns in middle of text are easy to miss.

rafaelroquetto · 2024-11-26T20:38:57Z

bpf/go_common.h

+                    if (!bpf_memicmp((const char *)temp, "traceparent", W3C_KEY_LENGTH)) {
+                        bpf_dbg_printk("found grpc traceparent header");
+                        bpf_probe_read(&temp, W3C_VAL_LENGTH, field.val_ptr);
+                        decode_go_traceparent(temp, tp->trace_id, tp->parent_id, &tp->flags);


should we break after this line?

Good catch.

rafaelroquetto · 2024-11-26T20:40:11Z

bpf/go_nethttp.h

+    u64 len = (u64)GO_PARAM2(ctx);
+    u8 *buf = (u8 *)GO_PARAM1(ctx);
+
+    if (len >= 68) {


any chance we could slap a name here instead of a magic number? It's not clear what 68 means without any further context.

100%, I forgot to update that.

rafaelroquetto · 2024-11-26T20:41:36Z

bpf/go_nethttp.h

+
+        connection_info_t *existing = bpf_map_lookup_elem(&ongoing_server_connections, &g_key);
+        if (existing) {
+            if (!bpf_memicmp((const char *)temp, "Traceparent: ", W3C_KEY_LENGTH + 2)) {


I think there's a potential bug here - the comment of bpf_memicmp states that s2 needs to be lowercase, which is not the case here.

Nice catch too, I forgot about those semantics.

rafaelroquetto · 2024-11-26T20:42:38Z

bpf/go_nethttp.h

-        goroutine_addr,
-        &invocation.tp,
-        (void *)(req + go_offset_of(ot, (go_offset){.v = _req_header_ptr_pos})));
+    __attribute__((__unused__)) u8 existing_tp =


do we still need this attribute xor existing_tp?

Nope we can clean up.

rafaelroquetto · 2024-11-26T20:43:25Z

bpf/trace_common.h

@@ -186,7 +186,7 @@ static __always_inline u8 valid_span(const unsigned char *span_id) {
 }

 static __always_inline u8 valid_trace(const unsigned char *trace_id) {
-    return *((u64 *)trace_id) != 0 && *((u64 *)(trace_id + 8)) != 0;
+    return *((u64 *)trace_id) != 0 || *((u64 *)(trace_id + 8)) != 0;


nice catch!

Co-authored-by: Rafael Roquetto <rafaelroquetto@users.noreply.github.com>

codecov · 2024-11-26T20:57:50Z

Codecov Report

Attention: Patch coverage is 81.81818% with 8 lines in your changes missing coverage. Please review.

Project coverage is 80.99%. Comparing base (d2a8a83) to head (a3990be).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/internal/goexec/structmembers.go	75.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1413      +/-   ##
==========================================
+ Coverage   80.95%   80.99%   +0.03%     
==========================================
  Files         146      146              
  Lines       14822    14877      +55     
==========================================
+ Hits        11999    12049      +50     
- Misses       2234     2240       +6     
+ Partials      589      588       -1

Flag	Coverage Δ
integration-test	`59.09% <81.81%> (-0.09%)`	⬇️
k8s-integration-test	`60.05% <75.00%> (+0.09%)`	⬆️
oats-test	`33.96% <59.09%> (+0.01%)`	⬆️
unittests	`51.48% <47.72%> (-0.05%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mariomac

Amazing! that's life saving

grcevski added 8 commits November 25, 2024 17:05

Add support for http and grpc

d99e8ae

Support http2 request header parsing

d9910bf

Remove unused code

54bda7f

Merge branch 'main' into change_go_map_read

6101843

update bpfs

7a7e162

clang tidy

5993398

Offsets, remove test we no longer support

0dfc0df

update bpfs

745f1ca

grcevski marked this pull request as ready for review November 26, 2024 20:30

grcevski requested review from mariomac, marctc and rafaelroquetto as code owners November 26, 2024 20:30

grcevski commented Nov 26, 2024

View reviewed changes

rafaelroquetto reviewed Nov 26, 2024

View reviewed changes

grcevski and others added 2 commits November 26, 2024 15:47

Update bpf/go_common.h

cb5d138

Co-authored-by: Rafael Roquetto <rafaelroquetto@users.noreply.github.com>

Update bpf/go_common.h

42fbe59

Co-authored-by: Rafael Roquetto <rafaelroquetto@users.noreply.github.com>

Apply review feedback

a742727

rafaelroquetto approved these changes Nov 26, 2024

View reviewed changes

grcevski added 3 commits November 26, 2024 18:50

Fix different grpc lib version

e56b7d0

fix format

7c46a4f

happy linter

4c5006d

mariomac approved these changes Nov 27, 2024

View reviewed changes

Fix old kernel issue

a3990be

grcevski merged commit 9efc255 into grafana:main Nov 27, 2024
15 checks passed

grcevski deleted the change_go_map_read branch November 27, 2024 16:20

tomatopunk mentioned this pull request Dec 16, 2024

bpf_loop Dependency Causes Errors in Kernel Versions < 5.17 When Extending TCP Payload with extend_skb Hook #1439

Open

grcevski mentioned this pull request Jan 17, 2025

Incorrect latency numbers for a Go demo application #1517

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't read Go maps anymore #1413

Don't read Go maps anymore #1413

grcevski commented Nov 26, 2024 •

edited

Loading

grcevski Nov 26, 2024

rafaelroquetto left a comment

rafaelroquetto Nov 26, 2024

rafaelroquetto Nov 26, 2024

grcevski Nov 26, 2024

rafaelroquetto Nov 26, 2024

grcevski Nov 26, 2024

rafaelroquetto Nov 26, 2024

grcevski Nov 26, 2024

rafaelroquetto Nov 26, 2024

grcevski Nov 26, 2024

rafaelroquetto Nov 26, 2024

grcevski Nov 26, 2024

rafaelroquetto Nov 26, 2024

codecov bot commented Nov 26, 2024 •

edited

Loading

mariomac left a comment

		}, test.Interval(100*time.Millisecond))

		spans := trace.FindByOperationName(method)

Don't read Go maps anymore #1413

Don't read Go maps anymore #1413

Conversation

grcevski commented Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

rafaelroquetto 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

mariomac left a comment

Choose a reason for hiding this comment

grcevski commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading