feat(rum): add sessionReplayOnError - #24
Open
Fiona2016 wants to merge 25 commits into
Open
Conversation
…export from mockProfiler imported registerCleanupTask and getGlobalObject from the rum package rather than core, and profiler.spec.ts imported from package names this repository does not publish. Since mockProfiler is re-exported from the rum test barrel, the broken imports took every spec that touches that barrel down with them - around 220 tests never ran.
A session drawn by this rate records from the start but uploads nothing until it reports an error. If none ever happens, nothing is sent and the session is never stored. On the first error the withheld buffer is released and recording continues normally, so the replay covers what led up to the error rather than starting at it. The buffer is bounded on both axes. Time: a buffer that spans more than a minute is dropped and restarted from a fresh full snapshot, so what is released stays a minute at most. Size: the existing segment byte limit still applies while withheld, and restarts are spaced out so that a document whose full snapshot alone exceeds that limit degrades instead of restarting in a loop. A withheld buffer belongs to the session that produced it. It is released only when that same session reports the error - if the session expires or is renewed first, the records are dropped, so an expiry can never turn into an upload for a session that never errored. Buffers that are dropped roll back their replay stats, and has_replay is not reported while a replay is being withheld, so neither the counters nor the link offer a replay that does not exist. Errors raised by the SDK about its own transport do not release anything: those are our failures, not the application's, and counting them would make every session an error session wherever our endpoint is unreachable.
The flush path derived the same thing twice under two names, and the mapping of the internal checkout reason onto a schema value only ever had one caller.
A page-exit rotation used to throw the buffer away, and with it the full snapshot a released replay has to start from - everything recorded afterwards is incremental and cannot be played on its own. Switching tabs raises this exit, and the page comes straight back, so an error reported after that would have released a replay that renders as good as nothing until the next view. Nothing can be sent while withheld, so there was never anything to gain from the rotation. A page that is really unloading takes the buffer with it either way.
…ased buffer Only the rotation notices that the withheld replay has been released, so a session that expires within one rotation of its own error still loses what the error had earned. Closing it would mean asking the session manager on every record.
The rollback that gives a dropped buffer's index_in_view back only lands when the encoder finishes, which is always a turn later. Restarting from a fresh full snapshot emitted records right away, so the next segment took its index before the rollback arrived - and once that session errored, two uploaded segments claimed the same index within one view while nothing claimed the first. Any error session that spends a minute on one view before erroring hit it. The restart now happens where the rollback lands. Also corrects a comment: a session expiring right after its own error does not lose the buffer. The history entry is still open when the recorder is stopped, so the stop flush sees the session as released and sends.
Without it, a replay collected under this rate is indistinguishable from one collected unconditionally once it has been uploaded - the two cost differently and answer different questions, and nothing downstream could tell them apart.
Marking a session as having reported an error is only useful to a session that is withholding its replay. Doing it for every session wrote the session store for customers who enabled no error sampling at all, and that write also pushes the session's expiry out, which moves where their sessions end.
…take it Flushing a segment always waits for a round trip to the deflate worker, because the trailer is written just before finishing. The collection state is reset synchronously, so a record arriving during that round trip created the next segment while the dropped one was still counted: two uploaded segments then claimed the same index_in_view, and index 0 was never uploaded at all. Each counter is now given back in the phase it was taken in - the segment count synchronously, the record and byte counts in the flush callback.
…d on A store write goes through a lock and can be retried for up to a second, and other tabs write the same store meanwhile - so the state a write lands on is not necessarily the one it was decided against. Updates are now expressed as a function of that state, and returning nothing makes the write a no-op, which is what a caller needs to say "only if this is still the session I meant".
Marking a session as having errored merged into whatever session the store held at the moment the write went through. A session that rolled over while the write waited for the lock - or that another tab renewed - was marked instead, and then uploaded a whole session that never reported anything. The mark now names the session it belongs to and is dropped if that session is gone. The same mark is also applied to the in-memory session straight away rather than only once the write lands, because until then the withheld buffer still reads the session as withholding: an error followed closely by the page or the session ending threw away the very buffer the error was meant to release.
Dropping a withheld buffer restarts it from a fresh full snapshot, and that runs in a flush callback which only arrives after a round trip to the deflate worker. Recording stopped in between still got a full re-serialization of the document, and its records counted into the replay stats with no segment to hold them.
Whether a withheld replay had been released was inferred from the session no longer withholding. That is true for a reason other than an error: a session store is shared with every other SDK bundle on the domain, and one that predates these tracking types does not recognise them, so it redraws the session and rewrites the type. The withheld replay was then uploaded for a session that never reported anything. Release now requires the session to still be one whose replay is kept on an error. Any other transition ends the buffer instead of sending it.
`has_replay` was set for any view that had replay stats at all. A withheld buffer that is dropped rolls its segments back, so a view can be left with stats and no replay - and the session then offers a replay nothing can play. It now takes a segment that survived.
Forcing a replay was only ever applied to a session whose replay was off, and only when the
recorder was not already running. A session withholding its replay fails both: it is
recording, and its replay is not off. So the session manager's rule that a forced replay
wins over withholding, and releases the events with it, could not be reached from the
public API at all - `startSessionReplayRecording({ force: true })` did nothing for exactly
the sessions where it has something to do.
sessionReplayOnErrorSampleRate is drawn from what the plain replay rate did not take, so some perfectly valid configurations can never draw anything: a plain rate of 100 leaves it nothing, a session rate of 0 leaves no session to draw from, and starting the recording manually leaves nothing recorded to withhold. Each of those now says so once at init. The option's own description also led with "the percentage of tracked sessions", which is not the base it is drawn from.
Every one of these covers a one-line change that would otherwise ship a feature that silently does nothing: the rate never reaching the built configuration, the recording not starting on its own, the release predicate inverted so no withheld replay is ever sent, the error mark no longer naming its session so the store refuses it, the internal buffer checkout reason leaking into the segment schema, the rate no longer range-checked, and the error-replay marker no longer emitted. Two fixtures were lying as well: non-error events carried an `error` object no real event has, which hid the order the guards have to read them in.
The segment collection took a buffering argument that defaulted to one that never withholds. Its only production caller always passes a real one, so the default existed for a single test that omitted the argument. That test now says what it means.
Excluding a view whose withheld buffer was dropped was right, but counting segments to do it was not: when a host bridge takes the records there is never a segment to count, so a webview session - which cannot enable any of this - stopped reporting `has_replay` at all. Records are rolled back with the buffer they belonged to and are counted in every mode, so they answer both cases.
A replay kept only because the session errored answers "do I want to see this error's session" - and that is a yes or a no, not a share. Keeping a random half of the error replays would just leave half the reports uninvestigable, and the cost this could guard against is already bounded by sessionReplaySampleRate and by turning the option off. The rate also hid an arithmetic trap: it applied to whatever the plain rate missed, so the real share was (100 - sessionReplaySampleRate) * rate / 100, and a rate set next to a plain rate of 100 silently did nothing. A switch has nothing to multiply. `sessionReplayOnErrorSampleRate: number` becomes `sessionReplayOnError: boolean`, default false. The tracking types and the session cookie are unchanged: what was drawn is now simply applied.
Fiona2016
changed the title
The error-session replay work was cut from main, but the fork integrates on publish, which has since gained remote configuration, forced sessions, direct replay upload and page reactivation. Resolved so that: - the on-error replay switch sits inside the draw branch, after the remote and beforeSampling-resolved replay rate has missed, so a session is still never counted by both; - forcing a session releases a withheld replay the same way it forces one onto a session without replay, since the host asked for it now; - a withheld buffer survives page reactivation: the fresh full snapshot taken on that event lands inside the buffer, which stays replayable from it, where cutting would only throw away what came before the switch; - the bridge stub never withholds, whether the host records or this page uploads directly.
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.
Motivation
A session is stored, and charged for, as a whole. That makes "keep every error, sample everything else" awkward to express by filtering events: dropping individual events still leaves the session stored, so the thing that actually counts does not go down.
sessionReplayOnErrorexpresses it where it counts. A session kept by this switch records a replay from the start but uploads nothing. If it never reports an error, nothing is ever sent and the session is never stored. On the first error the withheld recording is released, so the replay covers what led up to the error rather than starting at it.It is a switch, not a sample rate. Whether an error's session should be kept is a yes or a no: each such session is wanted for its own sake, so keeping a random share of them only leaves the rest uninvestigable, and the volume it adds is already bounded by
sessionReplaySampleRateand by switching it off. A rate would also have to apply to what the plain rate missed, which turns the real share into(100 - sessionReplaySampleRate) * rate / 100and makes a rate set next to a plain rate of 100 silently do nothing. A switch has nothing to multiply.Changes
sessionReplayOnError(boolean, defaultfalse), applying only to sessions the plainsessionReplaySampleRatedraw missed, so a session is never counted by both. It adds one tracking type and aBUFFERED_ON_ERRORreplay state.startSessionReplayRecordingManuallynow defaults tofalsewhen either the replay rate or the switch is set: a session that has to be recording before the error cannot wait to be started by hand.startSessionReplayRecordingManually.has_replaynor the replay link is offered while a replay is withheld, so nothing points at a replay that does not exist.error.source === 'agent') release nothing: an unreachable intake should not turn every session into an error session.Also included: the profiling test helpers imported from package names this repository does not publish. Because the helper is re-exported from the rum test barrel, those imports stopped roughly 220 specs from loading at all; fixing them brings the suite back.
Test instructions
yarn test:unit. New specs cover withholding, release, the checkout window, the byte limit, the stats rollback, hidden pages, and session expiry and renewal.By hand, against
yarn dev:initwithsessionSampleRate: 100,sessionReplaySampleRate: 0,sessionReplayOnError: true, then callstartSessionReplayRecording().has_full_snapshot: truewith records reaching back before the error.Driven through Playwright against the same page, a 108 second session uploaded nothing before the error and then released a segment reaching back 48 seconds, with the full snapshot present.
Checklist