Skip to content
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

feat(inputs.influxdb_v2_listener): Add support for rate limiting #15361

Merged

Conversation

LarsStegman
Copy link
Contributor

Summary

As discussed in #15340 it can be beneficial for the Influx listener to rate limit any client pushing data. This change is compatible with the API spec described here: https://docs.influxdata.com/influxdb/v2/api/#operation/PostWrite

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #15340

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels May 15, 2024
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this up. Played with this locally this morning.

There is a footgun if the max_undelivered_metrics size is greater than the number of metrics sent in a batch to Telegraf the batch would never get accepted :\ That's a little different from the other plugins with tracking metrics as they pull/consume as they can.

@powersj powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label May 15, 2024
@LarsStegman
Copy link
Contributor Author

LarsStegman commented May 15, 2024

Good catch!
I think it would be best to send a 413 status back in that case. That kind of follows the API docs. Do you agree with this?

It should be logged as a warning(?). I'll update the code tomorrow.

@powersj
Copy link
Contributor

powersj commented May 15, 2024

Good catch! I think it would be best to send a 413 status back in that case. That kind of follows the API docs. Do you agree with this?

Yep! 413 sounds like the right return code.

It should be logged as a warning(?). I'll update the code tomorrow.

Hmm a warning, an error, or ignored? We usually error when we cannot parse data. It opens the question if we should do the same for the 429

@LarsStegman
Copy link
Contributor Author

Hmm a warning, an error, or ignored? We usually error when we cannot parse data. It opens the question if we should do the same for the 429

I chose to ignore it. It is not really an error in the program, just user error. I did add a debug entry, since that might help the user pin down why batches are being rejected.

@telegraf-tiger
Copy link
Contributor

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LarsStegman!

@srebhan srebhan assigned powersj and unassigned srebhan and DStrand1 May 16, 2024
@srebhan
Copy link
Contributor

srebhan commented May 16, 2024

@powersj assigning it back to you as the code was changed since your accept. Feel free to merge if you are still OK with the current state. ;-)

@LarsStegman LarsStegman requested a review from powersj May 16, 2024 08:55
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

@powersj powersj merged commit dcb6177 into influxdata:master May 16, 2024
27 checks passed
@github-actions github-actions bot added this to the v1.31.0 milestone May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

influxdb_v2_listener rate limit
4 participants