Init tracing #28
Init tracing #28
Conversation
kanin/src/app/task.rs
Outdated
| @@ -79,7 +80,11 @@ where | |||
| // Requests are handled and replied to concurrently. | |||
| // This allows each handler task to process multiple requests at once. | |||
| tasks.push(tokio::spawn(async move { | |||
| handle_request(req, handler, channel, should_reply).await; | |||
| let req_id = ReqId::extract(&mut req).await.expect("infallible"); | |||
There was a problem hiding this comment.
Actually I think if we're extracting a req_id for every request anyway, shouldn't we just extract it as part of Request::new and store it in the request? Then the Extract impl for ReqId can also be simplified to just take the stored ReqId from the request directly.
Or instead of storing it, at least just have a method on Request that gets the ReqId from the delivery inside, then the Extract impl could use that. But if we extract it every time anyway, maybe storing it isn't so bad? Any handler with an RPC client would be able to avoid extracting it twice then.
There was a problem hiding this comment.
Yeah, I def agree with that. Seems wild to extract it every time. Also a lot easier on the eyes to just write req.req_id().
kanin/src/app/task.rs
Outdated
| let app_id = properties | ||
| .as_ref() | ||
| .and_then(|p| p.app_id().as_ref()) | ||
| .map(|app_id| app_id.as_str()) | ||
| .unwrap_or("<unknown>"); |
There was a problem hiding this comment.
Kind of similar but I feel this should be a method in Request, so this can just be req.app_id().
There was a problem hiding this comment.
It's definitely prettier ;)
Co-authored-by: Victor Nordam Suadicani <[email protected]>
Co-authored-by: Victor Nordam Suadicani <[email protected]>
Co-authored-by: Victor Nordam Suadicani <[email protected]>
Co-authored-by: Victor Nordam Suadicani <[email protected]>
Co-authored-by: Victor Nordam Suadicani <[email protected]>
Co-authored-by: Victor Nordam Suadicani <[email protected]>
Co-authored-by: Victor Nordam Suadicani <[email protected]>
Co-authored-by: Victor Nordam Suadicani <[email protected]>
Co-authored-by: Victor Nordam Suadicani <[email protected]>
kanin/src/app/task.rs
Outdated
| "Handler {:?} produced response: {response:?}", | ||
| std::any::type_name::<H>() | ||
| ); | ||
| info!("Handler produced response: {response:?}"); |
There was a problem hiding this comment.
I removed the name of the handler here (it feels weird that it's printed twice when you can just use the req_id to deduct the name.
But I have a concern about printing the full response of every single request. For small responses it's no problem; but it doesn't seem right thing to do as the default for every service.
There was a problem hiding this comment.
info!("Handler produced response of {} bytes that will be published on {reply_to}", bytes_response.len());could be an option.
There was a problem hiding this comment.
There won't always be a reply_to. But maybe you can log in the different scenarios of reply_to/no reply_to
There was a problem hiding this comment.
How about this? :)
kanin/src/request.rs
Outdated
| @@ -25,6 +29,7 @@ impl Request { | |||
| Self { | |||
| state, | |||
| channel, | |||
| req_id: ReqId::from(&delivery), | |||
There was a problem hiding this comment.
I think the usage of the From trait here is slightly unnecessary (a simple from_delivery method would do) but it's not a big deal.
There was a problem hiding this comment.
I am actually not a super big fan of these impl's (as with the example with thiserror), so I'll be happy to change it :)
kanin/src/app/task.rs
Outdated
| @@ -25,6 +25,7 @@ use super::StateMap; | |||
| pub(super) type HandlerTask = Pin<Box<dyn Future<Output = String> + Send>>; | |||
|
|
|||
| /// Creates the handler task for the given handler and routing key. See [`HandlerTask`]. | |||
| #[tracing::instrument(skip_all, field(routing_key = routing_key))] | |||
There was a problem hiding this comment.
Didn't see this before. Won't this add yet another layer of a span? How does it look when printed? Is it too much?
There was a problem hiding this comment.
oh! I think this was just me debugging/exploring. It doesn't do anything because.. yeah.. I'm not sure tbh.
Co-authored-by: Victor Nordam Suadicani <[email protected]>
Co-authored-by: Victor Nordam Suadicani <[email protected]>
No description provided.