Serde compatibility crate about tracing HOT 4 CLOSED

I'd like to take a shot at it!

from tracing.

I've been struggling a bit getting this issue done & I've talked with Eliza out of band about it. I thought I would go ahead & push up some of my work on this issue on my fork. Maybe some other folks have ideas about how we can get serde compatability.

The problem I'm running into is how exactly to implement Visit. All the Serializer methods consume self by-value but the Visit methods take &mut self. Another issue is that all the serde methods return Result & tokio-trace returns (). That makes it so we have to unwrap all the values. What makes sense to proceed on this issue? Do you have any thoughts @LucioFranco?

https://github.com/DebugSteven/tokio-trace-nursery/tree/tokio-trace-serde

from tracing.

@DebugSteven hmm, I took a look, I'm not sure I understand where the issue is with Serializer consuming self? Could you point out where you are seeing this issue?

I think for the second option, we probably just want to drop the field maybe? Serde can fail since its more general but in a logging/tracing context, it makes sense for it to not fail since im not sure what you'd do with the failed data? We prob dont want to log the error either since that will get noisy.

from tracing.

The problem I'm running into is how exactly to implement Visit. All the Serializer methods consume self by-value but the Visit methods take &mut self.

When serializing something that implements Visit, you're probably going to be using a SerializeMap rather than a Serializer, since we want to record fields as map key/value pairs. SerializeMap's serialize functions take &mut self.

Another issue is that all the serde methods return Result & tokio-trace returns (). That makes it so we have to unwrap all the values. What makes sense to proceed on this issue?

I'd probably recommend something like this (note that this is just a sketch):

struct SerdeVisitor<S: SerializeMap> {
     serializer: S,
     state: Result<(), S::Error>
} 

impl<S: SerializeMap> Visit for SerdeVisitor<S> {
    fn record_bool(&mut self, field: &Field, value: bool) {
        // If previous fields serialized successfully, continue serializing, 
        // otherwise, short-circuit and do nothing.
        if self.state.is_ok() {
            self.state = self.serializer
                .serialize_entry(field.name(), value)
                .map(|_| ());
        }
    }

    // ...
}

impl<S: SerializeMap> SerdeVisitor<S> {

    /// Completes serializing the visited object, returning `Ok(())` if all
    /// fields were serialized correctly, or `Err(S::Error)` if a field could
    /// not be serialized.
    fn finish(self) -> Result<S::Ok, S::Error> {
        self.state?;
        self.serializer.end()
    }
}

One other thing: after speaking to you a bit out of band, I realised something important that I don't think I made clear enough in the original issue description. In most cases, the right way to implement serialization is to try and serialize all the data exactly as it's represented in process, so that the same representation can be reconstructed. That's not what we want to do here --- tokio-trace has a lot of fairly complex types with complex relationships for performance reasons, but those often map to much simpler concepts. For example, the Field type is internally represented as a callsite ID and an index to a static string in an array. The general approach would be to serialize the index and callsite ID, so the same Field struct can be reconstructed. Instead, though, we want to serialize the string returned by calling "name" on the Field --- we don't want to expose these internal representation details that are in most cases the result of performance optimizations. Similarly, a ValueSet should not be deserialized as a ValueSet, but as a map.

from tracing.