-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Go gGPC probes for metrics and traces #46
Conversation
Codecov Report
@@ Coverage Diff @@
## main #46 +/- ##
==========================================
+ Coverage 50.30% 51.24% +0.94%
==========================================
Files 14 14
Lines 833 927 +94
==========================================
+ Hits 419 475 +56
- Misses 378 415 +37
- Partials 36 37 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
If you want to know why integration tests are failing, remember that here you have the docker compose logs from the last run: https://github.com/grafana/ebpf-autoinstrument/actions/runs/4662697238?pr=46 The instrumenter is failing in the test-suite-nodebug logs:
Probably you can fix the tests by adding here the required GRPC fields and running For example (but adding your required structs and fields: "google.golang.org/grpc": {
"versions": ">= 1.3",
"fields": {
"google.golang.org/grpc/internal/transport.Stream": [
"method",
"id",
"ctx"
],
"google.golang.org/grpc.ClientConn": [
"target"
],
"golang.org/x/net/http2.MetaHeadersFrame": [
"Fields"
],
"golang.org/x/net/http2.FrameHeader": [
"StreamID"
]
}
} |
#define EVENT_HTTP_REQUEST 1 | ||
#define EVENT_GRPC_REQUEST 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that, for now, it's fine. But once we merge the PR and make sure everything is working, we could refactor a bit to provide two eBPF instrumenters separately (one for HTTP and another for GRPC, in different files), and communicate both with the userspace via separate ringbuffers, so we don't need to differentiate the event type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree, I'll refactor this in a follow-up. After we successfully merge the goroutine PR.
It looks great, thank you! In addition to fixing the integration tests by adding prefetched indices for the GRPC fields, I'd also add a new integration test for GRPC. For example, adding this GRPC server to the |
After I pushed the fix for the eBPF verification error reporting, I realized that whatever we were hitting there must've been an eBPF loader issue, perhaps related to the host OS kernel version. It was claiming a call to bpf_printk was unknown?!
We can see it's trying to generate the code for
is the log level check, so the call must be bpf_printk. For some reason earlier calls to bpf_dbg_printk work just fine. To fix the problem, I simply removed those prints. I see no issues when running locally, so it must be kernel specific. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Good job!
Thanks Mario! |
This PR adds support for tracking gRPC calls with the following parameters:
The current support relies of us having DWARF symbols for the various fields.
Relates to #20