Add support for FastCGI protocol #1417
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1417 +/- ##
==========================================
+ Coverage 72.41% 81.07% +8.66%
==========================================
Files 145 147 +2
Lines 14868 15033 +165
==========================================
+ Hits 10766 12188 +1422
+ Misses 3389 2257 -1132
+ Partials 713 588 -125
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
grcevski
requested review from
mariomac,
marctc and
rafaelroquetto
as code owners
| //unsigned char tp_buf[TP_MAX_VAL_LENGTH]; | ||
| //make_tp_string(tp_buf, &tp_p->tp); | ||
| //bpf_dbg_printk("tp: %s", tp_buf); |
There was a problem hiding this comment.
Same, that's expensive but when I debug traceparent issues, I keep needing to write the code again.
| //bpf_dbg_printk("Looking up existing trace for connection"); | ||
| //dbg_print_http_connection_info(conn); |
There was a problem hiding this comment.
Just double checking you intended to leave those in.
There was a problem hiding this comment.
Yeah it's good for the future if we need to debug the code.
| //make_tp_string(tp_buf, &tp_p->tp); | ||
| //bpf_dbg_printk("tp: %s", tp_buf); | ||
|
|
||
| #ifdef BPF_TRACEPARENT |
There was a problem hiding this comment.
Since we are moving this around, may I suggest the following?
// at the top of this file or some common header
#ifdef BPF_TRACEPARENT
enum { PARSE_TRACEPARENT = 1 };
#else
enum { PARSE_TRACEPARENT = 0 };
#endif
// then here
if (PARSE_TRACEPARENT) { ... }
the rationale is that this ensures this code is always sieved through the compiler, thus allowing it to catch any compilation error even when the macro is used. The code will not be present in the final binary when PARSE_TRACEPARENT is 0 because the compiler will optimise it out anyway, so best of two worlds and less error-prone.
Another unrelated to the above thought: should this happen before the find_trace_for_xxx(...) code? This snippet seems to run irrespective of found_tp, so perhaps moving it before and bailing earlier if tp is found makes sense?
Or would it even make sense to only run this if found_tp == false, or is this some sort of unconditional fallback?
There was a problem hiding this comment.
I don't think the compilation is an issue, since we built all versions when compiling. Also the found_tp is not considered, because if we find something encoded in headers we don't care if we found something else before.
| @@ -94,15 +119,15 @@ static __always_inline u8 correlated_request_with_current(tp_info_pid_t *existin | |||
| return 0; | |||
| } | |||
|
|
|||
| u64 pid_tid = bpf_get_current_pid_tgid(); | |||
| //u64 pid_tid = bpf_get_current_pid_tgid(); | |||
There was a problem hiding this comment.
just noting those here in case they were not intended
There was a problem hiding this comment.
Yeah I removed the pid checking, I'll clean up this code in the next round of work I'm doing. I moved the type to be in the key, so now we don't have to worry about the pid.
This PR adds support for tracking the FastCGI protocol which is very popular with modern PHP applications. It's more less an extension to what we already do for other types of TCP requests, I just added a detector for it.
TODO:
Relates to #1395
I didn't want to put closes on the issue because I still need to ensure Beyla works with Unix sockets. If it's business as usual, then we can close the issue, if not I have to look into if it's possible at all.