Review: everything going out as 0.2.1 (do not merge) - #42
Open
Fiona2016 wants to merge 19 commits into
Open
Conversation
The rate a session is drawn on is the console's value falling back to init, with the application's beforeSampling given the last word. That resolution was written inline in the only branch that draws, which is fine as long as a draw is the only thing that needs to know the answer. Move it into a function that resolves and never draws, so the same question can be asked without spending a lottery ticket to find out. No behaviour changes.
Settings published from the console applied to sessions created after
they arrived, and to nothing else. For a visitor who never goes idle
that is hours: a session ends after fifteen minutes without activity or
four hours outright, so the change everyone is waiting on reaches the
people generating the most data last.
Three changes cannot wait, and they are exactly the three whose effect
on the running session can be told without drawing again:
- a session sample rate of 0 while the visitor is being collected;
- a rate of 100 while they are not;
- a stricter defaultPrivacyLevel, where every further second recorded
is a second of plaintext uploaded that masking cannot reach back for.
Each of them ends the current session; the visitor's next action starts
a new one under the new settings. Ending rather than flipping is the
point: the old session is collected to its end as it was begun, so no
replay is masked in one half and plain in the other, and no session is
invented that starts in the middle of a visit.
No other rate says anything about whether THIS session should have been
kept. Only a second draw could, and drawing twice quietly turns a rate p
into p², so every other change waits for the next session — a loosening
privacy level included, where being slow is what leaves room to undo a
mistake.
It needs no bookkeeping to stay idempotent: what it compares is what the
session was drawn under against what a draw would use now, and ending
the session is exactly what makes that difference disappear. The same
response arriving again, in another tab or after a reload, finds nothing
left to act on.
beforeSampling is now called outside a draw as well, to resolve the rate
that would actually apply, so the documentation asks for a callback free
of side effects and stable for the same input.
…ering A const enum's values are inlined at build time and every entry after an insertion shifts, so an entry wedged into the middle of a list that is otherwise upstream's is both a renumbering and a conflict on the next upstream merge. Move it to the end. Also drop a guard that restated its caller's precondition: the event is only ever emitted by the fetcher, which does not exist unless the site opted in, and reading the settings already answers with nothing when it did not.
A session that is not being collected is given no id, so no record of its draw is kept and the privacy level it was drawn under cannot be read back. The comparison fell through to the init value on every announcement and kept answering "tighter", so once an operator tightened `defaultPrivacyLevel` from the console, every sampled-out visitor was put on a loop: end the session, renew on the next click, refetch, end it again. It bought no privacy either -- a visitor who is not collected records nothing, so a stricter level has no plaintext to catch there. The rule now carries its own precondition and applies only while the session is being collected, which is also the only state in which a recording exists. Its fuel was the announcement firing on settings that had not changed: `store()` answered "stored" for a response repeating the version already held, which is the ordinary answer, since every new session refetches and most find nothing new. It now answers whether the stored version actually advanced. Three tests, each checked against the unfixed source first: a sampled-out session is left alone when the level tightens, it is still left alone as further settings arrive, and a response repeating the stored version is not announced.
Three paths the implementation documents had no test standing on them, each found by mutating the source and watching the suite stay green: - A session drawn before any settings arrived. A draw that lands exactly on the init values records nothing, so the level such a session runs under can only be read back off init -- the fallback every existing privacy test stepped around by storing settings before starting. Deleting that fallback passed the whole suite. - A response that carries no rate at all, with `beforeSampling` turning the delivered custom values into the decision. This is the "called away from a draw" contract, and both resolving the rate without the callback and bailing out when the console sends no rate passed the whole suite. - The console's kill switch, which stores a version and nothing else and so puts the rates back to the ones init passed. That is a change like any other, and where init never collected it is the decisive one. Also renames the opt-out test to what it actually pins down. Its store key is one no implementation could derive, so it cannot witness the store being left alone; what it does witness is the decision surviving an undefined `remoteConfig`.
…cts nothing The exemption that keeps a rate from ending a forced session was written for the case where ending it changes nothing: the page collects this visitor whatever the console says, so the replacement session would be the same session again. That reasoning runs out when the session is not collected. A page can adopt one drawn by a tab that never forced anything, and there a rate of 100 has something to change -- it is exactly the draw the page asked for. The guard now carries the precondition its own reasoning rests on. Also corrects two claims in the changelog entry that the code does not make good on: custom values do not always wait for the next session, since `beforeSampling` can turn them into a decisive rate -- the flagship pattern for this feature, and something the suite already pins down -- and the session after a split carries a new recording only if its draw keeps one.
…tings feat(rum): end the session when new settings decide its fate
Of the three changes that did not wait for the running session to end, this was the one with the weakest claim to a place. `setForcedSession()` already exists for "collect this visitor now" and is precise where a global rate is blunt; it was the only one of the three that raises volume, and does so the same day nobody asked for it; and nothing about "let us see more" is urgent enough that waiting for the next session costs anything that cannot be had later. The other two both undo something that cannot be undone later -- a second of plaintext already uploaded, an event already ingested. It was also what made the remaining rules hard to state. Both survivors are about a session that is being collected, so that precondition rises to the top of the function: the nesting around the privacy comparison goes, the rate check loses its conjunction, and the guard for a forced session goes back to being simply true -- ending a collected forced session on a rate really would only produce the same session again. The rule is nineteen lines with no nesting. The motivation is corrected everywhere it was stated, in the option's own documentation and in the changelog. It said this was for visitors who never go idle. It is not: settings are fetched at page load and at each new session and never on a timer, so a single tab that is never reloaded hears nothing until the four-hour cap -- an always-on screen is the case this does least for. What it actually changes is the ordinary visit, where the client downloads the new settings on the next page load and, until now, went on under the old decision for the rest of that visit.
refactor(rum): stop acting on a rate that rises to a hundred
The settings cache is keyed by application version, because two releases served at the same time are entitled to different rates and one entry between them would have each overwrite the other's at every fetch. The cost of that was an entry per release: nothing ever read or removed the one a previous release used, so on a site that deploys often they accumulated for good in a quota the host application shares. Every write now stamps when it happened, through the one path that writes an entry, and initialisation removes the entries nothing has refreshed for two days. Age is the only thing that can tell an abandoned entry from the entry of a tab still open on yesterday's release: a page that still reads its entry rewrites it at every session renewal, so the threshold only has to clear the longest session plus the longest outage worth surviving. The sweep runs before the first request rather than after each write. A session renewal is a hot path and localStorage is synchronous, and going first is what lets it free room on an origin that is already out of it - the very state the leak produces. This page's own entry is never a candidate: it holds the version floor that lets a late answer be refused. The one path that reaches an entry without storing anything - a response refused for carrying an older version - now rewrites it unchanged, so the entry a client is still asking for cannot be swept out from under it.
fix(rum): sweep the settings entries of releases nobody runs
A session sample rate published above 0 now ends the session of a visitor whose session was drawn AT 0, so collection starts at their next interaction instead of waiting for that session to rotate -- up to four hours. This is the case where waiting shows an operator who has just switched collection on nothing at all, and nothing at all is indistinguishable from a broken integration. It joins the two changes that already did not wait: a stricter privacy level, and a rate of 0. Written against the rate the session was DRAWN at rather than against whether it is being collected, which is what keeps it honest. Re-drawing every session that is not collected would spare the winners and re-roll the losers, so a fleet drawn at 20 and moved to 50 would come out at 60. A rate of 0 is the one value with no winners to spare -- nothing was collected and no coin was flipped -- so re-drawing everyone lands exactly on the new rate. A rate rising from one real value to another therefore still waits. Answering that question needed a record a sampled-out session never had. Such a session is given no id, so its draw was not recorded at all and the rate it was drawn at fell back to init -- which reads a session that lost a draw at 30 as one drawn at 0 and re-draws it, the bias above. Its draw is now recorded in the same single entry as a collected session's, under an id no session can hold, so the two cannot read each other's. That sentinel shares one id across every sampled-out session, so the id check that makes a stale record inert for a collected session does nothing here. What replaces it is that the page which draws now owns the slot: reportDraw hands over every draw rather than only the ones worth keeping, so a draw that lands on the init values clears the record instead of leaving the previous session's behind to answer for it. Resolving a rate runs the site's beforeSampling callback, so it is asked only where the answer settles whether the session ends, not once per announcement for every visitor.
…rage A rate leaving 0 ends the running session only if that session was drawn at 0, and the rate it was drawn at was read off the in-memory copy taken when the session was adopted. That copy can outlive the session: the session store tells sessions apart by id and tracking type, and two sampled-out sessions have neither an id nor a different type, so a tab whose storage poll misses the expired state between them never sees another tab end the first and draw the second. It keeps the first session's rate and, on the next delivered settings, may end a session that already lost a draw at the current rate. Read the rate off storage at the moment of the decision instead. The page that draws writes its record in the same stack that creates the session, so storage always describes the current draw, and it is the only thing the two tabs share. The tracked branch keeps the in-memory copy: a collected session carries an id, so its replacement is seen.
feat(rum): apply a rate that leaves zero to the running session
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
How it got to
publishThrough two pull requests, no direct commits:
was stated.
What 0.2.1 contains
Settings published from the console apply to sessions created after they arrive. A visitor who
loads a page downloads the new settings within seconds, and until now the SDK filed them away and
went on under the old decision for the rest of that visit — up to four hours. Two changes no longer
wait, because their effect on the running session can be told without drawing again:
defaultPrivacyLevelgets stricterBoth are gated on the session being collected: a visitor who is not records nothing and sends
nothing, so neither rule has anything to act on for them. Ending the session, rather than flipping
the running one, is what keeps a replay from being masked in one half and plain in the other.
A rate rising to 100 could be asserted about an uncollected session and is deliberately not acted
on:
setForcedSession()already covers "collect this visitor now", it is the one direction thatraises volume unannounced, and nothing about it is urgent.
What it is worth, honestly
Settings are fetched at start-up and on session renewal only, never on a timer. So this is worth
exactly as much as the client's chance to hear the news: seconds for a visitor who keeps loading
pages, and nothing at all for a single always-visible tab until the four-hour session cap — the
visibility timer keeps renewing that session, so it never goes idle and never refetches. Any other
tab the same visitor loads ends the session they share.
Where to look first
packages/rum-core/src/domain/rumSessionManager.ts—endSessionIfSettingsAreDecisive, the ruleitself: nineteen lines, no nesting.
packages/rum-core/src/domain/configuration/remoteConfiguration.ts—store()now reportswhether the response advanced what is stored, and the fetcher announces only when it did. An
answer repeating the stored version is the ordinary answer and must stay silent.
CHANGELOG.md— the## Unreleasedsection, which leads with the symptoms that will read like aregression on publish day.
State
lerna.jsonis still0.2.0and the changelog section is still## Unreleased: nothing isreleased. Cutting 0.2.1 is the version bump, the lockfile refresh and the
v0.2.1tag, which iswhat triggers the npm publish.
Local and CI checks were green on both #40 and #41:
format,lint,typecheck,build,check-packages,test:compat:tsc,test:compat:ssr, and the unit suite at 2984.