-
Notifications
You must be signed in to change notification settings - Fork 1
Feat/episode sparklines #1386
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
Feat/episode sparklines #1386
Conversation
| end | ||
| def calculate_episode_trend(episode, prev_episode) | ||
| return nil if (episode.first_rss_published_at + 1.day) > Date.utc_today | ||
| return nil unless episode.present? && prev_episode.present? |
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.
2 guards here: first, don't calculate a trend if the episode just dropped and it hasn't been 24 hours. second, don't calculate a trend if there isn't a previous episode to calculate from.
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.
Should this be Time.now, not Date.utc_today?
If it's first_rss_published_at is 12/2 9:18 UTC... are we waiting until 12/3 00:00 UTC or 12/3 9:18 UTC to show it?
| uniques_selection: "last_7_rolling" | ||
| ) | ||
| .merge(metrics_params) | ||
| ((ep_sum.to_f / prev_ep_sum.to_f) - 1).round(2) |
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.
correct math/thinking? the staging data numbers seem a bit outlandish (> 10% changes) because of how extreme the differences seem, but i'm not sure if this looks tamer in prod.
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.
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.
Is there a chance we can load this against a prod db? This way we might be able to see the edge cases a little easier.
| text.gsub(/[&<>]/, "&" => "&", "<" => "<", ">" => ">") | ||
| end | ||
|
|
||
| def episode_trend_pairs(episodes) |
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.
trying not to do extra querying in podcast_metrics_controller so i pair episodes with their previous episode here.
app/helpers/metrics_helper.rb
Outdated
| end | ||
|
|
||
| def parse_trend(trend) | ||
| return if trend.nil? |
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.
for the view, i use this helper to determine the visuals. for trending_flat i've arbitrarily chosen 5%, but could easily change it to 2 or 3% or whichever number we decide on makes sense.
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.
@radical-ube 5% seems a good place to start.
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 about this at various tiers though. like a podcast going from 2000 to 2100 (5% increase) maybe feels kinda small and not too significant. but 20000 to 21000 (still 5%) might be a bit more notable. so are there certain threshholds, maybe based on the size of the podcast or some other characteristics, where we do want to highlight, "hey, your latest episode is trending (up or down) much larger than usual" that might not be solely based on the percentage?
or should it be equal across all podcasts?
| @@ -1,27 +1,34 @@ | |||
| import { Controller } from "@hotwired/stimulus" | |||
| import { buildDateTimeChart, buildDownloadsSeries, dynamicBarAndAreaType } from "util/apex" | |||
| import { buildSparklineChart, destroyChart } from "util/apex" | |||
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.
repurposing this controller for the sparklines charts.
| <% trend = parse_trend(episode_trend) %> | ||
| <div class="d-flex align-items-center <%= trend[:color] %>"> | ||
| <span><%= trend[:percent] if trend.present? %></span> | ||
| <span class="material-icons ms-1" aria-hidden="true"><%= trend[:direction] if trend.present? %></span> |
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.
trend shouldn't render unless it has passed all the conditionals
| data-controller="apex-downloads" | ||
| data-apex-downloads-id-value="<%= episode.guid %>" | ||
| data-apex-downloads-downloads-value="<%= downloads.to_json %>"> | ||
| <div data-apex-downloads-target="chart"></div> |
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 sparkline chart is positioned/translated to fill the episode card. it has the tooltip removed and also z-index 0 so that it is not interactable and doesn't interfere with clicking the episode link.
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.
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.
there is currently a vertical gradient on the chart. i can make the border line transparent if that's helpful in seeing 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.
+1 to making the the border line transparent.
Can we change the colors in the gradient?
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.
what colors do you want?
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.
| <%= form.date_field :date_end, class: "d-none", data: {apex_nav_target: "endDate", action: "change->click#submit"}, value: date_end %> | ||
| <%= form.select :interval, interval_options, {selected: interval}, class: "d-none", data: {apex_nav_target: "interval", action: "change->click#submit", path: "episodes"} %> | ||
| <%= form.submit class: "d-none", data: {click_target: "submit"} %> | ||
| <div class="episode-card-info inside-card flex-fill px-3 py-2 position-relative"> |
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.
repurposing this partial for the published episode cards.
| Clickhouse not connected | ||
| </div> | ||
| <% end %> | ||
| <% elsif card_type == "blank" %> |
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.
don't render anything for the sparklines if clickhouse is not connected.
| <h2 class="h5 episode-card-title m-0"><%= link_to ep.title, episode_path(ep) %></h2> | ||
| </div> | ||
| </div> | ||
| <% if Rails.env.development? %> |
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.
keeping the sparklines in development until we release metrics > feeder.
cavis
left a comment
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.
Structurally, I think this all looks right. My questions are about the specific data/calculations. (Also see: slack thread in tech-feeder!)
| end | ||
| def calculate_episode_trend(episode, prev_episode) | ||
| return nil if (episode.first_rss_published_at + 1.day) > Date.utc_today | ||
| return nil unless episode.present? && prev_episode.present? |
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.
Should this be Time.now, not Date.utc_today?
If it's first_rss_published_at is 12/2 9:18 UTC... are we waiting until 12/3 00:00 UTC or 12/3 9:18 UTC to show it?
|
|
||
| @recently_published = @podcast.episodes.published.dropdate_desc.limit(3) | ||
| @recently_published_episodes = @podcast.episodes.published.dropdate_desc.limit(4) | ||
| @trend_episodes = @podcast.default_feed.episodes.published.dropdate_desc.where.not(first_rss_published_at: nil).offset(1).limit(4) |
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.
here i am pulling the episodes to be compared against in the trend calculation. offset by 1 because i don't want the most recent one.
| text.gsub(/[&<>]/, "&" => "&", "<" => "<", ">" => ">") | ||
| end | ||
|
|
||
| def episode_trend_pairs(episodes, trend_episodes) |
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.
and here i realized that i didn't want to necessarily pair episode and trend episode 1:1. i still needed to check if an episode should have a trend number at all, and then count which trend_episodes have been used in pairing. in most cases this should still line up 1:1, but should catch the cases where there's an episode that is not in the default feed and needs to be skipped in trend calculation.
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.
Since these are in separate turbo requests, you could just find which episode you should compare to in that other request? It avoids any kind of N+1, not attempting to pair them all upfront. And makes the outer request quicker.
Episode.where(podcast_id: ep.podcast_id, first_rss_published_at: ..ep.first_rss_published_at).order(first_rss_published_at: :desc).first(Not a request for change in this PR, just something to think about ... optimize for the outer/first GET request).
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.
added to #1400
cavis
left a comment
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.
Couple minor things on data loading, but otherwise this is looking good! Also, I understand what a sparkline is now.
| before_action :set_date_range, except: %i[dropdays] | ||
| before_action :set_uniques, only: %i[show uniques] | ||
| before_action :set_dropday_range, only: %i[show dropdays] | ||
| # before_action :check_clickhouse, except: %i[show] |
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.
kind of an aside (for me) for later, but i frequently (but not consistently always) get errors when trying to refresh the view and get aclickhouse_connected "no method" error. including ClickhouseUtils module both in this controller and the MetricsUtils concern didn't seem to fix, so i'll have to take a closer look in a future PR.
| dropday_range: 7 | ||
| ) | ||
| .merge(metrics_params) | ||
| def publish_hour(episode) |
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.
is this accurate? trying to address the .beginning_of_hour comment but also catching episodes that don't have .first_rss_published_at.
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 would probably just return a 0 / empty array, instead of querying clickhouse at all, if that's nil.
The implication is, this episode was never written to the default feed. So you're not going to find any data in Clickhouse.
Kind of surprised it got to querying clickhouse - i thought you had some early-returns when first_rss_published_at was nil?
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.
added to #1400
cavis
left a comment
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.
👍 looks good to me! Onwards!
| dropday_range: 7 | ||
| ) | ||
| .merge(metrics_params) | ||
| def publish_hour(episode) |
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 would probably just return a 0 / empty array, instead of querying clickhouse at all, if that's nil.
The implication is, this episode was never written to the default feed. So you're not going to find any data in Clickhouse.
Kind of surprised it got to querying clickhouse - i thought you had some early-returns when first_rss_published_at was nil?
| text.gsub(/[&<>]/, "&" => "&", "<" => "<", ">" => ">") | ||
| end | ||
|
|
||
| def episode_trend_pairs(episodes, trend_episodes) |
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.
Since these are in separate turbo requests, you could just find which episode you should compare to in that other request? It avoids any kind of N+1, not attempting to pair them all upfront. And makes the outer request quicker.
Episode.where(podcast_id: ep.podcast_id, first_rss_published_at: ..ep.first_rss_published_at).order(first_rss_published_at: :desc).first(Not a request for change in this PR, just something to think about ... optimize for the outer/first GET request).



This PR adds sparklines and trend numbers to the episode cards on the Podcast dashboard. With the updated designs, I am slowly refactoring the
podcast_metrics_controllerand various apex stimulus controllers because some of the code can still be repurposed. The updated podcast dashboard has several charts in it, so I plan on using thepodcast_metrics_controllerto handle as much of the metrics querying as possible so as not to bloat thepodcast_controller.With the case of the sparklines, each episode card has its own turbo frame, which sources from
podcast_metrics#episode_sparklinefor each of their individual querying needs.alltime_downloadspulls data for the sparkline itself, while theepisode_trendinclude the dropdate queries for the episode and the previously published episode.There is a little bit of a question around what is our opinion of
trending_flat, and the threshhold we would consider whether a change is significant enough to signal trend in a particular direction.@cavis for code review
@brandonhundt for design review