feat: send structured fields on the wire #26

zaharidichev · 2021-05-16T16:21:41Z

This PR adds structured fields to the wire format.

Fix: #6

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

hawkw

overall, this looks great!

i commented on a couple minor style nits you may want to address, and on potential follow-up work we can do once this is merged. also, it would be nice to see comments for the additions to the protobuf...but that's also not really a blocker as i haven't been consistent about documenting the protos either. :)

hawkw · 2021-05-22T17:59:29Z

proto/tasks.proto

-    // NOTE: eventually, it would be nice to support structured fields in tasks;
-    // we can deprecate this when we add that.
-    string string_fields = 4;
+    repeated common.Field fields = 4;


nit: it would be nice to have a comment here

hawkw · 2021-05-22T18:00:22Z

proto/common.proto

+    oneof name {
+        string str_name = 1;
+        uint64 name_idx = 2;
+    }
+    oneof value {
+        string debug_val = 3;
+        string str_val = 4;
+        uint64 u64_val = 5;
+        sint64 i64_val = 6;
+        bool bool_val = 7;


nit: it would be nice to have some comments here explaining the different potential fields here. in particular, it would be nice to explain how names work, since that's potentially not clear?

hawkw · 2021-05-22T18:01:27Z

proto/common.proto

-        string debug = 1;
+    oneof name {
+        string str_name = 1;
+        uint64 name_idx = 2;


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.

also, just a thought: it might be worth having this message also include the metadata ID that the index refers to, so that a Field message with a name index can always be hydrated? but, since the Field message is only ever sent in the Span and Task messages, which also include metadata IDs, this isn't strictly necessary...

hawkw · 2021-05-22T18:03:53Z

console-subscriber/src/lib.rs

@@ -304,3 +287,37 @@ impl proto::tasks::tasks_server::Tasks for Server {
        Ok(tonic::Response::new(stream))
    }
 }
+
+impl Visit for FieldsCollector {


nit, take it or leave it: I might call this FieldVisitor or something --- in tracing 0.2, the name "Collector" will refer to what are currently called "Subscribers", so this could be a little confusing in the future.

hawkw · 2021-05-22T18:09:32Z

console-subscriber/src/lib.rs

+            name: Some(field.name().into()),
+            value: Some(value.into()),
+        });
+    }


note that &dyn std::error::Error is also a potential primitive tracing field value.

this implementation will just record Errors with fmt::Debug, which is fine for now, but it might be nice to also allow recording Errors in a more structured way as a follow-up? we could have an Error message which includes the Error type's fmt::Display output and an optional source field, or maybe the entire chain of sources as a protobuf list?

right now this wouldn't be particularly useful, since we only look at the tokio::task spans which never record Error 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...

hawkw · 2021-05-22T18:12:14Z

console/src/tasks.rs

+            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,


👍 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).

hawkw · 2021-05-22T18:12:54Z

console-subscriber/src/lib.rs

+
+    fn record_i64(&mut self, field: &tracing_core::Field, value: i64) {
+        self.fields.push(proto::Field {
+            name: Some(field.name().into()),
+            value: Some(value.into()),
+        });
+    }
+    fn record_u64(&mut self, field: &tracing_core::Field, value: u64) {
+        self.fields.push(proto::Field {
+            name: Some(field.name().into()),
+            value: Some(value.into()),
+        });
+    }
+    fn record_bool(&mut self, field: &tracing_core::Field, value: bool) {
+        self.fields.push(proto::Field {
+            name: Some(field.name().into()),
+            value: Some(value.into()),
+        });
+    }
+    fn record_str(&mut self, field: &tracing_core::Field, value: &str) {
+        self.fields.push(proto::Field {
+            name: Some(field.name().into()),
+            value: Some(value.into()),
+        });
+    }


nitpicky, but: let's prefer to always put a newline between these methods:

Suggested change

fn record_i64(&mut self, field: &tracing_core::Field, value: i64) {

self.fields.push(proto::Field {

name: Some(field.name().into()),

value: Some(value.into()),

});

}

fn record_u64(&mut self, field: &tracing_core::Field, value: u64) {

self.fields.push(proto::Field {

name: Some(field.name().into()),

value: Some(value.into()),

});

}

