refac(console): use a newtype to separate ID types #358
Conversation
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
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.
|
cc @jswrenn |
hawkw
added a commit
that referenced
this pull request
Jul 6, 2022
Currently, there's a bit of repeated boilerplate code for processing updates to the stored state for tasks, resources, and async ops, such as tracking a list of new items since the last update, and handling ID remapping. This PR builds upon the refactor in #358, and replaces the `state::id` module with a new `state::store` module. This module contains all of the old code for ID remapping along with a new `Store` type that implements a store of objects by ID. The `Store` type now implements much of the boilerplate code that was previously repeated in the `state::tasks`, `state::resources`, and `state::async_ops` modules.
hawkw
added a commit
that referenced
this pull request
Jul 13, 2022
Currently, there's a bit of repeated boilerplate code for processing updates to the stored state for tasks, resources, and async ops, such as tracking a list of new items since the last update, and handling ID remapping. This PR builds upon the refactor in #358, and replaces the `state::id` module with a new `state::store` module. This module contains all of the old code for ID remapping along with a new `Store` type that implements a store of objects by ID. The `Store` type now implements much of the boilerplate code that was previously repeated in the `state::tasks`, `state::resources`, and `state::async_ops` modules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The console TUI's state model deals with two different types of IDs:
tracingspan IDs for an object (task, resource, or asyncop), which are not assigned sequentially, and may be reused over
the program's lifetime, and
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
Idtype,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_formethodtakes 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
u64s. This isunfortunate, 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
Noneeven ifthere is a task with that span ID. PR #251 fixed one such ID-confusion
issue (where
WatchDetailsRPCs were performed with local, rewrittentask 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
statemodule to make ID confusionissues much harder to write. This is accomplished by adding an
Idnewtype to represent our rewritten sequential IDs.
Ids::id_fornowstill takes a
u64(as the remote span IDs are represented asu64s inprotobuf, so there's nothing we can do), but it returns an
Idnewtype.Looking up or inserting objects in a state map now takes the
Idnewtype. 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
Idsmap for one.Additionally, the
Idtype has a type parameter for the type of objectit identifies. This prevents additional issues where we might look up
the ID of a task in the map of resources, or similar.