telegraf icon indicating copy to clipboard operation
telegraf copied to clipboard

Support canceling plugins using context.Context

Open goller opened this issue 4 years ago • 2 comments

Feature Request

I'd like to see support for canceling plugins including non-service plugins using context.Context.
My motivating example came from the SNMP plugin where it can take a long time for each SNMP field to be timed-out.

Proposal:

Follow the pattern in the go HTTP library:

diff --git a/accumulator.go b/accumulator.go
index 1ea5737a..0394c398 100644
--- a/accumulator.go
+++ b/accumulator.go
@@ -1,11 +1,16 @@
 package telegraf
 
 import (
+	"context"
 	"time"
 )
 
 // Accumulator allows adding metrics to the processing flow.
 type Accumulator interface {
+	// Context returns the existing context on the Accumlator or Background.
+	Context() context.Context
+	WithContext(ctx context.Context) Accumulator
+
 	// AddFields adds a metric to the accumulator with the given measurement
 	// name, fields, and tags (and timestamp). If a timestamp is not provided,
 	// then the accumulator sets it to "now".

Telegraf already sets a cancel on various signals here: https://github.com/influxdata/telegraf/blob/16784bca556be330a00bbda4881238701d1f5e59/cmd/telegraf/telegraf.go#L91

If one were to SIGINT, SIGTERM', SIGHUPthen the plugins could be canceled if they use the context of theAccumulator`.

In a brief survey it was common for the Accumulator to be passed into the specific implementation of the plugin.

goller avatar Nov 04 '19 20:11 goller

I don't think we should add the Context to the Accumulator as it is only available in inputs and aggregators. Instead the Context should be passed in to the Start/Gather functions. A transition could be made by creating a new plugin interface and supporting both variations.

danielnelson avatar Nov 05 '19 18:11 danielnelson

Obviously what I'm proposing is more work, in the meantime we have been creating a background/todo context in the Start function and cancelling it in Stop.

danielnelson avatar Nov 05 '19 18:11 danielnelson

Working going on around this in #13913, so closing this one as #13913 has more info.

powersj avatar Oct 24 '23 15:10 powersj