Make Wavefront internal sender's buffer size configurable

[hanwavefront]

Required for all PRs:

• Signed CLA.
• Associated README.md updated.
• Has appropriate unit tests.

Updating Wavefront Output Plugin with buffer size configuration option (#7409) and related config options.

[sjwang90]

Thanks for the PR! We'll take a look and let you know if we have any comments.

@prydin If you have any feedback that'd be great.

[prydin]

@prydin If you have any feedback that'd be great.

Looks good to me.

[ssoroka]

I guess at this point I don't understand why we would want to define these on the plugin. These are settings that are globally available already, and it doesn't appear that the plugin needs to access these.

[ssoroka]

flush_interval, metric_buffer_limit and metric_batch_size are set automatically and override-able on outputs. Typically we don't add these to the output config readme, as they can be overridden on any output, and adding them to the config is going to conflict with those values.

[ssoroka]

Ok, I see the issue. To clarify, the problem is that wavefront-sdk-go's BatchSize, MaxBufferSize, (though not necessarily FlushIntervalSeconds) conflict with our naming of BatchSize, MaxBufferSize, and FlushInterval, and setting these on the output is going to block the ability to override these settings for this output, so we probably don't want to do that.

Possible options:
A) Name them WavefrontBatchSize, WavefrontMaxBufferSize, and WavefrontFlushInterval (the seconds part is kind of pointless if you have duration types).
B) Don't expose them and set these values to whatever the influx values are set to. This might be difficult because I don't think they're directly exposed to the plugin.

What's the problem we're trying to solve here? I don't see an open issue.

[hanwavefront]

@ssoroka Thanks for the review. The corresponding issue is #7409 -- wavefront-sdk-go's config params (and specifically the buffer limit) need to be customizable to align with the influx values and prevent points from being dropped. I'm hesitant about option B because I can envision a scenario where we would want the values to be different than the influx values (e.g., internal metrics in wavefront-sdk-go). But I'm not clear on where the name conflict is and why option A is necessary? The names are different from the influx names (MetricBatchSize, MetricBufferLimit, and FlushInterval), and they would only be configured on [[outputs.wavefront]]. Let me know if I'm missing something.

[hanwavefront]

@ssoroka Please take a look at my previous comment if you get a chance. These new config params pertain only to the Wavefront output plugin's behavior and I don't believe they conflict with their influx counterparts, but let me know if I'm mistaken.

[ssoroka]

After reading the issue, I'm concerned that this change won't resolve the problem. Wavefront should not have its own internal buffer. You end up with two buffers, and it doesn't matter what the size is, it's going to eventually drop metrics. Wavefront should instead be blocking and actually writing the metrics out when passed batches of 1000 (agent.metric_batch_size) metrics, because Telegraf is managing the buffer, and keeping track of retries on failures.

Said another way, because Wavefront is not blocking and applying backpressure, it's not communicating with Telegraf that the buffer is full and to slow down. Due to this lack of communication, Telegraf happily sends all the metrics in, resulting in overflowing the wavefront buffer.

I'd recommend removing the wavefront buffer and sending the metrics immediately when passed a batch, and blocking until it succeeds or fails. If for some reason you really must have double buffers (I still think this is a mistake), then wavefront needs to block when the buffer is full, until there is room in the buffer to accept the new batch, not return an error and drop messages.

cc @prydin

[ssoroka]

closing this in favor of #8165