-
Notifications
You must be signed in to change notification settings - Fork 179
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
Update ScyllaDB Monitoring to 4.4.5 #1456
Conversation
5efa11b
to
f14617c
Compare
/hold |
ouch :(
|
0c4942f
to
cee188e
Compare
annotations: | ||
description: 'OOM Kill on {{ $labels.instance }}' | ||
summary: A process was terminated on Instance {{ $labels.instance }} | ||
- alert: tooManyFiles |
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.
fwiw, this fires on my toy cluster, but this is out of scope for this PR
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.
The same in my case.
c1c2a3c
to
bc7edd9
Compare
/cc @amnonh |
@@ -0,0 +1,4680 @@ | |||
{ |
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.
@amnonh this one is not updated as I haven't found a source for it in 4.4.5 - where can I find it?
(also this will break with grafana 10.0 for the 4.5 bump)
bc7edd9
to
3965912
Compare
@zimnx @rzetelskik @amnonh PTAL, I'd like to take this to |
Something is off with the commit names - one says |
3965912
to
3cee6b7
Compare
thanks, it was a leftover from when I've initially tried 4.5 (as the docs already refer to it but it's not released) + it breaks serverless dashboard (bumps grafana to 10.0) so I went with 4.4.5. Updated. |
Dashboards require having ScyllaCluster name set to |
544dffe
to
8c1c774
Compare
61e58a6
to
7704251
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.
/assign rzetelskik
lgtm, thanks
/lgtm |
I've tried the upgrade but I was surprised the saas dashboard broke, given we only ended up upgrading minor version for Grafana.
@amnonh can you pls help us figure out why this breaks? |
|
the errors are meaningless to me, but I have manually bisected all panels and identified these 4 that break it
not yet sure why |
ok, this seems to be what's fixing it. not even sure what the meaning was as grafana expects corresponding url
|
7704251
to
5ee2c8e
Compare
tested the upgraded grafana works with SaaS dashboard without dummy links |
@tnozicka Do you need my help? there is an issue with the dashboard for the serverless, it has no repository yet, I wanted it to be part of Scylla Monitoring, but Tzach was against it. |
5ee2c8e
to
ef4406f
Compare
@amnonh I think this has fixed it Maybe you could have it within monitoring, test it and just not publish it when you make a release? |
But I have spent the day doing manual bisects between the panels and brute forcing through changing every option available which quite sucked + it delays the release. Eventually I got lucky but I believe the SaaS dashboard needs to be maintained with the other ones so you propagate the changes and test it when you change grafana version requirement. |
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.
/lgtm
Good job tracing what breaks it!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tnozicka, zimnx The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
flaked on #1476 |
Description of your changes:
This updates ScyllaDB Monitoring to 4.4.5, structures the dashboards and gzips their content so it will fit into a ConfigMap.
Which issue is resolved by this Pull Request:
Resolves #1449