Vector: Event ergonomics

[MOZGIII]

Code uses Event everywhere, but there are places where using LogEvent or Metric makes more sense. I suggest taking a closer look into the use the of the Event in attempt to improve the ergonomics.

Here's one example of how we can improve the current situation: LogEvent can currently only be constructed via Event::new_empty_log().into_log(). It would be much easier if we could do LogEvent::new().
Another related issue is in some places Event is used where LogEvent would be sufficient, for instance at the docker source. It does a lot of as_log and as_mut_log, while it only exclusively works with log event under the hood. Those do a runtime check, and can potentially generate a panic. Although the compiler is probably smart enough to optimize those, it just doesn't make a lot of sense to write it like that. Everything can be done with LogEvent, and we only have to wrap it into the Event::Log when we pass it to the sink.
It's even possible to implement more advanced traits to provide better type safety: instead of implementing SourceConfig that works with Sender<Event>, we could add LogEventSourceConfig, that works with Sender<LogEvent> and provides impl<T: LogEventSourceConfig> SourceConfig for T, and also sets the SourceConfig::output_type to DataType::Log!

I think polishing the Event enum ergonomics is a good way to improve the code quality and clarity, which would make it more approachable to new people. I myself was quite surprised by the approach when I first saw it, and I think it should be dealt with. It serves no purpose really.

I didn't bring it up before because I thought it was an optimization to eliminate copying when wrapping the LogEvent into Event, but then I realized that it can't possibly work if that was the intention: not only it's not properly expressed, but it's also not tested anywhere. Furthermore, as long as we have Event::into_log and do things like Event::new_empty_log().into_log() we lose all the benefits, cause I'd believe the compiler would rather optimize that to a bare LogEvent construction. I realized I was trying to be too smart and that I tricked myself. :disappointed:
That said, I think the compiler can quite trivially deduce that it'd need extra space for the enum discriminant when working with LogEvent that's being wrapped into Event, and it should be able to reserve space when allocating the LogEvent. There's a lot of speculation though. Including the alleged rationale for using Event everywhere to optimize away copying: with the current approach, we don't properly express to the compiler that we need it reserve memory when we do Event::new_empty_log().into_log(). What I'm certain of though is that it's an odd approach to writing a constructor, and an ergonomically unpleasant one. I think this has to change. In addition, I'd say we should explicitly address the memory representations and alignment if we have a goal to optimize them - explicitly tell it to the compiler, document it and test that compiler actually generates the memory layout we look for.

[binarylogic]

I'm curious about what @a-rodin thinks since he is working on the data processing project, and is this worth adding to the project?

[lukesteensen]

Yes, this is something I've been thinking about. The whole event module has grown organically over time and slowly gotten more and more annoying to use.

One particular area we should be able to improve is the ergonomics around Atom and Value. In areas dealing with getting and setting values on events, there is an enormous amount of distracting &"foo".into() and similar. This causes quite a bit of unnecessary friction, especially when writing tests.

---

@MOZGIII

Here's one example of how we can improve the current situation: LogEvent can currently only be constructed via Event::new_empty_log().into_log(). It would be much easier if we could do LogEvent::new().

Agreed, this is a small and easy win.

Another related issue is in some places Event is used where LogEvent would be sufficient, for instance at the docker source. It does a lot of as_log and as_mut_log, while it only exclusively works with log event under the hood. Those do a runtime check, and can potentially generate a panic. Although the compiler is probably smart enough to optimize those, it just doesn't make a lot of sense to write it like that. Everything can be done with LogEvent, and we only have to wrap it into the Event::Log when we pass it to the sink.

Also agreed. This I think is a problem with usage patterns as much as it is with the API. There is a lot of code that was written before Event became an enum, and the simplest fix at the time was to sprinkle as_log() where needed. It would be valuable to make a pass and clean up these usages.

It's even possible to implement more advanced traits to provide better type safety: instead of implementing SourceConfig that works with Sender<Event>, we could add LogEventSourceConfig, that works with Sender<LogEvent> and provides impl<T: LogEventSourceConfig> SourceConfig for T, and also sets the SourceConfig::output_type to DataType::Log!

This starts to touch on some larger issues and limitations, so I'd like to separate it from the rest of the discussion. There are a lot of quick wins we can get with small changes before we start to consider things like this.

