-
-
Notifications
You must be signed in to change notification settings - Fork 137
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.
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
feat: send structured fields on the wire #26
Changes from 4 commits
28114b1
4362dcf
658733d
6b8ab19
b3aa9e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ use std::{ | |
cell::RefCell, | ||
collections::HashMap, | ||
convert::TryFrom, | ||
fmt::Write, | ||
rc::{Rc, Weak}, | ||
time::{Duration, SystemTime}, | ||
}; | ||
|
@@ -115,13 +116,21 @@ impl State { | |
proto::tasks::task::Kind::Spawn => "T", | ||
proto::tasks::task::Kind::Blocking => "B", | ||
}; | ||
|
||
let fields = task | ||
.fields | ||
.iter() | ||
.fold(String::new(), |mut res, f| { | ||
write!(&mut res, "{} ", f).unwrap(); | ||
res | ||
}) | ||
.trim_end() | ||
.into(); | ||
let id = task.id?.id; | ||
let stats = stats_update.remove(&id)?.into(); | ||
let task = Task { | ||
id, | ||
id_hex: format!("{:x}", id), | ||
fields: task.string_fields, | ||
fields, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 for now, since the purpose of this change was just to add structured fields to the wire format. a good follow-up would be to change the console TUI to store task fields as structured values as well (maybe a hashmap of names -> values for starters?). that would enable some additional things we could do, such as sorting the task list by the values of various fields, filtering which fields are displayed, and nicer formatting (e.g. bolding field names or displaying them in a different color). |
||
kind, | ||
stats, | ||
completed_for: 0, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,16 +24,28 @@ message SpanId { | |
uint64 id = 1; | ||
} | ||
|
||
// A message representing a key-value pair of data associated with a `Span` | ||
message Field { | ||
oneof field { | ||
string debug = 1; | ||
oneof name { | ||
// The string representation of the name. | ||
string str_name = 1; | ||
// An index position into the `Metadata.field_names`. | ||
uint64 name_idx = 2; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in order to actually use this, we'll also want to include the list of field names in the metadata message, so that these indices are meaningful. but we don't have to do that in this PR, that can be added in a follow-up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, just a thought: it might be worth having this message also include the metadata ID that the index refers to, so that a |
||
} | ||
oneof value { | ||
string debug_val = 3; | ||
string str_val = 4; | ||
uint64 u64_val = 5; | ||
sint64 i64_val = 6; | ||
bool bool_val = 7; | ||
} | ||
MetaId metadata_id = 8; | ||
} | ||
|
||
message Span { | ||
SpanId id = 1; | ||
MetaId metadata_id = 2; | ||
map<string, Field> fields = 3; | ||
repeated Field fields = 3; | ||
google.protobuf.Timestamp at = 4; | ||
} | ||
|
||
|
@@ -54,6 +66,10 @@ message Metadata { | |
Kind kind = 5; | ||
Level level = 6; | ||
|
||
// The names of the key-value fields attached to the | ||
// span or event this metadata is associated with. | ||
repeated string field_names = 7; | ||
Comment on lines
+69
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as a follow-up, we should actually populate this field... |
||
|
||
enum Kind { | ||
SPAN = 0; | ||
EVENT = 1; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,7 +63,7 @@ message Task { | |
// When the task's stats change, or when the task completes, it will be | ||
// identified by this ID; if the client requires additional information | ||
// included in the `Task` message, it should store that data and access it | ||
// by ID. | ||
// by ID. | ||
common.SpanId id = 1; | ||
// The numeric ID of the task's `Metadata`. | ||
// | ||
|
@@ -74,11 +74,8 @@ message Task { | |
// The category of task this task belongs to. | ||
Kind kind = 3; | ||
|
||
// A string representation of any fields recorded about this task. | ||
// | ||
// NOTE: eventually, it would be nice to support structured fields in tasks; | ||
// we can deprecate this when we add that. | ||
string string_fields = 4; | ||
// A list of `Field` objects attached to this task. | ||
repeated common.Field fields = 4; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it would be nice to have a comment here |
||
|
||
// An ordered list of span IDs corresponding to the `tracing` span context | ||
// in which this task was spawned. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that
&dyn std::error::Error
is also a potential primitivetracing
field value.this implementation will just record
Error
s withfmt::Debug
, which is fine for now, but it might be nice to also allow recordingError
s in a more structured way as a follow-up? we could have anError
message which includes theError
type'sfmt::Display
output and an optionalsource
field, or maybe the entire chain ofsource
s as a protobuf list?right now this wouldn't be particularly useful, since we only look at the
tokio::task
spans which never recordError
fields. but in the future, we could potentially support nice error reporting in the console, so it's something that could be worth thinking about as a follow-up change...