Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refac: move ID rewriting to console #244

Merged
merged 3 commits into from
Dec 28, 2021
Merged

refac: move ID rewriting to console #244

merged 3 commits into from
Dec 28, 2021

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Dec 27, 2021

Currently, console-subscriber contains a bunch of machinery for
rewriting non-sequential span::Ids 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:

This branch removes ID rewriting from console-subscriber, and adds it
to the console CLI's state module.

Closes #240

hawkw added 3 commits December 27, 2021 11:27
@hawkw
wip
2e6912d 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
finish subscriber change
072e457 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
move ID rewriting into the console
85995d3 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
zaharidichev
zaharidichev approved these changes Dec 28, 2021
View reviewed changes
Copy link
Collaborator

@zaharidichev zaharidichev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and yes this makes a lot of sense!

@hawkw hawkw merged commit 095b1ef into main Dec 28, 2021
@hawkw hawkw deleted the eliza/move-id-rewriting branch December 28, 2021 20:33
hawkw added a commit that referenced this pull request Jan 6, 2022
@hawkw
fix(console): don't make details requests with rewritten IDs
8e4a1fa 
PR #244 moved the rewriting of `tracing` span IDs to sequential
low-number IDs from the `console-subscriber` crate to the console CLI.
However, this introduced a bug with task details, because that PR didn't
change the part of the console that makes `watch_details` RPCs to use
the `tracing` span ID recieved from the remote --- it continued to use
the `Task` and `Resource` structs' `id` fields. These are now different
from the `tracing` ID recieved from the remote, because they're
rewritten in the console CLI. This means that `watch_details` RPCs would
generally recieve an error, or (in the off-chance that a rewritten ID
collides with a low-numbered `tracing` ID) the details for anoter task
or resource.

This branch fixes the bug by changing the `Task` and `Resource` structs
to store both the span ID received from the remote _and_ the rewritten
pretty ID. This way, when we make `watch_details` RPC calls, we do it
with the span ID we received from the remote process.

I also changed some naming to make the distinction between rewritten IDs
and span IDs clearer.
@hawkw hawkw mentioned this pull request Jan 6, 2022
Merged
hawkw added a commit that referenced this pull request Jan 7, 2022
@hawkw
fix(console): don't make details requests with rewritten IDs (#251)
4ec26a8 
## Motivation

PR #244 moved the rewriting of `tracing` span IDs to sequential
low-number IDs from the `console-subscriber` crate to the console CLI.
However, this introduced a bug with task details, because that PR didn't
change the part of the console that makes `watch_details` RPCs to use
the `tracing` span ID recieved from the remote --- it continued to use
the `Task` and `Resource` structs' `id` fields. These are now different
from the `tracing` ID recieved from the remote, because they're
rewritten in the console CLI. This means that `watch_details` RPCs would
generally recieve an error, or (in the off-chance that a rewritten ID
collides with a low-numbered `tracing` ID) the details for anoter task
or resource.

## Solution

This branch fixes the bug by changing the `Task` and `Resource` structs
to store both the span ID received from the remote _and_ the rewritten
pretty ID. This way, when we make `watch_details` RPC calls, we do it
with the span ID we received from the remote process.

I also changed some naming to make the distinction between rewritten IDs
and span IDs clearer.
@zaharidichev zaharidichev mentioned this pull request Jan 9, 2022
Merged
hawkw pushed a commit that referenced this pull request Jan 10, 2022
@zaharidichev
fix(console): fix task lookup in async ops view (#257)
9a50b63 
#244 moved rewriting of ids to the console. We have however forgot to look up
the pretty ID when trying to figure out which task is running a particular async op.
The result of that was that we were displaying the raw ID for the task and we were
not able to properly resolve the name of the task if specified.


Before: 
<img width="838" alt="Screenshot 2022-01-09 at 19 52 27" src="https://user-images.githubusercontent.com/4391506/148694237-1d9b995c-a99a-42d8-865d-2af638524582.png">

After: 
<img width="861" alt="Screenshot 2022-01-09 at 19 53 24" src="https://user-images.githubusercontent.com/4391506/148694247-9af70d61-6b8b-4bca-bdcb-63c1a0a334f9.png">

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
@hawkw hawkw mentioned this pull request Jan 18, 2022
Merged
hawkw added a commit that referenced this pull request Jan 18, 2022
@hawkw
api: prepare to release v0.1.1 (#259)
1fe0650 
<a name="0.1.1"></a>
## 0.1.1 (2022-01-18)

#### Features

- add `From<tracing_core::span::Id>` for `Id` (#244) ([095b1ef](095b1ef))
hawkw added a commit that referenced this pull request Jan 18, 2022
@hawkw
subscriber: prepare to release v0.1.1
4a49574 
<a name="0.1.1"></a>
## 0.1.1 (2022-01-18)

#### Bug Fixes

*  only send *new* tasks/resources/etc over the event channel (#238) ([fdc77e2](fdc77e2))
*  increased default event buffer capacity (#235) ([0cf0aee](0cf0aee))
*  use saturating arithmetic for attribute updates (#234) ([fe82e17](fe82e17))

#### Changes

*  moved ID rewriting from `console-subscriber` to the client (#244) ([095b1ef](095b1ef))
## 0.1.0 (2021-12-16)

- Initial release! &#x1f389;
@hawkw hawkw mentioned this pull request Jan 18, 2022
Merged
hawkw added a commit that referenced this pull request Jan 18, 2022
@hawkw
subscriber: prepare to release v0.1.1
c2abf85 
<a name="0.1.1"></a>
## 0.1.1 (2022-01-18)

#### Bug Fixes

*  only send *new* tasks/resources/etc over the event channel (#238) ([fdc77e2](fdc77e2))
*  increased default event buffer capacity (#235) ([0cf0aee](0cf0aee))
*  use saturating arithmetic for attribute updates (#234) ([fe82e17](fe82e17))

#### Changes

*  moved ID rewriting from `console-subscriber` to the client (#244) ([095b1ef](095b1ef))
## 0.1.0 (2021-12-16)

- Initial release! &#x1f389;
hawkw added a commit that referenced this pull request Jan 18, 2022
@hawkw
subscriber: prepare to release v0.1.1 (#260)
12a4821 
<a name="0.1.1"></a>
## 0.1.1 (2022-01-18)

#### Bug Fixes

*  only send *new* tasks/resources/etc over the event channel (#238) ([fdc77e2](fdc77e2))
*  increased default event buffer capacity (#235) ([0cf0aee](0cf0aee))
*  use saturating arithmetic for attribute updates (#234) ([fe82e17](fe82e17))

#### Changes

*  moved ID rewriting from `console-subscriber` to the client (#244) ([095b1ef](095b1ef))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zaharidichev zaharidichev zaharidichev approved these changes

@seanmonstar seanmonstar Awaiting requested review from seanmonstar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

move ID rewriting to the client side
2 participants
@hawkw @zaharidichev