Added area for the client callstack to the UI #33
Conversation
The tester consists of three parts: grpc-tester, grpc-generator and grpc-server. Tester is a library which enables units tests to start and stop the testing framework. The generator sends messages to server and server will collect statistics of the messages it receives. This initial implementation does not allow injecting the proxide proxy between the grpc-generator and grpc-server. This functionality will be added in another pull request. #### grpc-tester The tester is a Rust library with an interface for starting and tying the generator and server together. During the startup it ensures the connection between the generator and the server are ready before returning the tester to ensure unit tests utilizing the tester won't experience random timing related failures. Both the generator and the server are started in their own tokio runtimes to isolate the runtime used for testing. The threads associated with the generator and the server have named threads so that they can be distinguished in the debugger. #### grpc-generator The generator periodically sends gRPC requests (SendMessage) to the server. The purpose of the generator is to provide a constant stream of message that can be observed with the proxide proxy. #### grpc-server The server's main purpose is to receive and then ignore the messages it receives. A support for retrieving statistics of the received messages will be added in the future.
The generator now takes "tasks" parameter which tells it how many tasks it should launch. The server counts the number of send_message calls it has processed and the generator periodically retrieves this value and reports how many such calls have been processed in one millisecond. The goal of this is to enable performance assessment of the proxide proxy. Includes linting support in the pipeline.
The first unit test for proxide is now implemented with the help of the grpc_tester library which enables the unit test to start and stop gRPC server and gRPC message generation client. The new server and generator allows the test to check whether proxide is able to intercept messages. In this first iteration the test only verifies that this basic principle works.
This step prepares for the client stack capture support implementation for the proxide proxy. The goal is for proxide to capture the callstacks of the threads of client process when the traffic from the client goes through the proxide. This enables analysis of the client application behavior when unexpected calls pass through the proxy. Functionality will be available when the proxide proxy is on the same host machine as the client.
The area becomes visible if the client included both the process id and the thread id of the calling thread in the headers of a request. Support for capturing and displaying the callstack will be added in another commit.
There was a problem hiding this comment.
Looks good! I didn't dig that deep into the test rust_grpc crate code, but I did skim through it. The proxide changes themselves looked good enough. Most of the comments are mostly related to nice to have ideas we could implement, but I'm happy enough with the current code as well.
| match rust_grpc_private::test_service_client::TestServiceClient::connect( | ||
| self.http(), | ||
| ) | ||
| .await |
There was a problem hiding this comment.
Use (even if function-local use) for rust_grpc_private::test_service_client::TestServiceClient would be better here, as that should then keep this all to one line. The current code layout is quite confusing, as there is a match, with parenthesis, and then await, and only then the actual match body starts.
| id: c.id, | ||
| threads: c.threads, | ||
| }) | ||
| .collect(), |
There was a problem hiding this comment.
Isn't this just a diagnostics.clients.clone()?
I was originally thinking there were two different ClientProcess types and this did a proto-ClientProcess-to-pure-Rust-ClientProcess conversion, but that's not the case, is it?
| let vertical_chunks: Vec<Rect> = if ClientThreadId::try_from(&request.request_msg).is_ok() { | ||
| Layout::default() | ||
| .direction(Direction::Vertical) | ||
| .margin(0) | ||
| .constraints([Constraint::Percentage(80), Constraint::Percentage(20)].as_ref()) | ||
| .split(block.inner(c)) | ||
| } else { | ||
| Vec::from([block.inner(c)]) | ||
| }; |
There was a problem hiding this comment.
At some point we should have some kind of a kill switch (either implicit or explicit) for the call stack mechanism.
It's not a problem if we just show the client-reported values, but once we start capturing the actual call stack stack frames by pausing the process, we should be careful we don't allow arbitrary processes to tell Proxide to pause (and capture the stack trace) from other processes. This is especially important, if Proxide binds itself to public network interface and allows connections from non-local clients. I think it might be sensible to enable the call stack capture only when Proxide is bound to a local-only interface (at least by default).
|
|
||
| // The right side view is split vertically only if the client included its process id and thread id in the request | ||
| // enabling the callstack capture. | ||
| if vertical_chunks.len() > 1 { | ||
| CallstackView { | ||
| request: request.request_data.uuid, | ||
| offset: 0, | ||
| } | ||
| .draw(ctx, f, vertical_chunks[1]); | ||
| } |
There was a problem hiding this comment.
Going forward, it might make sense to give all rects a name.
As in, after split, acquire references to individual rects:
let (upper_chunk, maybe_callstack_chunk) = if ... {
let split = Layout::default()...split(block.inner(c))
(split[0], Some(split[1]))
} else {
(block.inner(c), None)
};
let (request_chunk, response_chunk) = {
let split = Layout::default()...split(upper_chunk);
(split[0], split[1])
};That would allow us to use named chunks here in the draw-calls instead of (somewhat) arbitrary [0] and [1] indexes.
Especially I'd like the option of replacing the if here with:
if let Some(callstack_chunk) = maybe_callstack_chunk {
/* draw */
}
The area becomes visible if the client included both the process id and the thread id of the calling thread in the headers of a request. Support for capturing and displaying the callstack will be added in another commit.