fn record_bool(&mut self, field: &tracing_core::Field, value: bool) {

self.fields.push(proto::Field {

name: Some(field.name().into()),

value: Some(value.into()),

});

}

fn record_str(&mut self, field: &tracing_core::Field, value: &str) {

self.fields.push(proto::Field {

name: Some(field.name().into()),

value: Some(value.into()),

});

}

fn record_i64(&mut self, field: &tracing_core::Field, value: i64) {

self.fields.push(proto::Field {

name: Some(field.name().into()),

value: Some(value.into()),

});

}

fn record_u64(&mut self, field: &tracing_core::Field, value: u64) {

self.fields.push(proto::Field {

name: Some(field.name().into()),

value: Some(value.into()),

});

}

fn record_bool(&mut self, field: &tracing_core::Field, value: bool) {

self.fields.push(proto::Field {

name: Some(field.name().into()),

value: Some(value.into()),

});

}

fn record_str(&mut self, field: &tracing_core::Field, value: &str) {

self.fields.push(proto::Field {

name: Some(field.name().into()),

value: Some(value.into()),

});

}

hawkw · 2021-05-22T18:15:03Z

console-api/src/common.rs

+            field::Value::BoolVal(v) => write!(f, "{}", v)?,
+            field::Value::StrVal(v) => write!(f, "{}", v)?,
+            field::Value::U64Val(v) => write!(f, "{}", v)?,
+            field::Value::DebugVal(v) => write!(f, "{}", v)?,
+            field::Value::I64Val(v) => write!(f, "{}", v)?,


nit: it might be slightly simpler to just call the inner value's fmt::Display impl directly, rather than using the write! macro...i think there may be a very small amount of additional overhead introduced by going through a format_args!, and also i think this will ensure that any format parameters (e.g. padding/width/etc) are passed through to the inner value

Suggested change

field::Value::BoolVal(v) => write!(f, "{}", v)?,

field::Value::StrVal(v) => write!(f, "{}", v)?,

field::Value::U64Val(v) => write!(f, "{}", v)?,

field::Value::DebugVal(v) => write!(f, "{}", v)?,

field::Value::I64Val(v) => write!(f, "{}", v)?,

field::Value::BoolVal(v) => fmt::Display::fmt(v, f)?,

field::Value::StrVal(v) => fmt::Display::fmt(v, f)?,

field::Value::U64Val(v) => fmt::Display::fmt(v, f)?,

field::Value::DebugVal(v) => fmt::Display::fmt(v, f)?,

field::Value::I64Val(v) => fmt::Display::fmt(v, f)?,

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

hawkw

this looks great, thank you! i'll open issues for some of the follow-up changes.

hawkw · 2021-05-25T20:53:22Z

proto/common.proto

+    // The names of the key-value fields attached to the
+    // span or event this metadata is associated with.
+    repeated string field_names = 7;


as a follow-up, we should actually populate this field...

In #26 `field_names` field was introduced in the `Metadata` message but was never populated. This PR populates it with the names of the fields Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev added 3 commits May 16, 2021 16:17

add structured fields

28114b1

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

Merge branch 'main' into zd/structured-fields

4362dcf

remove extra newline

658733d

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

hawkw approved these changes May 22, 2021

View reviewed changes

feedback

6b8ab19

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev requested a review from hawkw May 25, 2021 18:28

hawkw approved these changes May 25, 2021

View reviewed changes

hawkw changed the title ~~Structured fields~~ feat: send structured fields on the wire May 25, 2021

Merge branch 'main' into zd/structured-fields

b3aa9e9

hawkw merged commit 38adbd9 into tokio-rs:main May 25, 2021

This was referenced May 26, 2021

api: populate field names #32

Merged

console: store structured task fields #33

Closed

subscriber: provide richer data for error fields #34

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: send structured fields on the wire #26

feat: send structured fields on the wire #26

zaharidichev commented May 16, 2021

hawkw left a comment

hawkw May 22, 2021

hawkw May 22, 2021

hawkw May 22, 2021

hawkw May 22, 2021

hawkw May 22, 2021

hawkw May 22, 2021

hawkw May 22, 2021

hawkw May 22, 2021

hawkw May 22, 2021

hawkw left a comment

hawkw May 25, 2021

feat: send structured fields on the wire #26

feat: send structured fields on the wire #26

Conversation

zaharidichev commented May 16, 2021

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment