feat(rum): let the console set the session sample rate - #16
Open
Fiona2016 wants to merge 9 commits into
Open
Conversation
…types Groundwork for taking the sampling rates from the console. The core's HTTP client only surfaced the status code, which is all an upload needs; asking the backend for something needs what came back, and must go through the same client so it honours the proxy the customer configured. Not wired to anything yet.
The application owner can change how much traffic RUM keeps without the customer shipping a new release. Off by default: without `remoteConfigurationEnabled` the SDK makes no extra request and behaves exactly as before. The core fetches the configuration when RUM publishes its source — at SDK init and at every session creation — and stores it for the next draw. A change therefore only affects sessions created after it arrives, so a visitor is never dropped halfway through. A failed fetch keeps the stored values rather than falling back, so a bad minute at the endpoint cannot swing a fleet back to its built-in rates. Each session records the rate it was actually drawn under and the configuration version it came from, and its view events report both, so server-side extrapolation lines up with the draw that kept the session rather than with whatever has arrived since. The console's custom values ride along untouched and are handed to the host application through `remoteConfig()`; the platform delivers them, their meaning belongs to the application. Only the session sample rate is delivered. Session Replay and distributed tracing are not configured from here on this platform. Evaluating the context closure on the caller's thread, which an earlier draft of this change did to inspect the published value, races with the scope mutations that happen on the context queue — ThreadSanitizer catches it in RUMApplicationScope. The closure is evaluated on the context queue as before and the value inspected there.
The same call is `getRemoteConfig()` on web and Android. A host writing the same integration twice should not have to remember that one platform spells it differently, and `get` prefixes already appear in this SDK's public API.
`RemoteSamplingRates.isEmpty` had no caller outside two test assertions, and a public symbol nobody needs is not worth publishing. `readDictionary` took a `required` flag both call sites passed as false, so the throwing branch could never run. The rest is residue from narrowing this feature to the session sample rate: two intermediate values that now read one field each, two misaligned continuation lines, a constant that no longer needs to be visible outside its file, and an explicit nil the surrounding properties do without.
… add the missing controls Three changes to remote sampling, all of them about the iOS SDK doing what the other platforms already do. The first session of every launch ignored the stored configuration. The rates reach RUM through the core context, which is written on its own queue, so a value published there is not visible to a draw that happens now - and the first session is drawn immediately after RUM.enable(). Worse, the controller that loads the snapshot was only built when RUM published its source, which happens inside the session scope's initialiser, after the draw. So every cold start drew its first session at the values the app was built with, and the console's setting only took hold from the second session on. A customer dialling a rate down to shed volume still got every first-session at the old rate. The stored snapshot is now read synchronously, through a small `RemoteSamplingReader` protocol, before RUM can draw anything: the feature primes it at init, and the draw reads the rates without a queue hop. The context stays the channel for every other feature. `setForcedSession()` collects a visitor regardless of the configured rates, for an allow-list or a support flow. A session that was not being collected ends and a collected one starts in its place; a session already being collected keeps running, because RUM cannot retro-collect what a running session already dropped. The forced state lasts for the process lifetime and rides to Session Replay on the core context, so a forced session comes out with replay rather than being dropped by replay's own draw. `beforeSampling` is consulted synchronously at every draw, with the rate that would apply and the console's custom values; return a rate to override it or nil to leave it alone. It runs after the console's rate so an allow-list can keep a visitor that rate would drop. A rate outside 0...100 is ignored - a mistake in the host application must never take a customer's collection down with it. Also here: - A configuration written to a `schema_version` this SDK does not read is refused whole rather than read field by field, and is not retried: the server answered, and asking again would fetch the same refusal. - The no-op default for getRemoteConfig() lived in an internal extension, so it satisfied nothing outside this module. NOPMonitor now implements both methods explicitly and warns, like every other method on it.
Three fixes from a review pass over the remote configuration channel. A body carrying an older version than the one in effect was applied unconditionally. Versions only ever climb — a rollback republishes the older content under a new number, and pruning removes the oldest rows — so a lower one can only be a stale copy from an edge cache or a proxy answering 200 with something it held on to. Applying it put the client back on settings the console had already replaced, and it then reported that older number, so the rollout view read as the change losing ground. The custom values an application reads through getRemoteConfig() reached the monitor only on the next context broadcast, which arrives on another queue. An app calling it right after enabling RUM — to decide whether to force a session, which is what the API exists for, at exactly the moment it happens — was told nothing had been published while the very same stored configuration was already deciding how its first session was drawn. They are now seeded from the same synchronous read that primes the rates. One test asked for the configuration again as soon as the first request had gone out, but the answer to that request is what releases the in-flight guard, so the second ask raced it and was sometimes dropped. Both tests that did this now wait for the exchange to finish rather than for the request to leave. Two comments claiming Session Replay reads these rates are corrected: it samples with the rate the application configured and follows RUM's decision about the session.
…t just views Three findings from a review that read one behaviour across all the SDKs at once rather than one SDK at a time. Only the view event reported the rate the session was actually drawn with. Errors, actions, resources and long tasks reported the value passed to init, so a session drawn at the console's 10% told the backend, and anyone reading the explorer, that it had been drawn at 100%. Every event now reports the rate that decided the session, which is what the view event already did. The record of that draw was kept only when the console had published something, so an application that uses beforeSampling on its own — the hook applies whether or not remote configuration is enabled — drew at the hook's rate and then reported the init rate on every event. It is now kept whenever the draw moved away from init, whatever moved it. A response carrying no schema stamp at all was refused as a shape this build cannot read. A body without one is, by construction, the shape that existed before the stamp did, which is the shape this reader was written against; refusing it switches remote configuration silently off against a server that merely predates the field, with nothing to say so. Only a stamp that is present and unrecognised is a refusal now — which is what the web SDK already did, so the two no longer disagree about the same response.
The expression that resolves it — the rate the session was drawn with, falling back to the value the app was initialised with — was written out at six event sites. Six copies of a rule that must stay identical is how one of them ends up different; it is now stated once, on the context that carries the draw.
An absent key and a key holding an explicit null say the same thing — nothing was stamped — but only the first was read that way, so the same response was accepted by the Android and HarmonyOS SDKs and refused here. A field whose whole purpose is that every reader agrees about a response cannot be the one place they disagree.
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.
Lets an application owner change how much traffic RUM keeps without the customer
shipping a new release.
Off by default: without
RUM.Configuration.remoteConfigurationEnabledthe SDKmakes no extra request and behaves exactly as before.
How it works
and at every session creation. There is no polling: a change can only matter at
the next draw, so asking more often than sessions are drawn would be requests
for nothing.
answers, so the first sessions after a cold start already draw with the values
the console last provided rather than the ones the app was built with.
they were, and is retried twice (5s then 60s, ±20% jitter) before waiting for
the next natural trigger. Wiping the values on a bad minute would swing a whole
fleet back to its built-in rates, which is the opposite of what someone who
turned a knob deliberately wants.
with
Cache-Control: private, no-cache, and a 304 changes nothing.version it came from; its view events report both, so server-side extrapolation
lines up with the draw that kept the session rather than with whatever has
arrived since.
application through
getRemoteConfig(). The platform delivers them; theirmeaning belongs to the application.
Only the session sample rate is delivered. Session Replay and distributed tracing
are not configured from here on this platform.
Note on the first commit
wip(core)adds the body-returningfetchonHTTPClientand the shared typesthe rest of the change builds on. It is kept as its own commit rather than
rewritten, since the branch is already published.
One defect worth calling out
An earlier draft evaluated the context closure on the caller's thread in order to
inspect the published value. That races with the scope mutations that happen on
the context queue — ThreadSanitizer reports it in
RUMApplicationScope, and thetest process aborts. The closure is evaluated on the context queue as before and
the value is inspected there.
Verification
DatadogCore iOS760/760,DatadogRUM iOS666/666, both under ThreadSanitizermake lintclean