Skip to content

Commit

Permalink
refac: move ID rewriting to console (#244)
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
hawkw authored Dec 28, 2021
1 parent ef55303 commit 095b1ef
Show file tree
Hide file tree
Showing 8 changed files with 115 additions and 133 deletions.
6 changes: 6 additions & 0 deletions console-api/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,3 +221,9 @@ impl From<Id> for u64 {
}

impl Copy for Id {}

impl From<tracing_core::span::Id> for Id {
fn from(id: tracing_core::span::Id) -> Self {
Id { id: id.into_u64() }
}
}
24 changes: 7 additions & 17 deletions console-subscriber/src/aggregator/id_data.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{shrink::ShrinkMap, DroppedAt, Id, Ids, ToProto};
use std::collections::{HashMap, HashSet};
use super::{shrink::ShrinkMap, DroppedAt, Id, ToProto};
use std::collections::HashMap;
use std::ops::{Deref, DerefMut};
use std::time::{Duration, SystemTime};

Expand Down Expand Up @@ -66,9 +66,12 @@ impl<T> IdData<T> {
match include {
Include::UpdatedOnly => self
.since_last_update()
.map(|(id, d)| (*id, d.to_proto()))
.map(|(id, d)| (id.into_u64(), d.to_proto()))
.collect(),
Include::All => self
.all()
.map(|(id, d)| (id.into_u64(), d.to_proto()))
.collect(),
Include::All => self.all().map(|(id, d)| (*id, d.to_proto())).collect(),
}
}

Expand All @@ -78,7 +81,6 @@ impl<T> IdData<T> {
now: SystemTime,
retention: Duration,
has_watchers: bool,
ids: &mut Ids,
) {
let _span = tracing::debug_span!(
"drop_closed",
Expand All @@ -90,7 +92,6 @@ impl<T> IdData<T> {
// drop closed entities
tracing::trace!(?retention, has_watchers, "dropping closed");

let mut dropped_ids = HashSet::new();
stats.data.retain_and_shrink(|id, (stats, dirty)| {
if let Some(dropped_at) = stats.dropped_at() {
let dropped_for = now.duration_since(dropped_at).unwrap_or_default();
Expand All @@ -105,10 +106,6 @@ impl<T> IdData<T> {
stats.dirty = *dirty,
should_drop,
);

if should_drop {
dropped_ids.insert(*id);
}
return !should_drop;
}

Expand All @@ -118,13 +115,6 @@ impl<T> IdData<T> {
// drop closed entities which no longer have stats.
self.data
.retain_and_shrink(|id, (_, _)| stats.data.contains_key(id));

if !dropped_ids.is_empty() {
// drop closed entities which no longer have stats.
self.data
.retain_and_shrink(|id, (_, _)| stats.data.contains_key(id));
ids.remove_all(&dropped_ids);
}
}
}

Expand Down
Loading

0 comments on commit 095b1ef

Please sign in to comment.