-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
writing to influxdb with batches #1321
Conversation
can you explain what this is doing and why it's necessary? |
This parameter gives the option to break up the number of metrics sent per message from nstat to influxdb. In the case without the param, the nstat plugin grabs every...single...stat from /proc/netstat which results in about 75+ fields sent per measurement. Although we can tweak the size of the UDP payload being sent/received, this is an insanely large number of metrics to send per message. This feature request allows us to split the fields into multiple messages (and as an optional param only) so we can send, say 20 fields per measurement AND stay within the existing UDP payload size
|
This seems like it should be an option for the influxdb output, rather than only for the nstat plugin. @maksadbek can you look into making this change on the |
@sparrc Okay, I will look at it. |
@sparrc, how would you recommend we do that? If any measurement has over x |
@sebito91 yes, that's what I had in mind, but doing this probably should be "off" by default. It's also probably less relevant when making http writes. |
I'll get you a commit shortly, this will absolutely be an optional setting. |
@sparrc I moved the logic of breaking into batches to BTW, the build actually failed because of another plugin:
|
sorry about the inconvenience but please rebase and repush, thanks! |
Thank you @maksadbek @sparrc!! |
Any idea when we can merge this? Waiting for v1.0.0? |
aren't BatchSize and BatchLimit pretty much the same thing? Seems like the same functionality could be accomplished with just one of the two, unless I'm misunderstanding something. |
also the term "Batch" is already being used in telegraf to refer to a "batch" of points, so maybe call it something that explicitly indicates that it's only limiting the number of fields sent per metric, |
@sparrc,
I can change this logic if it is required. Yes, I agree with the name, |
it seems to me that only |
Okay, I will apply this and set |
Looks good @maksadbek. Apologies for not seeing this earlier, but I think this is actually a bug in the InfluxDB UDP client, isn't it? The UDP client should not be writing packets that are too large to write. You should be able to drop your code more or less in-place here: https://github.com/influxdata/influxdb/blob/master/client/v2/client.go#L414. Doing this, you would remove the option of configuring MetricFieldLimit, and instead you could calculate how many packets the single packet should be split into based on how many times larger it is than the max packet size. let me know what you think about implementing that, and then the Telegraf change would simply be updating the influxdb dependency commit. |
OK, I will look up at InfluxDB client code and apply changes there. |
thanks @maksadbek, ping me when you have a PR for that on the influxdb repo. |
Closing as this will be a change to the influxdb client. Feel free to reopen if that's not the case. |
@sparrc @sebito91 I've sent a PR to InfluxDB, influxdata/influxdb#6913 |
Required for all PRs: