Conversation
hawkw
left a comment
There was a problem hiding this comment.
Overall, this mostly looks good to me. I had a couple small suggestions.
Thanks for working on it!
| match self.task_id_mappings.entry(span_id) { | ||
| Entry::Occupied(entry) => *entry.get(), | ||
| Entry::Vacant(entry) => { | ||
| let task_id = self.task_id_counter; | ||
| entry.insert(task_id); | ||
| self.task_id_counter = self.task_id_counter.wrapping_add(1); | ||
| task_id | ||
| } | ||
| } |
There was a problem hiding this comment.
nit/TIOLI: I think this match could be replaced with Entry::or_insert_with
| match self.task_id_mappings.entry(span_id) { | |
| Entry::Occupied(entry) => *entry.get(), | |
| Entry::Vacant(entry) => { | |
| let task_id = self.task_id_counter; | |
| entry.insert(task_id); | |
| self.task_id_counter = self.task_id_counter.wrapping_add(1); | |
| task_id | |
| } | |
| } | |
| self.task_id_mappings | |
| .entry(span_id) | |
| .or_insert_with(|| { | |
| let task_id = self.task_id_counter; | |
| entry.insert(task_id); | |
| self.task_id_counter = self.task_id_counter.wrapping_add(1); | |
| task_id | |
| }) |
There was a problem hiding this comment.
This was actually the first thing I tried, but unfortunately the borrow checker is not happy about the mutation inside the closure 😕 I'm going to do a bit more reading to see if there are any other idiomatic ways.
error[E0500]: closure requires unique access to `self` but it is already borrowed
--> console-subscriber/src/aggregator.rs:449:62
|
449 | *self.task_id_mappings.entry(span_id).or_insert_with(|| {
| --------------------- -------------- ^^ closure construction occurs here
| | |
| | first borrow later used by call
| borrow occurs here
450 | let task_id = self.task_id_counter;
451 | self.task_id_counter = self.task_id_counter.wrapping_add(1);
| -------------------- second borrow occurs due to use of `self` in closure
There was a problem hiding this comment.
oh, huh, never mind — the current approach is fine, then, if the borrow checker doesn't like this.
I think we could "split" the borrows, like this:
| match self.task_id_mappings.entry(span_id) { | |
| Entry::Occupied(entry) => *entry.get(), | |
| Entry::Vacant(entry) => { | |
| let task_id = self.task_id_counter; | |
| entry.insert(task_id); | |
| self.task_id_counter = self.task_id_counter.wrapping_add(1); | |
| task_id | |
| } | |
| } | |
| let task_id_mappings = &mut self.task_id_mappings; | |
| let task_id_counter = &mut self.task_id_counter; | |
| task_id_mappings | |
| .entry(span_id) | |
| .or_insert_with(|| { | |
| let task_id = *task_id_counter; | |
| entry.insert(task_id); | |
| *task_id_counter = task_id_counter.wrapping_add(1); | |
| task_id | |
| }) |
but this is the same number of lines as the match version, and i'm not sure if it's meaningfully clearer. so, it may not be worth bothering...
proto/tasks.proto
Outdated
| message TaskDetails { | ||
| // The task's ID which the details belong to. | ||
| common.SpanId task_id = 1; | ||
| uint64 task_id = 1; |
There was a problem hiding this comment.
i have a slight preference for continuing to have a semantically-named newtype for these, although i agree they shouldn't be SpanIds any longer. I would consider adding a TaskId message type in this file.
not a big deal though.
| task_id_counter: TaskId, | ||
|
|
||
| /// A table that contains the span ID to pretty task ID mappings. | ||
| task_id_mappings: HashMap<span::Id, TaskId>, |
There was a problem hiding this comment.
a possible optimization for this is adding a custom Hasher implementation that always just calls span::Id::into_u64 and returns that value as the hash, rather than actually hashing them. but, we can do that in a follow-up...especially because I don't really think the performance impact is particularly significant.
hawkw
left a comment
There was a problem hiding this comment.
okay, this looks good to me, thanks for switching to a newtyped TaskId. most of my other suggestions can be addressed as follow-ups.
Currently, `console-subscriber` contains a bunch of machinery for rewriting non-sequential `span::Id`s from `tracing` to sequential IDs (see #75). Upon thinking about this for a bit, I don't actually understand why this has to be done on the instrumentation-side. This seems like extra work that's currently done in the instrumented application, when it really doesn't have to be. Instead, the client should be responsible for rewriting `tracing` IDs to pretty, sequential user-facing IDs. This would have a few advantages: - it moves some work out of the application, which is always good - if data is being emitted through an implementation other than `console-subscriber`, we will *still* get nicely ordered ids - this also makes some of the stuff i'm working on in #238 easier This branch removes ID rewriting from `console-subscriber`, and adds it to the `console` CLI's `state` module. Closes #240 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Move away from travis
Closes #61
Subscriber
Console
Task::id_hex()