Broadcast tracker script config updates#5806
Conversation
|
| require Logger | ||
|
|
||
| @spec broadcast_put(any(), Keyword.t()) :: :ok | ||
| @spec broadcast_put(any(), any(), Keyword.t()) :: :ok |
lib/plausible_web/tracker.ex
Outdated
|
|
||
| def broadcast_script_update(tracker_script_configuration), | ||
| do: | ||
| PlausibleWeb.TrackerScriptCache.broadcast_put( |
There was a problem hiding this comment.
minor, just noticed this - by convention, *Web namespace if for HTTP things. Cache is completely agnostic in that sense, even if Web things are its clients.
There was a problem hiding this comment.
Thanks, I hadn't considered the namespace question. At the moment we have the script at Plausible.Site.TrackerScriptConfiguration. By following existing naming principles, its Cache should be at Plausible.Site.TrackerScriptConfiguration.Cache, correct?
There was a problem hiding this comment.
Ah damn, it's not so straightforward. It caches the script, not the configuration. The script's module is PlausibleWeb.Tracker. So PlausibleWeb.Tracker.Cache or refactor also PlausibleWeb.Tracker to Plausible.Tracker?
There was a problem hiding this comment.
refactor also PlausibleWeb.Tracker to Plausible.Tracker
Probably this, but we can live with not doing it
| end | ||
|
|
||
| defp cache_content(tracker_script_configuration) do | ||
| def cache_content(tracker_script_configuration) do |
There was a problem hiding this comment.
a bit unclear what is this function for. returns either always true or some string for CE? And build_script for CE does eex-like string replace but not using EEx? Very confusing. 🤔
There was a problem hiding this comment.
Thanks! Agreed that this could use a comment. Will put something like the following:
On EE, we cache that this particular script exists to prevent Postgres stress from lookups of non-existent script IDs. The heavy lifting of caching is left to the CDN. If the ID exists, we always generate a fresh version of the script.
On CE, since we don't anticipate them using a CDN, we cache the rendered script.
Regarding the CE solution, it means that every refresh interval, every script is re-rendered by the server. This doesn't seem ideal actually.
Regarding building the script, we planned to use EEx but there were issues. There's a discussion on the PR that introduced it.
There was a problem hiding this comment.
Regarding the CE solution, it means that every refresh interval, every script is re-rendered by the server. This doesn't seem ideal actually.
Just realised that only every updated script is re-rendered by the server on CE. That's not a problem.
aerosol
left a comment
aerosol
left a comment
There was a problem hiding this comment.
The CI error looks related to the change
lib/plausible_web/tracker.ex
Outdated
| ) | ||
| ) do | ||
| created_config = | ||
| Repo.preload(created_config, :site) |
There was a problem hiding this comment.
Do we need the two extra db queries is site is already available?
There was a problem hiding this comment.
Thanks!
I think we don't. In these functions, site is actually available as an argument, so theoretically we don't need additional DB queries at all.
Refresh all updated scripts -> load tracker script config with site association, run generate_script/1
Get or create config -> (site is argument) receive tracker script config without site association, run generate_script/2
Update config -> (site is argument) receive tracker script config without site association, run generate_script/2
I started to refactor it but it got messy so I pushed the interim state.
There was a problem hiding this comment.
What I meant was
%{created_config | site: site}if this doesn't carry any risk of outdated reads
There was a problem hiding this comment.
if this doesn't carry any risk of outdated reads
Good question.
What we need from the site association is site.domain for building the script for the cache
ON CE
T1 This conn: Starts process to update tracker script config
T2 Other conn: Starts process to update domain name
T3 Other conn: Finishes process to update domain name
T4 This conn: Expands stale site domain to tracker script
T5 This conn: Caches the stale tracker script and broadcasts this to all nodes
T6 Application: Checks for tracker_script_config entities that have been updated in the last 15 minutes, doesn't find anything
Result: stale tracker script cached until next refresh_all, which may be in 180mins
ON EE
T1 This conn: Starts process to update tracker script config
T2 Other conn: Starts process to update domain name
T3 Other conn: Finishes process to update domain name and starts a job that clears the cached tracker script on CDN in 10s
T4 This conn: Caches that a tracker script with this ID exists and broadcasts this to all nodes
T5 Application: Checks for tracker_script_config entities that have been updated in the last 15 minutes, doesn't find anything
Result: local cache is correct, CDN cache is purged
So there is some risk with outdated reads on CE. I don't think it's a huge risk, because we keep handling events with the stale domain for a while.
But I also see other risks doing it like this. The site from the function arguments has a bunch more associations loaded. By setting it to the struct we can accidentally expose those associations down the line.
There was a problem hiding this comment.
@aerosol I know you already approved the version with the preloads, but I didn't think it was good enough, so I had another look at this.
First I tried injecting the site from args only when it doesn't exist and is needed, accepting the small risk on CE from stale domain reads.
Then I realized we don't need to accept that small risk: on CE we don't need to optimize postgres queries so much.
I'm now proposing we reload the config with necessary associations after insert and update operations, but only on CE. I also changed the base query a little bit, so as not to select the whole site when only one field is needed.
apata
force-pushed
the
script-v2/broadcast-cache-updates
branch
from
f104dfa to
472847b
Compare
2 participants
Changes
Broadcasts tracker script config creates / updates to other app nodes, reducing the time between creating / updating a script and being able to reliably receive it.
Tests
Changelog
Documentation
Dark mode