feat(new transform): Add buffered gate transform #21071

ilinas · 2024-08-14T12:06:14Z

See issue: #15263

A simple implementation of ring buffer / backtrace event handling that I ended up naming the gate transform. Keeps events in a buffer until a trigger is encountered and the buffer is flushed. When the buffer is full, the oldest events are being dropped, and it works pretty much like the filter transform.
The code is essentially a simple VecDeque.

Example configuration:

transforms:
  app_gate:
    type: gate
    inputs:
      - app_logs
    pass_when: '"info" == .level'
    open_when: '"error" == .level'
    auto_close: true
    tail_events: 20
    max_events: 200

Submitting as a draft for now.

bits-bot · 2024-08-14T12:06:18Z

All committers have signed the CLA.

jszwedko

Thanks for opening this PR! Apologies, I somehow missed your comments on the other issue, I think this is a really interesting and useful idea.

I noticed you opened this as a draft to start. What level of feedback are you looking for currently? I'm on board with the general idea and am happy to leave more detailed feedback in line.

ilinas · 2024-08-19T09:43:25Z

Hi @jszwedko, no worries. I am glad that you think this is useful.

I am new to both Rust and Vector code, so this implementation is based on my observations of how other similar transforms were implemented. It's highly possible that I missed something completely obvious, so some sanity check would be highly appreciated.

I know I am missing the docs, but first I wanted to make sure that the general functionality and the config parameters are sensible?

jszwedko

Apologies for the delayed response here. I took another look over and feel like it is generally heading in the right direction. think the behavior will be a bit difficult to describe to users, but I can't immediately think of a better configuration model so I think we can rely on some examples on the documentation page to help.

Again I think this is a super nifty feature and a great fit for Vector's use-cases so I appreciate you proposing it!

If you are interested in pushing this forward, some next steps I see:

Add documentation including examples (see https://github.com/vectordotdev/vector/blob/master/website/cue/reference/components/transforms/reduce.cue for an example of another transform's docs)
Add a changelog entry, see: https://github.com/vectordotdev/vector/blob/master/changelog.d/README.md

jszwedko · 2024-09-06T21:57:38Z

src/transforms/gate/config.rs

+                .transpose()?,
+            self.max_events.unwrap_or(100),
+            self.auto_close.unwrap_or(true),
+            self.tail_events.unwrap_or(10),


My intuition would be that tail_events would be 0 though I can't articulate why exactly.

jszwedko · 2024-09-06T22:17:02Z

src/transforms/gate/config.rs

+    /// Automatically close the gate after the buffer has been flushed.
+    pub auto_close: Option<bool>,


Would we want to disallow auto_close and close_when being specified at the same time?

It also seems like auto_close might be the equivalent of close_when: "true" but that may not terribly discoverable behavior. We could treat the absence of close_when as auto-close 🤔

jszwedko · 2024-09-06T22:18:21Z

src/transforms/gate/config.rs

+    pub pass_when: Option<AnyCondition>,
+
+    /// A logical condition used to open the gate.
+    pub open_when: Option<AnyCondition>,


Do we want to require this option? It's unclear to me what the behavior should be if there is no open_when 🤔

jszwedko · 2024-09-06T22:20:31Z

src/transforms/gate/transform.rs

+
+impl FunctionTransform for Gate {
+    fn transform(&mut self, output: &mut OutputBuffer, event: Event) {
+        let (pass_gate, event) = match self.pass_when.as_ref() {


I think you could return early after this block if pass_gate is true to avoid evaluating the other conditions and pushing/popping from the dequeue.

jszwedko · 2024-09-06T22:24:02Z

src/transforms/gate/transform.rs

+        } else if open_gate {
+            self.current_state = GateState::Open;
+            self.buffer.drain(..).for_each(|evt| output.push(evt));
+            self.events_counter = 0;


I'd maybe set this in the close_gate block and call it tail_events_counter to make it clearer.

jszwedko · 2024-09-06T22:26:05Z

src/transforms/gate/transform.rs

+}
+
+#[cfg(test)]
+mod test {


I'd like to see some more test permutations here to cover other cases.

ilinas · 2024-09-16T09:37:09Z

Thank you so much for your comments @jszwedko.

You are right that some of the option combinations are a bit confusing.

I tried to cover two different use cases here:

Traditional backtrace: when something important happens, e.g. '"error" == .level', flush the buffer and close the gate.
Manual gate: when you want to manually control the flow, e.g. open_when: '"session started" == .message' and close_when: '"session ended" == .message'.

Maybe it would be more logical to limit the transform to just the first use case? Especially because in the second use case you don't really benefit from having a buffer.

Could rename the transform to something like buffer or backtrace and just have flush_when instead of open_when, and completely ditch close_when and auto_close?

transforms:
  app_buffer:
    type: buffer
    inputs:
      - app_logs
    pass_when: '"info" == .level'
    flush_when: '"error" == .level'
    tail_events: 20
    max_events: 200

jszwedko · 2024-09-28T18:10:39Z

Thanks for the additional thoughts! I see what you are saying. Maybe it would be easier to just focus on one use-case at a time. It might end up being a better model to have two separate transforms to support the two use-cases. Would you want to refocus this PR just on one of them and then we can have a second PR focused on the other? It sounds like you'd like to target the "backtrace" use-case first?

Add gate transform

8555243

github-actions bot added the domain: transforms Anything related to Vector's transform components label Aug 14, 2024

jszwedko reviewed Aug 16, 2024

View reviewed changes

jszwedko reviewed Sep 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(new transform): Add buffered gate transform #21071

feat(new transform): Add buffered gate transform #21071

ilinas commented Aug 14, 2024

bits-bot commented Aug 14, 2024 •

edited

Loading

jszwedko left a comment

ilinas commented Aug 19, 2024

jszwedko left a comment

jszwedko Sep 6, 2024

jszwedko Sep 6, 2024

jszwedko Sep 6, 2024

jszwedko Sep 6, 2024

jszwedko Sep 6, 2024

jszwedko Sep 6, 2024

ilinas commented Sep 16, 2024

jszwedko commented Sep 28, 2024

		/// Automatically close the gate after the buffer has been flushed.
		pub auto_close: Option<bool>,

feat(new transform): Add buffered gate transform #21071

Are you sure you want to change the base?

feat(new transform): Add buffered gate transform #21071

Conversation

ilinas commented Aug 14, 2024

bits-bot commented Aug 14, 2024 • edited Loading

jszwedko left a comment

Choose a reason for hiding this comment

ilinas commented Aug 19, 2024

jszwedko left a comment

Choose a reason for hiding this comment

jszwedko Sep 6, 2024

Choose a reason for hiding this comment

jszwedko Sep 6, 2024

Choose a reason for hiding this comment

jszwedko Sep 6, 2024

Choose a reason for hiding this comment

jszwedko Sep 6, 2024

Choose a reason for hiding this comment

jszwedko Sep 6, 2024

Choose a reason for hiding this comment

jszwedko Sep 6, 2024

Choose a reason for hiding this comment

ilinas commented Sep 16, 2024

jszwedko commented Sep 28, 2024

bits-bot commented Aug 14, 2024 •

edited

Loading