feat(outputs/wavefront): make maximum http batch size configurable

[LukeWinikates]

Required for all PRs:

• Updated associated README.md.
• Wrote appropriate unit tests.
• Pull request title or commits are in conventional commit format

The Wavefront golang SDK can send metrics via http(s) or tcp. In the case of http, the Wavefront SDK batches metrics internally, flushing them periodically or when they reach a maximum batch size. At present, configuring this batch size is not possible from the wavefront plugin output, so the sdk default value (10,000) is always used. In some cases smaller or larger batch sizes might be preferred, or users might want to match the wavefront client's batch size to the batch size configured for the telegraf output.

This pr exposes the wavefront internal batch size for configuration as http_batch_size.

I added a unit test validating that the intended default is set, although the outputs.Outputs map doesn't necessarily seem like an interface that wants to be used for this purpose.

[Hipska]

I'm wondering if it would make sense to match always with the telegraf configured batch size, so there's no additional config option to be set.

It would also remove the confusion of what will happen when metric_batch_size > http_batch_size or metric_batch_size < http_batch_size.

[LukeWinikates]

I'm wondering if it would make sense to match always with the telegraf configured batch size, so there's no additional config option to be set.

I think that's reasonable. I took a look at doing this, but I didn't see a way to get at the metric_batch_size configuration from within the output plugin itself. Without refactoring the way output plugins are initialized, is there a way to find the metric_batch_size from the Connect method?

Another thing that could work would be to set a larger default batch size for the wavefront SDK while leaving it non-configurable. Right now the Wavefront output's immediate_flush boolean option will cause a batch to be sent on every Write call, and I assume Write gets called once per Telegraf output batch. If metric_batch_size > 10,000, currently we would cap the batch size at 10,000, but if we change the Wavefront max batch size to to 50,000, then we could honor larger values of metric_batch_size. But it would make the batch size bigger for anyone not using immediate_flush = true.

I am conflicted about it because I like the transparency of exposing the same configuration hooks that the underlying SDK has, but I would also like the configuration to be simpler.

[LukeWinikates]

Without refactoring the way output plugins are initialized, is there a way to find the metric_batch_size from the Connect method?

@Hipska thanks for your earlier comment on this PR. Do you happen to know the answer to the above question?

[Hipska]

Good question, I checked the code and found a few things:

1. It's currently not possible to know this in the Connect method. (Didn't try this, but you could try to add a struct field with the same TOML name. I don't know if this would conflict or not. However, You would still not be able to retrieve the global agent batch size.)
2. The used wavefront.NewDirectSender and wavefront.NewProxySender methods are deprecated according to their documentation. Advised is to use the wavefront.NewSender(url) method.
3. The batch size can be derived from the amount of metrics you get in the Write method, it will be less than or equal to the batch size.
4. There seems to be already a possibility to match the metrics batch size with the http batch size for wavefront sdk, by setting the config item of immediate_flush to true, which also seems to be the default value, so I don't even know what problem you are actually trying to fix here???

EDIT: Whoops you also figured point 4 out. It indeed has problems if you increase that size..

In that case, I would give option 1 a go and also use the same technique to get the flush_interval to set FlushIntervalSeconds which is currently set to a high value. However, I saw the following about batch size in the documentation:

recommended not to exceed 40,000

[LukeWinikates]

Thanks @Hipska

For what it's worth, ideally I would love to disable the batching in the Wavefront client's batching entirely in this plugin, because the configuration is complex and hard to reason about. It would be nice to always immediately flush each batch, then do away with the FlushIntervalSeconds and immediate_flush. That's a lot of breaking changes, but maybe it'll be possible to do more simplification of the config scheme in the future.

I'm reluctant to pursue option 1 because not being able to get at the agent-level batch setting is a big caveat. I think that's more confusing than having the additional batch size setting. But now that we've talked about it more, I think a different name for the config field makes sense.

Rather than: http_batch_size
Instead: http_maximum_batch_size

Most people will be fine with the maximum of 10,000, but for folks who want to use larger batch sizes, they'll need to set the http_maximum_batch_size higher. The existing metric_batch_size field will still be the main way of setting batch sizes. I'll test this again this week, since it's been a little more than a week since I ran this in a test environment.

As far as the factory method deprecations go, I would prefer to be using non-deprecated methods. ...But I wasn't going to tackle it at the same time as this change. I'm concerned that the new single factory method may be hard to adopt here without causing potentially surprising changes for existing users.

[LukeWinikates]

Just curious, is there anything I can follow up on to help this get merged?

I recognize that the proposed change is solving for an uncommon edge case, but it would allow my team to more easily experiment with different batch sizes.

That said, if y'all think it's better not to merge this change, I do think we can eliminate this use case by redesigning the configuration schema of the wavefront plugin and how it initializes the WF client. (I just didn't think that a schema redesign or big internal changes to the plugin were a great place to start with a first PR).

I also feel bad for @-ing @Hipska over and over again but I'm not sure who else might be reviewing and commenting on PRs.

[powersj]

I approved, but I also had two questions I would like to resolve before merging. Thanks

[telegraf-tiger]

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz