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
Load queries for dashboard page from new system.dashboards
table
#56771
Conversation
This is an automated comment for commit 663c8cd with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
Broken tests are unrelated |
Patch for development to avoid rebuilding and restarting clickhouse server on dashboard.html file changes https://pastila.nl/?0030c045/ad7a323b7a20438f9496de223b43cb63#XJFhTRYYrbf96IhbLuo5vQ== |
GROUP BY t | ||
ORDER BY t WITH FILL STEP {rounding:UInt32} | ||
)EOQ") } | ||
} |
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.
Interesting idea. What do you think about moving this into config.xml
? That way, user can customize it.
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.
Yes, interesting. I was thinking about making another writable table system.custom_dashboards
to make it changeable even in runtime... but it might be an overkill. I like your idea.
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.
I wonder how we migrate to this... Because releasing the version with charts read from config.xml
will make system.dashboards
become an empty table unless config.xml
is also updated in sync...
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.
We can add this predefined charts only if nothing had been configured by user in config. That way it will be compatible.
And also it will have one more bonus - it will make this charts available even after upgrade, since not all users sync config.xml during upgrade (they ship it with ansible or similar tools) and without defaults in code they will see nothing.
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.
This way we need to have the same charts both in some .xml and in .cpp file (unless we use some magic to include .xml file contents in our binary, as It is done e.g. for dashboard.html
).
Also if we do this transition it would be hard to add new charts just by releasing clickhouse binary. If one NOT define it's own charts, then they will be auto-updated. If one indeed define charts, then they should look for updates and deploy newer charts with the release. So it is a different deploy procedures w/ and w/o custom charts. I do not like this.
Maybe the solution is just to combine built-in charts in .cpp with charts in .xml instead of replacing?
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.
This way we need to have the same charts both in some .xml and in .cpp file (unless we use some magic to include .xml file contents in our binary, as It is done e.g. for dashboard.html).
Not really, I mean that if something is configured by user, we just consider that this is all what he wants and do not show builtins.
Also if we do this transition it would be hard to add new charts just by releasing clickhouse binary. If one NOT define it's own charts, then they will be auto-updated. If one indeed define charts, then they should look for updates and deploy newer charts with the release. So it is a different deploy procedures w/ and w/o custom charts. I do not like this.
If someone changed them, then likely he knows what he is doing (I guess <1% will do this).
For example I would likely to configure them, plus, I may not be interested in some default charts either.
Maybe the solution is just to combine built-in charts in .cpp with charts in .xml instead of replacing?
That way you cannot remove some charts...
Personally I, want this ability.
Changes:
system.dashboards
table that holds all the queries along withtitle String
anddashboard String
.dashboard.html
to SELECT queries from this system table instead of a hard-coded queries list.system.dashboards
or other user-defined tables.Benefits:
system.dashboards
.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Configurable dashboards. Queries for charts are now loaded using a query, which by default uses a new
system.dashboards
table.Documentation entry for user-facing changes