feat(event): Add MultiValueEvent interface and MultiObserverEvent implementation #602
Conversation
…lementation This PR introduces the first step in refactoring the event handling system to better support multiple values in a single event, which will help reduce allocations when processing events. This is part of a larger effort to improve performance and reduce memory allocations in the statsd exporter. Changes: - Add new `MultiValueEvent` interface that supports multiple values per event - Add `MultiObserverEvent` implementation for handling multiple observations - Add `ExplodableEvent` interface for backward compatibility - Add `Values()` method to existing event types - Add comprehensive tests for new interfaces and implementations This change is the foundation for future improvements that will: 1. Move explosion logic to a dedicated package 2. Update the line parser to use multi-value events 3. Modify the exporter to handle multi-value events directly 4. Eventually remove the need for event explosion The changes in this PR are backward compatible and don't affect existing functionality. Relates to #577 Signed-off-by: Pedro Tanaka <[email protected]>
pkg/event/event.go
Outdated
| // MultiValueEvent is an event that contains multiple values, it is going to replace the existing Event interface. | ||
| type MultiValueEvent interface { | ||
| MetricName() string | ||
| Value() float64 |
There was a problem hiding this comment.
What are the semantics of this function on a multi-value event? Do we need it to be part of the MultiValueEvent interface?
I still like the idea of implementing this interface on the concrete single events by implementing the Values() function since going event -> []{event} is lossless, but that doesn't force us to have the single-Value() function in the interface.
There was a problem hiding this comment.
Got it, I think it makes more sense as well. Going to refactor this part.
pkg/event/event.go
Outdated
| } | ||
|
|
||
| type ExplodableEvent interface { | ||
| Explode() []Event |
There was a problem hiding this comment.
I know I used that word initially but I'm having second thoughts about using "explode" here, it seems a bit, hmm, violent 😬 Maybe we could use "expand" instead?
pkg/event/event_test.go
Outdated
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| switch tt.name { |
There was a problem hiding this comment.
Hmm switching on the name of the test kind of defeats the purpose of a table-driven test. What do you think of writing out the test cases under an explicit sub-test each, like
t.Run("MultiObserverEvent implements MultiValueEvent", func(t *testing.T) {
if _, ok := tt.event.(MultiValueEvent); !ok {
t.Error("MultiObserverEvent does not implement MultiValueEvent")
}
})I would also be happy with assert.True(t, ok) to make it even more compact, your choice 😄
pkg/event/event_test.go
Outdated
| if _, ok := tt.event.(ExplodableEvent); !ok { | ||
| t.Error("MultiObserverEvent does not implement ExplodableEvent") | ||
| } | ||
| case "MultiObserverEvent implements Event": |
There was a problem hiding this comment.
See the comment on Value() – do we need this to implement Event? What are the semantics of doing so, what does it mean for a multi-event to act like a single event?
and simplify tests; Signed-off-by: Pedro Tanaka <[email protected]>
Signed-off-by: Pedro Tanaka <[email protected]>
This PR introduces the first step in refactoring the event handling system to better support multiple values in a single event, which will help reduce allocations when processing events. This is part of a larger effort to improve performance and reduce memory allocations in the statsd_exporter.
Changes:
MultiValueEventinterface that supports multiple values per eventMultiObserverEventimplementation for handling multiple observationsExplodableEventinterface for backward compatibilityValues()method to existing event typesThis is the first PR of a two-part series of changes I plan on doing.
Here is the overall plan I have in my head:
Part 1
Introduce the
MultiValueEventinterface and theMultiObserverEventimplementation.With support for "exploding" the event into an slice of events, this will help users that use the project as a library to migrate the code when we break interface.
Part 2 (⚠️ BREAKING)
Introduce some kind of EventExploder and create a new EventQueue to accept a channel of
event.MultiValueEventinstead of receiving a channel ofevent.Events(i.e. slice of Events).Also rename the current EventQueue to LegacyEventQueue which allows users to use the old behavior, annotate this as deprecated.
Relates to #577