-
Notifications
You must be signed in to change notification settings - Fork 397
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
Exponential backoff when encountering HTTP 429 and add status field to grafana #689
Exponential backoff when encountering HTTP 429 and add status field to grafana #689
Conversation
Ping @robshelly |
7a7d00e
to
795616c
Compare
a0ae46b
to
8582f1d
Compare
This PR hasn't been updated for a while, marking as stale |
@pb82 do you have time to take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addreas sorry for extremely slow response at our end.
I think it's a really nice idea to start showing the status of the API in the grafanadashboard CRD:s and It would help our end users allot.
I will try to take a deeper look at this PR this week and truly understand that impact that it will have.
We are a a bit scared of adding the statuses fields to the dashboard, mostly since some people would argue that changing them would be a breaking change. So if we decide to bring this in we need to make sure it's the correct API that we provide to our end-users.
I gave a quick nit picking thing as a comment for start that I would like your feedback on.
Thanks once more for your help with this feature.
No worries on the delay, just happy to contribute! Open for suggestions on the status field/breaking change part. From my point of view this PR only adds new fields to the CRD, which shouldn't break anything by itself. However what is breaking is the change in behavior, since now the |
537910e
to
3032e01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry again for the delay @addreas.
I finally had time to check this out, kubecon and life came in the way of the review...
I would like us to get better at documentation and the best way by doing that is through code in my mind.
First I added a minor comment that I would like to see in the go code.
I would also like to see a example usage of this new value in the example folders deploy/examples/dashboards/
You can make a copy of a existing dashboard, but just use contentCacheDuration
somewhere.
Also please document this in documentation/dashboards.md
.
Other then that I think it looks great. The backoff feature is nice but what I really like is the introduction of the status fields, It will help out allot with debugging <3
If you want a extra star you could also update documentation/debug.md
with the information about the new statues field. But if you don't feel up for it I will fix that in a later PR.
Sorry again for the delay, I will be much faster to merge this when you have provided the updates.
0f0caee
to
ee164e8
Compare
b9a5cdf
to
dff8b54
Compare
dff8b54
to
af0b5c3
Compare
Ready for review, but happy to wait for the CI issues to be resolved first. |
1790a8f
to
ad4b486
Compare
3d23f42
to
2ec5f1b
Compare
2ec5f1b
to
96dffa6
Compare
@NissesSenap are you ok with merging this? |
Description
After having hit some issues with rate limiting by the Grafana API I ended up implementing this in my attempts to figure out the problem.
This PR also introduces a new status field in the grafana CRD. It adds the following output when it finds a dashboard that it should apply to it's deployment.
Type of change
Arguably all three apply, since it kind of fixes and issue, adds functionality that re-fetches periodically, and generally changes behavior. Though, I don't know how common it would be to depend on the fact that a dashboard is fetched once, and then put into the
spec.json
field, never to be fetched again.Checklist
Verification steps
While I was having issues this did work as expected and backed off on fetching dashboards from grafana.com, and the errors were nicely visible in the resource status. After getting to the bottom of the underlying issue this has been chugging along nicely updating according to the cache duration, as well.