Better support for Go HTTP Client calls #1564
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1564 +/- ##
==========================================
+ Coverage 71.15% 71.18% +0.03%
==========================================
Files 197 197
Lines 19671 19730 +59
==========================================
+ Hits 13996 14044 +48
- Misses 4995 5004 +9
- Partials 680 682 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bpf/go_nethttp.h
Outdated
| if (!url_ptr || !read_go_str("host", | ||
| url_ptr, | ||
| go_offset_of(ot, (go_offset){.v = _host_ptr_pos}), | ||
| &trace.host, | ||
| sizeof(trace.host))) { | ||
| bpf_dbg_printk("can't read http Request.URL.Host"); | ||
| return; | ||
| } | ||
|
|
||
| if (!url_ptr || !read_go_str("scheme", | ||
| url_ptr, | ||
| go_offset_of(ot, (go_offset){.v = _scheme_ptr_pos}), | ||
| &trace.scheme, | ||
| sizeof(trace.scheme))) { | ||
| bpf_dbg_printk("can't read http Request.URL.Scheme"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
| if (!url_ptr || !read_go_str("host", | |
| url_ptr, | |
| go_offset_of(ot, (go_offset){.v = _host_ptr_pos}), | |
| &trace.host, | |
| sizeof(trace.host))) { | |
| bpf_dbg_printk("can't read http Request.URL.Host"); | |
| return; | |
| } | |
| if (!url_ptr || !read_go_str("scheme", | |
| url_ptr, | |
| go_offset_of(ot, (go_offset){.v = _scheme_ptr_pos}), | |
| &trace.scheme, | |
| sizeof(trace.scheme))) { | |
| bpf_dbg_printk("can't read http Request.URL.Scheme"); | |
| return; | |
| } | |
| if (!url_ptr) { | |
| return; | |
| } | |
| if (!read_go_str("host", | |
| url_ptr, | |
| go_offset_of(ot, (go_offset){.v = _host_ptr_pos}), | |
| &trace.host, | |
| sizeof(trace.host))) { | |
| bpf_dbg_printk("can't read http Request.URL.Host"); | |
| return; | |
| } | |
| if (!read_go_str("scheme", | |
| url_ptr, | |
| go_offset_of(ot, (go_offset){.v = _scheme_ptr_pos}), | |
| &trace.scheme, | |
| sizeof(trace.scheme))) { | |
| bpf_dbg_printk("can't read http Request.URL.Scheme"); | |
| return; | |
| } |
rationale: it go me a bit confused if we were checking for a side-effect on url_ptr after the first call
alternatively, check if base_ptr is not null in read_go_str & co.
There was a problem hiding this comment.
good idea, I'll refactor it.
bpf/go_nethttp.h
Outdated
| trace->host[0] = 0; | ||
| trace->scheme[0] = 0; |
There was a problem hiding this comment.
| trace->host[0] = 0; | |
| trace->scheme[0] = 0; | |
| trace->method = '\0'; | |
| trace->path = '\0'; | |
| trace->host[0] = '\0'; | |
| trace->scheme[0] = '\0'; |
- ensure method and path are always null-terminated as well (it matters in spanner.go I guess)
- just being pedantic here, so feel free to ignore -
\0helps the reader visualise we are dealing with a string and not with some sort of array ofint.
There was a problem hiding this comment.
ok, sure, I'll replace those.
There was a problem hiding this comment.
@rafaelroquetto can you explain a bit more that? how is 0 equal to '\0'? shouldn't be just empty string?
There was a problem hiding this comment.
@marctc of course! - all of those above are C strings. A C string is defined to be an array of char that ends with the null terminator, i.e. the number 0.
So the string literal
const char rabbit_str[] = "Rabbit";
is equivalent to
const char rabbit_[7] = { 'R', 'a', 'b', 'b', 'i', 't', '\0' };
// or
const char rabbit_[7] = { 'R', 'a', 'b', 'b', 'i', 't', 0 };
The null terminator tells string manipulation functions where the end of the string is, for instance, to calculate the length of a C string, we can do
size_t strlen(const char* str)
{
size_t i = 0;
while (*str++ != '\0') ++i;
return i;
}
In C, the char literal '\0' has the value of 0. They are the same, and have the same size (usually sizeof(int)). In C++ they are not the same - 0 is an int literal whose size is sizeof(int), whereas '\0' is a char literal whose size is sizeof(char) - so sizeof(0) != sizeof('\0') in C++.
What @grcevski is doing here is simply setting the first element of those char arrays to the null-terminator \0, which represents an empty C-string:
char something[16]; // uninitialised -> garbage
something[0] = '\0'; // something is now { '\0', ... garbage ... };
printf("str: '%s', size = %lu\n", something, strlen(something)); // prints "str: '' size = 0"
In other words, it suffices to set the first char as 0 instead of zeroing the entire buffer. My point was then to suggest using the char literal '\0' rather than the int literal 0 to explicitly indicate to the reader these buffers are actually carrying C strings.
There was a problem hiding this comment.
In other words, it suffices to set the first char as
0instead of zeroing the entire buffer. My point was then to suggest using the char literal'\0'rather than theintliteral0to explicitly indicate to the reader these buffers are actually carrying C strings.
gotcha, thanks for the explanation!
| method := cstr(trace.Method[:]) | ||
| path := cstr(trace.Path[:]) | ||
| scheme := cstr(trace.Scheme[:]) | ||
| origHost := cstr(trace.Host[:]) |
This PR enriches the information we generated for Go HTTP client calls. We use the original host name where possible and we extract the scheme too.
I also fixed a few tests which could be flaky sometimes. The traces test could sometimes miss inqueue and processing because we couldn't capture that information and the Go client program calls out to grafana.com and if we can't reach it we'd never generate any requests. I added a timeout.