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

Exponential backoff when encountering HTTP 429 and add status field to grafana #689

Merged
merged 9 commits into from Jul 12, 2022

Conversation

addreas
Copy link
Contributor

@addreas addreas commented Feb 16, 2022

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.

status:
  dashboards:
  - folderId: 1
    folderName: grafana-operator-system
    hash: 3e6c0fc9d47b809373eaf280f19c8d28a3d787eb8fbb7dbf151531b81a7c80b5
    name: keycloak-dashboard
    namespace: grafana-operator-system
    uid: a99708b864c0a9b9708edcfa8c8834c3c8b71000

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.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • This change requires a documentation update
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a test case that will be used to verify my changes
  • Verified independently on a cluster by reviewer

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.

@NissesSenap
Copy link
Collaborator

Ping @robshelly

@pb82 pb82 self-assigned this Mar 22, 2022
@github-actions
Copy link

This PR hasn't been updated for a while, marking as stale

@github-actions github-actions bot added the stale label Apr 21, 2022
@NissesSenap
Copy link
Collaborator

@pb82 do you have time to take a look?

Copy link
Collaborator

@NissesSenap NissesSenap left a 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.

@addreas
Copy link
Contributor Author

addreas commented May 9, 2022

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 spec.json field isn't updated with the content. Perhaps this will interact in an unexpected way with existing deployments?

@NissesSenap NissesSenap assigned NissesSenap and unassigned pb82 May 24, 2022
Copy link
Collaborator

@NissesSenap NissesSenap left a 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.

api/integreatly/v1alpha1/grafanadashboard_types.go Outdated Show resolved Hide resolved
@NissesSenap NissesSenap changed the title Exponential backoff when encountering HTTP 429 Exponential backoff when encountering HTTP 429 and add status field to grafana May 29, 2022
@addreas addreas force-pushed the dashboard-fetch-rate-limit branch 3 times, most recently from 0f0caee to ee164e8 Compare May 31, 2022 14:12
@NissesSenap
Copy link
Collaborator

golangci-lint needs an upgrade, I have fixed that in #771.
If you don't want to wait for that to get merge just bump the ci to golangci/golangci-lint-action@v2.5.2.
@addreas other then that is it ready for review?

@addreas addreas force-pushed the dashboard-fetch-rate-limit branch from b9a5cdf to dff8b54 Compare June 7, 2022 18:12
@addreas addreas force-pushed the dashboard-fetch-rate-limit branch from dff8b54 to af0b5c3 Compare June 7, 2022 18:22
@addreas
Copy link
Contributor Author

addreas commented Jun 7, 2022

Ready for review, but happy to wait for the CI issues to be resolved first.

@addreas addreas force-pushed the dashboard-fetch-rate-limit branch from 2ec5f1b to 96dffa6 Compare July 5, 2022 12:09
@pb82
Copy link
Collaborator

pb82 commented Jul 12, 2022

@NissesSenap are you ok with merging this?

@NissesSenap NissesSenap self-requested a review July 12, 2022 11:04
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.

None yet

3 participants