Added new response body size to metrics/traces #1801
Conversation
|
|
16f6e01 to
9858e85
Compare
go.mod
Outdated
| @@ -39,7 +39,6 @@ require ( | |||
| go.opentelemetry.io/collector/config/confighttp v0.122.1 | |||
| go.opentelemetry.io/collector/config/configopaque v1.28.1 | |||
| go.opentelemetry.io/collector/config/configretry v1.28.1 | |||
| go.opentelemetry.io/collector/config/configtelemetry v0.122.1 | |||
There was a problem hiding this comment.
Why is this removed? Did we forget to remove it in our PRs updating OTEL?
There was a problem hiding this comment.
I thought it was missing after go mod tidy && go mod vendor, but I got it back
pkg/internal/goexec/offsets.json
Outdated
| @@ -44,7 +44,7 @@ | |||
| "conn": { | |||
| "versions": { | |||
| "oldest": "1.40.0", | |||
| "newest": "1.45.1" | |||
There was a problem hiding this comment.
I'm not sure if we should modify this file manually. It's being updated thru this https://github.com/grafana/go-offsets-tracker
There was a problem hiding this comment.
I updated that file by running the command:
make update-offsetsIs that wrong? btw my current version is go 1.24.2
There was a problem hiding this comment.
Yes, I probably did something wrong, I rolled back the changes in the offsets.json file except for adding an offset for net/http.Response.ContentLength
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1801 +/- ##
==========================================
+ Coverage 67.84% 68.03% +0.18%
==========================================
Files 225 225
Lines 22966 23067 +101
==========================================
+ Hits 15582 15694 +112
+ Misses 6601 6593 -8
+ Partials 783 780 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bpf/gotracer/go_nethttp.c
Outdated
| sizeof(trace->response_length), | ||
| (void *)(resp_ptr + response_length_ptr_pos)); | ||
|
|
||
| bpf_dbg_printk("response_length %d, offset %d, resp_ptr %lx", |
There was a problem hiding this comment.
| bpf_dbg_printk("response_length %d, offset %d, resp_ptr %lx", | |
| bpf_dbg_printk("response_length %llu, offset %llu, resp_ptr %llx", |
(it's possible other parts of our code carry the same mistake, so I understand if you were just being consistent here)
There was a problem hiding this comment.
Thank you, corrected
| @@ -45,6 +45,7 @@ typedef struct http_request_trace_t { | |||
| u16 status; | |||
| connection_info_t conn __attribute__((aligned(8))); | |||
| s64 content_length; | |||
| s64 response_length; | |||
There was a problem hiding this comment.
should this be u64, or is the original value being read signed?
There was a problem hiding this comment.
Yes, the original value can be a sign value, -1 is returned when the length is unknown
bpf/headers.go
Outdated
| @@ -2,4 +2,4 @@ | |||
|
|
|||
| package bpf | |||
|
|
|||
| import _ "github.com/grafana/beyla/v2/bpf/headers" | |||
| import _ "github.com/grafana/beyla/v2/bpf" | |||
There was a problem hiding this comment.
Sorry about that - hopefully you can rebase once #1808 has been merged
There was a problem hiding this comment.
Not a problem, I pulled up the actual main :)
|
|
|
👍 for the minimal docs additions. |
|
I see you're still having issues with clang-format, what version are you using? We are using clang-format-19. The error probably stems from some implicit configuration that we should make explicit (not the scope of this PR though). |
|
@rafaelroquetto yes, I think I was previously running clang-format with version 20, now I installed llvm@19 and the formatting seems to work fine, thanks :) |
|
The precommit hook with a different version of clang replaced my changes and I didn't notice |
There was a problem hiding this comment.
Amazing @almostinf ! Thank you so much for this, top notch contribution!
Added response body size handling in ebpf/metrics/traces
TODO:
Closes #1286