Refactor dashboard page to be data-driven, and expose resulting config#183
Conversation
db203a4 to
9522e3c
Compare
9522e3c to
72c6b16
Compare
|
I decided to do a little more in this PR. I just pushed a commit that also extracts a |
69ec187 to
8dd4da4
Compare
| @throughput_report_data = RailsPerformance::Reports::ThroughputReport.new(db).data | ||
| @response_time_report_data = RailsPerformance::Reports::ResponseTimeReport.new(db).data | ||
| @percentile_report_data = RailsPerformance::Reports::PercentileReport.new(db).data | ||
| @widgets = RailsPerformance.dashboard_charts.map do |row| |
There was a problem hiding this comment.
maybe code could be simpler if you consider widgets always as array of arrays, even if only one element is inside
@widgets = RailsPerformance.widgets_config_for(:dashboard).map do |row|
Array.wrap(row).map { |class_name| DashboardCharts.const_get(class_name).new(@datasource) }
endand in view you can always render an array of arrays
UPDATE: I see that for single widget we don't render widget inside columns/column. So maybe this is not needed.
But it would be worth to think about it and maybe try.
Also I think method like widgets_config_for will be needed for other pages in the future.
There was a problem hiding this comment.
okay, another comment, just want to challenge - for main dashbiard page I see widgets could be useful, but what about other pages
maybe for System metricts also could be useful.
For requests/etc probably not?
But I like the idea of widgets, we can allow users to create own widgets with own metrics. Example:
rails_performance.config.widgets[:dashboard] << RallsPerformance::Widget.new(label: "Users", data_type: :single_value, data: -> { User.count } )
rails_performance.config.widgets[:dashboard] << RallsPerformance::Widget.new(label: "ReportGeneration", data_type: :sparkline, data: -> { SomeReport.group(:date_on).count } )There was a problem hiding this comment.
letting you to think more about it, in general PR can be merged, because it's changing some internal things, without exposing new configuration options
There was a problem hiding this comment.
I like your idea of Array.wrap(row).map a lot! Maybe the CSS can be adjusted so that this can be possible with the same markup for single and multiple items in a row, to make the HTML really simple. I'm not an expert in CSS, but maybe something like display: flex? I will try this now.
There was a problem hiding this comment.
Regarding the configuration API, yes! I like the idea to allow users to create their own widgets, too. I am already doing this in my projects :) And I'm sure that the exact details of the configuration API will be something you and I discuss and adjust, before we make it public and document it in the README. For now, this is mostly exploration, by making the pages configurable, one at a time, one PR at a time. When they're all configurable, then I think we can look at them all and decide what the best public API should be.
The reason that I do think it makes sense to try to make the Requests page configurable is because I want to try make the entire UI configurable, including which pages are displayed in the nav at the top, so that you could add a custom page of custom widgets, for example. Or remove whole pages if you don't want them. All in the config. But yeah that's for a future PR! Just making the dashboard page configurable is the focus for this PR.
There was a problem hiding this comment.
Up to you, of course! But personally speaking, I think this PR is ready to be merged. As you say, its all internal changes... we're not committing to a configuration API just yet. This helps us explore what that might look like. I already have some follow-up PRs ready to go that build on top of this one and explore the space further.
There was a problem hiding this comment.
merged, I'll keep it in main branch, and won't release new version
|
@igorkasyanchuk Okay I've just pushed another commit that switches the dashboard page from column-based to row-based HTML and CSS. Now rendering that page is effectively just |
Hi Igor!
I found some time today to further pursue my agenda of making
rails_performancemore data-driven, and thus more configurable.This PR is very similar to the work already done to make the System page data-driven, so nothing too ground-breaking here. I think the only new part is my decision to represent the P50, P95, and P99 cards as a multi-column row with a nested array:
What do you think?
In future PRs, I'll keep pursuing this direction of refactoring page layouts into data-driven config settings, until we eventually end up with something like:
But one step at a time!