-
-
Notifications
You must be signed in to change notification settings - Fork 148
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
refac(console): use a newtype to separate ID types (#358)
The console TUI's state model deals with two different types of IDs: - the actual `tracing` span IDs for an object (task, resource, or async op), which are *not* assigned sequentially, and may be reused over the program's lifetime, and - sequential "pretty" IDs assigned by the console, which are mapped to span IDs when an object is sent to the console. These are assigned based on separate counters for each type of object (so there can be both a task 1 and a resource 1, for example). Remote span IDs are mapped to rewritten sequential IDs by the `Id` type, which stores a map of span IDs to sequential IDs, and generates new sequential IDs when a new span ID is recorded. The `Ids::id_for` method takes a span ID and returns the corresponding sequential ID, and this must be called before looking up or inserting an object by its remote span ID. Currently, *all* of these IDs are represented as `u64`s. This is unfortunate, as it makes it very easy to accidentally introduce bugs where the wrong kind of ID is used. For example, it's possible to accidentally look up a task in the map of active tasks based on its span ID on the remote application, which is likely to return `None` even if there is a task with that span ID. PR #251 fixed one such ID-confusion issue (where `WatchDetails` RPCs were performed with local, rewritten task IDs rather than the remote's span ID for that task). However, it would be better if we could just *not have* this class of issue. This branch refactors the console's `state` module to make ID confusion issues much harder to write. This is accomplished by adding an `Id` newtype to represent our rewritten sequential IDs. `Ids::id_for` now still takes a `u64` (as the remote span IDs are represented as `u64`s in protobuf, so there's nothing we can do), but it returns an `Id` newtype. Looking up or inserting objects in a state map now takes the `Id` newtype. This ensures that all lookups are performed with sequential IDs at the type level, and the only way to get a sequential ID is to ask the `Ids` map for one. Additionally, the `Id` type has a type parameter for the type of object it identifies. This prevents additional issues where we might look up the ID of a task in the map of resources, or similar.
- Loading branch information
Showing
6 changed files
with
167 additions
and
63 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| use std::{ | ||
| any, cmp, | ||
| collections::hash_map::{Entry, HashMap}, | ||
| fmt, | ||
| hash::{Hash, Hasher}, | ||
| marker::PhantomData, | ||
| }; | ||
|
|
||
| pub(crate) struct Ids<T> { | ||
| next: u64, | ||
| map: HashMap<u64, Id<T>>, | ||
| } | ||
|
|
||
| /// A rewritten sequential ID. | ||
| /// | ||
| /// This is distinct from the remote server's span ID, which may be reused and | ||
| /// is not sequential. | ||
| pub(crate) struct Id<T> { | ||
| id: u64, | ||
| _ty: PhantomData<fn(T)>, | ||
| } | ||
|
|
||
| // === impl Ids === | ||
|
|
||
| impl<T> Ids<T> { | ||
| pub(crate) fn id_for(&mut self, span_id: u64) -> Id<T> { | ||
| match self.map.entry(span_id) { | ||
| Entry::Occupied(entry) => *entry.get(), | ||
| Entry::Vacant(entry) => { | ||
| let id = Id { | ||
| id: self.next, | ||
| _ty: PhantomData, | ||
| }; | ||
| entry.insert(id); | ||
| self.next = self.next.wrapping_add(1); | ||
| id | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<T> Default for Ids<T> { | ||
| fn default() -> Self { | ||
| Self { | ||
| next: 1, | ||
| map: Default::default(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<T> fmt::Debug for Ids<T> { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.debug_struct("Ids") | ||
| .field("next", &self.next) | ||
| .field("map", &self.map) | ||
| .field("type", &format_args!("{}", any::type_name::<T>())) | ||
| .finish() | ||
| } | ||
| } | ||
|
|
||
| // === impl Id === | ||
|
|
||
| impl<T> Clone for Id<T> { | ||
| #[inline] | ||
| fn clone(&self) -> Self { | ||
| Self { | ||
| id: self.id, | ||
| _ty: PhantomData, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<T> Copy for Id<T> {} | ||
|
|
||
| impl<T> fmt::Debug for Id<T> { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| let path = any::type_name::<T>(); | ||
| let type_name = path.split("::").last().unwrap_or(path); | ||
| write!(f, "Id<{}>({})", type_name, self.id) | ||
| } | ||
| } | ||
|
|
||
| impl<T> fmt::Display for Id<T> { | ||
| #[inline] | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| fmt::Display::fmt(&self.id, f) | ||
| } | ||
| } | ||
|
|
||
| impl<T> Hash for Id<T> { | ||
| #[inline] | ||
| fn hash<H: Hasher>(&self, state: &mut H) { | ||
| state.write_u64(self.id); | ||
| } | ||
| } | ||
|
|
||
| impl<T> PartialEq for Id<T> { | ||
| #[inline] | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.id == other.id | ||
| } | ||
| } | ||
|
|
||
| impl<T> Eq for Id<T> {} | ||
|
|
||
| impl<T> cmp::Ord for Id<T> { | ||
| #[inline] | ||
| fn cmp(&self, other: &Self) -> cmp::Ordering { | ||
| self.id.cmp(&other.id) | ||
| } | ||
| } | ||
|
|
||
| impl<T> cmp::PartialOrd for Id<T> { | ||
| #[inline] | ||
| fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> { | ||
| Some(self.cmp(other)) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.