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: allow defining a rate limit to call the newrelic api #2741

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

woehrl01
Copy link
Contributor

@woehrl01 woehrl01 commented Aug 30, 2024

Fixes #2582

I appreciate some feedback what a good default value could be.

Not sure if it should be change to reflect this limit: https://docs.newrelic.com/docs/apis/nerdgraph/nerdgraph-usage-limits/

@woehrl01 woehrl01 marked this pull request as draft August 30, 2024 19:13
@pranav-new-relic
Copy link
Member

Hey @woehrl01, thanks for writing this piece of code. This looks interesting upon first look. However, our team shall need quite some time to get to this, considering other stuff we've prioritized, and the fact that unlike any other PR, we would not only need to review this PR but also deeply investigate into the outcomes of having this merged; test it out thoroughly to make sure no unexpected repercussions are seen, or even have a conversation within the team around the feasibility of having this change deployed, given this could directly affect the mechanism by which API calls are routed out of the New Relic Terraform Provider. Thank you for understanding :) in the meanwhile, it'd be great if you can just write a short brief of the exact change you intend to bring by these changes in the description (if you can just briefly explain, so this can also be useful to others on my team who might want to take a look at this PR later on). Thanks!

@deadlysyn
Copy link

appreciate the need to prioritize and asses impact. the issue we have (same as reported in the linked issue) is inability to update managed resources without hitting the rate limit. we have a lot of SLOs for example, and applying a change which affects them all (window, runbook, tags, whatever) fails with the rate limit.

the odd thing is, the triggered rate limit does not show up in the limits ui. i am not sure if that is because it is a limit which isn't exposed there, or implies the provider's current value is far below the account value. it would make sense if those were closely aligned, or the ui made it clearer what was being hit.

it seems a workaround would be splitting our modules into smaller pieces, but that is sub-optimal organizationally (these are tightly coupled resources) and difficult to find the right seams without clearer insight into the limits.

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Rate Limiting for Total Requests per Minute
3 participants