feat[tracing-subscriber]: extra fields #2733
Conversation
|
This is very useful for linking logs to traces. I would love to see it get merged. |
hseeberger
force-pushed
the
feat/tracing-subscriber-additional-fields
branch
from
1157692
to
8537bb7
Compare
|
Dear maintainers, please approve the workflow for this PR at least. Reviewing and potentially merging would also be highly appreciated. |
|
Hi, I'll preface this with that I am not a maintainer so I don't have approval rights, I'll just offer some observations. So feel free to ignore any or all of my suggestions. Anyway, thanks you for this, I really think we need something like this. Are you still open working on this even after the time since you have opened the PR? I've tried to play around with this and made a test, would you be open to me opening a PR to your branch so that it can be added? This also currently lacks documentation. Would you be open to me writing something for this? |
|
|
||
| /// Set the function to make extra fields. | ||
| #[cfg(feature = "extra-fields")] | ||
| pub fn with_extra_fields(self, make_extra_fields: Box<dyn format::MakeExtraFields>) -> Self { |
There was a problem hiding this comment.
Nit, the other methods are named with_make_extra_fields, maybe it would be better to use just one of the two names.
| @@ -662,6 +708,8 @@ impl<F, T> Format<F, T> { | |||
| display_thread_name: self.display_thread_name, | |||
| display_filename: true, | |||
| display_line_number: true, | |||
| #[cfg(feature = "extra-fields")] | |||
| make_extra_fields: None, | |||
There was a problem hiding this comment.
Please take the extra fields from self. Here and in all other instances of this.
If this sets it to None, we cannot do some chains such as builder.with_make_extra_fields(...).with_timer(...) because you'll lose this.
| @@ -304,6 +304,13 @@ where | |||
| .serialize_entry("threadId", &format!("{:?}", std::thread::current().id()))?; | |||
| } | |||
|
|
|||
| #[cfg(feature = "extra-fields")] | |||
| if let Some((make_extra_fields, current_span)) = self.make_extra_fields.as_ref().zip(current_span) { | |||
There was a problem hiding this comment.
You should also add an option to line 236 so that the current span is present even if it is not to be logged.
| #[cfg(feature = "extra-fields")] | ||
| if let Some((make_extra_fields, current_span)) = self.make_extra_fields.as_ref().zip(current_span) { | ||
| for (k, v) in make_extra_fields(current_span.extensions()) { | ||
| serializer.serialize_entry(&k, &v)?; |
There was a problem hiding this comment.
I think this needs to be moved to where fields serialization happens. Currently, the output is this for me even though I did not want to flatten it (lorem and foo are extra fields):
{
"timestamp": "fake time",
"level": "INFO",
"fields": {
"message": "hello"
},
"target": "tracing_subscriber::fmt::format::test",
"span": {
"name": "span"
},
"spans": [
{
"name": "span"
}
],
"lorem": "ipsum",
"foo": "bar"
}
This can be used when the fields are flattened but when not, this will most likely need to be merged with event.field_map().
I tried to simplify this by calling serializer.serialize_entry("fields", ...) twice, but it will create two entries in the JSON map called fields so I don't think there is any way except for merging (or maybe just chaining will work if this takes an iterator).
| #[cfg(feature = "extra-fields")] | ||
| pub fn with_make_extra_fields(self, make_trace_id: Box<dyn MakeExtraFields>) -> Self { |
There was a problem hiding this comment.
I think this function (and the field on the struct) should be moved to the Json struct similarly to the options such as with_current_span.
If it were here, it's too easy to misuse because it does nothing for all formatters except for json.
Co-authored-by: David Mládek <david.mladek.cz@gmail.com>
Ref #1531. Superseeds #2664.
Currently the trace ID, added to Format, is only serialized for Json.