feat: support JSON-RPC over HTTP request#2008
feat: support JSON-RPC over HTTP request#2008titaneric wants to merge 28 commits intografana:mainfrom
Conversation
grcevski
left a comment
There was a problem hiding this comment.
This is a great start @titaneric ! Thanks for putting this together. I left a few comments/questions.
| } | ||
|
|
||
| var obj JSONRPCRequest | ||
| if err := json.Unmarshal([]byte(body), &obj); err != nil { |
There was a problem hiding this comment.
I wonder if we can make this more solid. One problematic example would be if the request JSON doesn't fit in 256 bytes, then I think the unmarshal call will fail with error. What do you think about looking for the method and version by string search?
There was a problem hiding this comment.
Sounds reasonable, matching the method and version may be relatively robust way to match partitioned JSON-RPC in request body
There was a problem hiding this comment.
Hey @titaneric, thanks for bringing this up. Adding to what was said above, I think this should be done at the ebpf level if feasible (just in case that is not already the idea), as there's non-negligible overhead in shipping non-trivial amounts of data to userspace, allocating strings and instantiating a json parser.
Since you are already reading the body and the content types in go_nethttp.c, there are few things I reckon you could do:
- when reading
content_type, you can simply check on the spot if it matches one of the valid types and instead of storing the string, you can store an enum value (such asJSON) - later when reading the body, you can perform some sort of string search or parsing as @grcevski is suggesting - it's not as trivial in ebpf, but we have a few examples scattered across the code that can serve as inspiration, for instance, in https://github.com/grafana/beyla/blob/main/bpf/common/tc_common.h there are a few functions we use for looking things up - in your case, the good news is that you know the buffer sizes at compile time, so that will keep the ebpf verifier happy.
You can then ultimately fill in the method and any other info you may have directly from the ebpf layer, leaving the http_request_trace_t untouched.
Feel free to reach out on the Slack channel if you need further help with this. Happy to assist :)
There was a problem hiding this comment.
May I introduce new event type for JSON-RPC over HTTP, or I could simply reuse http event and overwrite the method / error code?
There was a problem hiding this comment.
I'd say whatever is the easiest (provided that we don't end up doing too many gymnastics in the code)- what do you think?
There was a problem hiding this comment.
I wound try to reuse current http event type, overwrite the method and status code inside the bpf.
There was a problem hiding this comment.
I detect the JSON-RPC in bpf and overwrite the method for http event type successfully.
Present implementation works, and I would like to try to detect JSON-RPC and extract method in single pass function.
7bb1909 to
8bdfd90
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2008 +/- ##
==========================================
+ Coverage 69.48% 72.30% +2.81%
==========================================
Files 182 186 +4
Lines 20162 20474 +312
==========================================
+ Hits 14010 14804 +794
+ Misses 5395 4881 -514
- Partials 757 789 +32
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:
|
rafaelroquetto
left a comment
There was a problem hiding this comment.
Great stuff! I did another pass, will dig deeper into it after we iterate. I also triggered the tests, it's mostly looking good - we'll see if OATS pass, and if you need any help ping me on Slack!
| while (val_search_start < body_len && | ||
| (body[val_search_start] == ' ' || body[val_search_start] == '\t' || | ||
| body[val_search_start] == '\n' || body[val_search_start] == ':')) { | ||
| val_search_start++; | ||
| } |
There was a problem hiding this comment.
likewise here you could reuse the function I suggested above.
I wonder if we can further reduce this to:
// returns the start of value (i.e. after the key and whitespace) inside 'body`, NULL otherwise
const char *value = json_rpc2_value(key, key_size);
const char *value_end = jsonrpc2_value_end(value);
value_end can use memchar or find_first_pos_of in tc_common.h.
There was a problem hiding this comment.
That's a good idea!
There was a problem hiding this comment.
I don't think we no longer need json_value_offset to skip space and colon since we have json_str_value function, but we could keep it for possible usage to match not only string type but any types as well?
| u8 path[PATH_MAX_LEN]; | ||
| u8 _pad[5]; | ||
| u64 body_addr; // pointer to the body buffer | ||
| u8 content_type[HTTP_CONTENT_TYPE_MAX_LEN]; |
There was a problem hiding this comment.
it's best to move body_addr under status - and content_type after path - the idea here is to reduce padding altogether
There was a problem hiding this comment.
I think we still need 5 additional bytes padding in the end after moving it.
May we increase the METHOD_MAX_LEN from 7 bytes to a higher value such as 12 or 16?
There was a problem hiding this comment.
Please note that in this PR I set JSONRPC_METHOD_BUF_SIZE to 16.
|
For the OATS tests, it may be related to #2028 - so you may wanna rebase (or merge) |
b17ce3b to
3d3e5a6
Compare
|
Please give me some time to apply the patch into https://github.com/open-telemetry/opentelemetry-ebpf-instrumentation/tree/main, and I would raise the PR there. |
|
Thank you so much! |
|
The code that this PR is touching is moved to https://github.com/open-telemetry/opentelemetry-ebpf-instrumentation and Beyla vendors it, so this is code is not here anymore. Please re-open the PR in that repository if this still relevant. Thanks and sorry for the inconveniences. |
This is golang's uprobe implementation for #1976 .
I try to capture http request body by introducing new hook for
net/http.(*body).Readfunction.The body struct is initialized in the readTransfer in readRequest.
readTransfer prepare the body reader, and body's
io.Readerimplementation is here.Note that I capture partial http request body and retrieve the
Content-Typein bpf, and send them into userspace.Test JSON-RPC code:
Sample request
curl -X POST http://localhost:8080/jsonrpc \ -H "Content-Type: application/json" \ -d '{ "jsonrpc": "2.0", "method": "Arith.Multiply", "params": [{"A": 7, "B": 8}], "id": 1 }'Output:
I hope that present direction is correct for the implementation. I would like to hear more discussion and feedback, and I would continue the work for response body capturing, error code handling, etc.