-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Live.Dashboard.Pages continued #5975
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
Changes from all commits
2b006b8
9ca02fa
36949d7
1543614
e298dcf
c17031b
2f4bff3
2091e5b
c5a0d53
0799cf1
a5cd80a
9f7695b
796a655
9340e3a
fa44c4d
3240cc3
a9cdf38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,8 @@ defmodule Plausible.Stats.QueryInclude do | |
| trim_relative_date_range: false, | ||
| compare: nil, | ||
| compare_match_day_of_week: false, | ||
| legacy_time_on_page_cutoff: nil | ||
| legacy_time_on_page_cutoff: nil, | ||
| dashboard_metric_labels: false | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion, non-blocking: Since metrics are ordered in the response and the dashboard knows what it requested (choose_metrics), I think we should provide / map the labels there. Then this struct will only contain fields that influence the actual database query.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see anything wrong with keeping logic unrelated to the ClickHouse SQL query in However, I agree that the mapping part is unnecessary. I think you reviewed before I pushed this commit: 9f7695b. I've changed There's another edge case that will be a bit easier to handle this way. Revenue metrics are sometimes dropped silently (when mixing currencies in goal filters). So even though a LiveView report always asks for them, they might not be returned.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, it's actually weird that time labels don't go through the database. It's very database adjacent though. I see your point about it making an edge case easier to handle, I disagree that it's worth the tradeoff (of making Query and its result more complex). I suspect we'll be tempted to put a lot of configuration in QueryInclude. I think we should do so only for things that can't be easily handled another way. Metric labels can be. Still, it's not a blocker for merge.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't say this adds much complexity into the query function. A bit more code and a new input/response field, sure, but it doesn't make it more difficult to reason about. But I see your concern because the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Whether something is easy or hard to reason about is quite hard to discuss. My argument is that we don't want it in What's the responsibility of the Query module? As I see it, it's to represent a request for data, and must contain exactly what is needed to get that data from the database. If we keep to this responsibility, we can say that as long as one doesn't touch
Regarding cleanup, I'm not sure that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand your point about separating responsibilities but I'm not seeing how this new include field could cause us any pain in the future. The benefit of not having to worry about metric mapping in every LiveView stats report module sounds far more real. That said though, the query refactoring work is still ahead and we can consider it further down the line.
It could also be a nested struct with the fields explicitly defined with default values. The point would be to separate the input fields from those that are attached to the query automatically.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@RobertJoonas, that overall point is very good!
Thanks for considering it! This one field is definitely innocent and good code. It just got me thinking on why we need to do it this way. I believe by doing it a little bit differently, we can have the benefits that both of us brought up (shared code between LiveView stats reports, more singular responsibility for QueryInclude). To illustrate what I had in mind, I put together this #5983. |
||
|
|
||
| @type date_range_tuple() :: {:date_range, Date.t(), Date.t()} | ||
| @type datetime_range_tuple() :: {:datetime_range, DateTime.t(), DateTime.t()} | ||
|
|
@@ -22,6 +23,7 @@ defmodule Plausible.Stats.QueryInclude do | |
| compare: | ||
| nil | :previous_period | :year_over_year | date_range_tuple() | datetime_range_tuple(), | ||
| compare_match_day_of_week: boolean(), | ||
| legacy_time_on_page_cutoff: any() | ||
| legacy_time_on_page_cutoff: any(), | ||
| dashboard_metric_labels: boolean() | ||
| } | ||
| end | ||
Uh oh!
There was an error while loading. Please reload this page.