Repository navigation
tp: allow the HTTP RPC accelerator to open more than 1 file - #2615
WangJialin547 wants to merge 125 commits into
Conversation
|
So this is cool but definitely not sufficient for two reasons:
Landing this change requires with fixes for both 1) and 2). |
|
I had a looked a the proposed fix and I'm reasonably sure it's not going to work if the lid is closed for a few hours (the laptop will wake up every now and again but likely will not connect within the timeout). Sketching out how I expect a solution to work like:
So TLDR the solution has to be significantly more complex than just changing the rpc layer: it will need changes to the UI as well: this is unavoidable. @sashwinbalaji fyi. |
|
both issues 1 and 2 have been addressed according to the instructions. this current modification allows the user to open multiple files using the trace processor as the backend establishes multiple trace processors according to the connections, and the "trace processor is already being used" modal is updated so the user can choose the desired file. However there is a limitation that each trace processor remains limited to only one tab at a time. in order for a single trace processor to be accessible for multiple tabs i need to remove the rpc sequence mismatch check and update the restoreInitialTables so the TP doesn't clear tables when it restarts and both tabs can access their respctive tabes, but i'm not sure if that's allowed |
We definitely don't want to support that the same TP instance is accessible from multiple tabs at the same time. So we should keep the sequence checks and the restore initial state. @sashwinbalaji will take care of reviewing this. |
|
Hi @stevegolton , just checking in if you had the time to take a look at this PR. Appreciate it! |
|
@sashwinbalaji can you PTAL at this? |
|
@WangJialin547 @stevegolton is OOO this week. We'll follow up with him next week on this. |
|
@sashwinbalaji I think Lalit just means can you just confirm the TP part - I'll look at the UI part. I'm actually back tomorrow (returning early from vacation) so I can take a look at the UI part again either tomorrow or Friday. Apologies for the late reply but there's a lot going on in the UI so unfortunately some things are taking precedence at the moment. The unfortunate bit about this PR IIRC is that it touches and changes the logic of a core part of the UI codebase that was quite complex to begin with and it deserves more time than I've been able to give to it recently. |
stevegolton
left a comment
There was a problem hiding this comment.
Thanks for making the changes - I added a few more comments.
In general I'm not super convinced about the way the rpc engine now needs a whole bunch more logic to obtain the traceProcessorId when requesting a new instance. It feels like the REST interface you have to look up the trace processor instances would be the ideal place where clients can request new instances - as it already acts as a higher level meta / management API on top of the low-level RPC websocket streaming API. Could we not add a POST endpoint here that allows us to create a new trace processor, returning the instance id, then we can just connect straight to it from the engine?
| if (traceTitle) { | ||
| engine.sendTraceTitle(traceTitle); | ||
| } | ||
| return; |
| break; | ||
| case 'HTTP_RPC': | ||
| break; | ||
| default: |
There was a problem hiding this comment.
Nit: Can you add an assertUnreachable() here in the default statement please? That'll make sure at the typescript level that we cover all bases in the switch/case
E.g.
default:
assertUnreachable(traceSource);
| switch (traceSource.type) { | ||
| case 'FILE': | ||
| // Split on both \ and / (because C:\Windows\paths\are\like\this). | ||
| traceTitle = traceSource.file.name.split(/[/\\]/).pop()!; |
There was a problem hiding this comment.
Nit: It would be nice to dedupe this logic with above, but I'm not going to block on this.
| @@ -220,6 +220,10 @@ async function loadTraceIntoEngine( | |||
| engine, | |||
| ); | |||
|
|
|||
| if (engine instanceof HttpRpcEngine) { | |||
There was a problem hiding this comment.
Using instanceof in a runtime conditional feels like a bit of a code smell. Can't we just set the trace title for all engines regardless?
Also what happens if we set the trace title for an HttpRpcEngine where a trace was already loaded by the user from the command line - i.e. when traceStream is undefined
| @@ -34,43 +35,64 @@ export class HttpRpcEngine extends EngineBase { | |||
| private disposed = false; | |||
| private queue: Blob[] = []; | |||
| private isProcessingQueue = false; | |||
| private traceProcessorId = 0; | |||
There was a problem hiding this comment.
Nit: This is always set in the constructor so there's no point in initializing it here as well.
| } else { | ||
| this.requestQueue.push(data); // onWebsocketConnected() will flush this. | ||
| } | ||
| } | ||
|
|
||
| private onWebsocketConnected() { | ||
| onWebsocketConnected() { |
There was a problem hiding this comment.
Does this need to be public?
| // again once the instance ID arrives. | ||
| if (this.isWaitingForid) return; | ||
|
|
||
| this.connected = true; |
There was a problem hiding this comment.
Did this line need to be moved?
| this.fail(`${rpc.fatalError}`); | ||
| } | ||
| if (rpc.response !== TPM.TPM_GET_STATUS) { | ||
| this.fail(`Initial message missing instance ID: ${rpc}`); |
There was a problem hiding this comment.
Since this.fail throws, you could omit the else if logic below and just put everything in its own if condition. I was initially confused as the importance of the use of else but it doesn't look like it does anything.
|
Hi @stevegolton , thank for the feedback! I am currently working on it but I wanted to make sure what is expected here before proceeding: The Websocket implementation follows the design document sent by @primiano in this writeup which specifies to implement 2 different "/websocket" endpoints depending on whether you want to create or attach to a TP instance. If the team has decided to change that to a POST endpoint I am happy to implement that change but I just like to confirm this is what the team wants to prevent another round of back and forth here. |
Hey @WangJialin547, sorry for the inconsistency. We will discuss internally (today hopefully) and get back to you. |
|
First of all, huge apologies from our end for the delay and multiple back and forth. We did a post-mortem on how to avoid these kinds of issues on external contributions and have come up with a process to handle them (will share soon in a separate discussion). So, coming to this CL, we concluded that the rpc_engine should not handle any logic. The UI dialog you have built is great. For the backend, we'd like to go with a different approach, somewhat similar to your previous implementation change.
Please keep the deletion/cleanup logic. Once again, thank you for your responsiveness and your contribution. |
|
Hi @WangJialin547, Are you still actively working on this? As you know this is a critical core feature, we are very eager to land it soon. If you're busy, we're happy to take this over the finish line, making sure you get full credit for your significant contributions in the commit history. Please let us know your preference. Thanks again for all your hard work on this PR. |
|
Hi there, thanks for checking in. I indeed am still actively working on this and my preference is that I still want to finish this PR myself. Right now I’m in the middle of an unusually heavy academic period in college, with two major exams and multiple important deadlines next week (11/17 - 11/23), but I will be able to start making meaningful commits after that. If the project’s timeline can accommodate the delay, I’d really appreciate the chance to complete the remaining changes myself. If waiting causes issues, I understand, but my strong preference is to continue owning the PR. Thank you again for your patience and understanding, and I will start working on this as soon as time allows. |
|
Sure, take your time and please ensure it aligns with #2615 (comment). We are always happy to have external contributions. |
|
Hi @sashwinbalaji , Here's what I have made so far. I have addressed the issues stated in #2615 (comment) , including replacing the integer instaceID to UUID and moved the UUID generation logic from the backend to the UI. I have 2 questions regarding how to move forward:
In my current implementation, for backward comp I loaded it into the "global" rpc as requested in the comment since it doesn't have an associated UUID. However, due to the strcture of /status protos, this means that it can't: Should I leave it like this or implement some modifications?
--- Presubmit Errors Found ---
Thank you again for your time. (I know there are still some undeleted comments in the source files, I will take care of them ASAP once I know the directions to resolve these issues) |
|
Thanks for picking up the task back.
Please load it into a separate thread. You can update RunHttpRPCServer to take a preloaded tp instance and plumb it to RpcThread. RunHttpRPCServer can also handle the uuid creation and updating the map as needed.
try to manually revert these changes by running : Do ping us when the CL is in a state for review, thanks. |
|
Hi, I have addressed the comments and incorporated the requested changes. Some explanations to specific design choices:
Also, apologies for the slow update, it was finals week at my college so I didn't have the time to update this PR. I am back to normal now because school is over and I'm open to any further revisions. |
|
@WangJialin547 thank you for getting back to this patch. Most of the team (including @sashwinbalaji) have left for the Chrismas break so we will get back to reviewing this patch in the new year. Thanks again for your continued contribution and patience. |
sashwinbalaji
left a comment
There was a problem hiding this comment.
Thanks for your continued work on this.
Added some initial comments for now. To test it more please rebase your change and resolve the conflicts can be verified further.
| // trace_processor_shell -D trace_file.pftrace . | ||
| // If present and not empty, a trace is already loaded already in one or more | ||
| // trace processors. This happens when using the HTTP+RPC mode and passing a | ||
| // trace file to the shell, via trace_processor_shell -D trace_file.pftrace . |
| optional uint32 instance_id = 5; | ||
| optional uint64 inactivity_ns = 6; | ||
| optional bool is_attached = 7; | ||
| optional string instance_uuid = 8; |
There was a problem hiding this comment.
please add some description comments.
| std::string metric_v1_names; | ||
| std::string metric_v1_output; | ||
| std::vector<std::string> raw_metric_v1_extensions; | ||
| size_t tp_timeout_mins = 24 * 60; // 24 hours |
| RunHttpRPCServer( | ||
| /*rpc=*/rpc, | ||
| /*preloaded_tp=*/rpc.TakeTraceProcessor(), | ||
| /*is_preloaded_eof-*/ !options.trace_file_path.empty(), |
There was a problem hiding this comment.
instead of 2 separate params (preloading tp and is_preloaded_eof) how about just continuing with passing rpc and exposing a getter in rpc to get eof_ ?
|
|
||
| // New endpoint, returns data in batches using chunked transfer encoding. | ||
| // The batch size is determined by |cells_per_batch_| and | ||
| // The batch size is determined by |cells_per_batch_| |
| closed_conn_ids_; // if you don't keep a list of the UUIDs of closed | ||
| // connections, the frontend will confuse it with | ||
| // closing the laptop lid and try to reopen them, | ||
| // causing a new thread to be reopened immediately. | ||
| // when you make a new websocket handshake, check this | ||
| // set to see if it was previously closed, and if so, | ||
| // refuse to reopen it. |
There was a problem hiding this comment.
comment styling refer to others in the file.
| if (closed_conn_ids_.find(instance_uuid) != closed_conn_ids_.end()) { | ||
| PERFETTO_ILOG("Refusing to reopen previously closed TP instance %s", | ||
| instance_uuid.c_str()); | ||
| return conn.SendResponseAndClose("410 Gone", {}); |
| void Httpd::OnHttpConnectionClosed(base::HttpServerConnection* conn) { | ||
| std::lock_guard<std::mutex> lock(websocket_rpc_mutex_); | ||
| auto conn_to_id_it = conn_to_id_map.find(conn); | ||
| if (conn_to_id_it != conn_to_id_map.end()) { |
There was a problem hiding this comment.
use early returns to reduce some nesting
| // without warning. | ||
| optional string version_code = 4; | ||
|
|
||
| optional uint32 instance_id = 5; |
There was a problem hiding this comment.
instance_id is not used, i prefer keeping it rather than introducing a string instance_uuid
| std::string tp_uuid = entry.first; | ||
| const auto& tp_rpc = entry.second; |
There was a problem hiding this comment.
can we name the variable tp_rpc better please, maybe tp_rpc_thread as tp_rpc->rpc_ is confusing and also you can use structure bindings to fetch them both
|
@WangJialin547 how is it going? What's the status of this PR? Are you still interested in completing it? |
|
Catching up to see, if you are working on this PR ? If not, I can take it over and ensure you get the authorship for the change. |
|
Hi @sashwinbalaji , thanks for checking in. I’ve spent quite a bit of time working on this PR, but I’m currently too busy to finish it properly, so I think it would be the best if you take over. I really appreciate you offering to preserve my authorship for the change. I’m happy to share any notes to ensure smoother handoff, if needed. Thanks again! |
Added a feature that solves the issue that at most one tab can be using the http + rpc accelerator.
Revised #2597 and made the change focused and minimal