At the highest level, when considering data flow between configured components, we will always need runtime type checking due to the fact that we don't know the configured topology until runtime. That fact, combined with the fact that our current design for sources, transforms, and sinks leaves them as the top-level entity (i.e. they are not run by some higher-level concept), leads to the current state where the dynamic runtime checks happen directly in all our components.

The solution would involve a redesign of how our components fit together such that there is some higher layer that deals with types before passing data down into transforms, sinks, etc. In that model, we could write transforms directly on LogEvent, etc. However, such a redesign would be a large undertaking and I'd want to spend some time exploring the design space before settling on a particular way of doing it.

I think polishing the Event enum ergonomics is a good way to improve the code quality and clarity, which would make it more approachable to new people. I myself was quite surprised by the approach when I first saw it, and I think it should be dealt with. It serves no purpose really.

I agree! This seems like one of the easier ways available to us for improving the contributor experience.

I didn't bring it up before because I thought it was an optimization...

I appreciate your assumption that there was some intelligent thought behind the current design :smile:

[LucioFranco]

impl<T: LogEventSourceConfig> SourceConfig for T

I don't this works because of coherence since we could then never implement anything for SourceConfig since there is a blanket impl. So if we had a MetricsEventSourceConfig we could never impl<T: MetricsEventSourceConfig> SourceConfig for T.

I do agree we could probably improve some of the API though around Event.

[a-rodin]

I agree with the points about improving Event, and I think @lukesteensen already summed up what we can do here. @binarylogic I think it is worth to add this issue to the data processing project.

[MOZGIII]

impl<T: LogEventSourceConfig> SourceConfig for T

I don't this works because of coherence since we could then never implement anything for SourceConfig since there is a blanket impl. So if we had a MetricsEventSourceConfig we could never impl<T: MetricsEventSourceConfig> SourceConfig for T.

I do agree we could probably improve some of the API though around Event.

The next best thing to impl<T: LogEventSourceConfig> SourceConfig for T is a generic struct LogEventSourceAdapter<T: LogEventSourceConfig>(T) that implements SourceConfig. It doesn't won't cause difficulties with coherence.

I agree with the points about improving Event, and I think @lukesteensen already summed up what we can do here. @binarylogic I think it is worth to add this issue to the data processing project.

I think there are a few things more that we could improve.

At the highest level, when considering data flow between configured components, we will always need runtime type checking due to the fact that we don't know the configured topology until runtime. That fact, combined with the fact that our current design for sources, transforms, and sinks leaves them as the top-level entity (i.e. they are not run by some higher-level concept), leads to the current state where the dynamic runtime checks happen directly in all our components.

I think we could restructure the topology system in such a way that events and metrics do not get mixed with each other. In my opinion, metrics are quite different in their behavior than log events, and there's also a difference in what'd be meaningful ways to process them. The current representation that we use makes sense for now, but I suspect it is bound to diverge in the future - especially if we start offering statistically correct aggregations and sieving.
To keep this short for now, what I propose, in addition to the things before, and as an independent point, is that we make the events types a concern at the topology system. Then we can make it capable of producing source-transform-sink chains that have concrete "variant" requirements, effectively getting rid of the enum itself where we don't need it. We still might need it in some cases, but in most, if something (for instance, a transform) supports both logs and metrics, a generic input/output type might do a better job than an enum.
I guess this one is a kind of global change in the architecture, and this approach must've been considered before. So, if someone is aware of a previous discussion somewhere that touches this particular proposal - I'd appreciate links to that. If not - the further discussion for this point probably worth a separate issue. That's, of course, if this proposal doesn't immediately look like something we don't want to do. :smile:

[a-rodin]

I think this should be moved to one of the "tech debt" milestones because it is related to the organization of Vector's internals, but not to the observable behavior.

[binarylogic]

Agree. Let's defer this change for now.

[binarylogic]

@Hoverbear I've blocked this on https://github.com/timberio/vector-product/issues/8. Do you agree that we should wait for that before making these changes?

[binarylogic]

Closing this since it's not clear what we should do. The log event RFC covers this tech-debt and we plan to take it further in https://github.com/timberio/vector-product/issues/40.