feat(feedback): cache the feedback summary

[vishnupsatish]

Hopefully this one doesn't ping the whole company

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #93461      +/-   ##
==========================================
+ Coverage   88.03%   88.09%   +0.05%     
==========================================
  Files       10335    10324      -11     
  Lines      596828   595765    -1063     
  Branches    23196    22992     -204     
==========================================
- Hits       525432   524852     -580     
+ Misses      70937    70481     -456     
+ Partials      459      432      -27     

[vishnupsatish]

Changed cache key to: projects selected and year, month, day of start and end dates. Also made the cache time out after 1 hour. This is done so that if the same filters are selected, the cache stays the same for at least an hour and wouldn't immediately change if new feedbacks are submitted. Also, the time of the start and end dates are not used since if they were, the cache would become useless.

[cmanallen]

It wouldn't be useless to include the time. Including the time up to the hour still gives you an hour long cache except it has the granularity to discern ranges between the potential two day window outlined above.

[vishnupsatish]

I wonder if granularity up to the hour is needed, as I'm just thinking if the feedbacks that may be submitted within one day are enough to justify changing the feedback every hour. Maybe they are

[michellewzhang]

it really depends on the org size - for sentry javascript i don't think we even get enough feedbacks to justify recalculating a summary every hour

[vishnupsatish]

Yeah, that's true. But also, if a user chooses the stat period to be the last hour, they'd probably expect a different summary than if they chose the last 24 hours, but not including the hour may mean that those two stat periods result in the same summary, potentially confusing UX there.

[vishnupsatish]

hmm, read this again and I'm not too sure what you mean by "discern ranges between the potential two day window outlined above", since the cache times out after an hour anyways. Or is that better done by adding the hour in the cache key and timing out in 2 hours (or more)? (cc: @cmanallen)

Edit: never mind, got confused here, I think it does make sense to add hour to the cache key

[vishnupsatish]

hmm, django's default seems to be five minutes but I don't know if we've overridden that anywhere. I'll set it to one day just to be on the safer side

[aliu39]

Why raise it to one day? Timeout should match the granularity right?

[vishnupsatish]

Curious but why does timeout have to match granularity? For custom date ranges, an hour timeout seems too short. And if the stats period includes the current time, by including hour don't we regenerate every hour anyways?

[aliu39]

Sorry I was thinking about this wrong, 1 day seems good to me. To summarize my understanding

• users looking at hour-level views will likely use relative time ranges, through statsPeriod and "last X hours" view. These naturally update every hour with the cache key.
• users looking at fixed time ranges will likely do it at day+ granularity using the calendar selector we have in the UI. These will update every day with the cache timeout.

[vishnupsatish]

Yes, that understanding matches what I had in mind. Including hour in the cache key also allows the distinction between different hour-level time ranges to be made (within the same day).