refactor: Use u64 task IDs

[oguzbilgener]

Closes #61

Subscriber

• Assign sequential ID numbers to tasks, keep a HashMap of span ID --> task ID.
• Change the task id type to uint64 in protos.

Console

• Use the u64 task IDs, display them in decimal.
• Remove now-unused Task::id_hex()
• Shrink the TID column to a minimum of 4 chars.

[hawkw]

Overall, this mostly looks good to me. I had a couple small suggestions.

Thanks for working on it!

[hawkw]

nit/TIOLI: I think this match could be replaced with Entry::or_insert_with

Suggested change

	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
	})

[oguzbilgener]

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

[hawkw]

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:

Suggested change

	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...

[hawkw]

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.

[hawkw]

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]

okay, this looks good to me, thanks for switching to a newtyped TaskId. most of my other suggestions can be addressed as follow-ups.