diff --git a/docs/plans/2026-08-27-datadog-android-metric-recovery.md b/docs/plans/2026-08-27-datadog-android-metric-recovery.md index b7f5a5f797..f54fa75a70 100644 --- a/docs/plans/2026-08-27-datadog-android-metric-recovery.md +++ b/docs/plans/2026-08-27-datadog-android-metric-recovery.md @@ -71,6 +71,19 @@ continuous across the release. Once the pre-14.3.6 install base is gone, the leg weight and should be removed. Leaving them is not harmful, but they obscure what the metric actually measures and they will confuse the next person. +### Blocker: the two login metrics are exempt until their SLI is replaced + +`android.login.network.count` and `android.login.network.error` must **not** have their legacy +branches removed on this trigger. Measured 2026-09-07: every login-view resource event in the last +7 days came from 14.3.2 or older and carries the old name. No 14.4.x build emits one at all, because +`:authlib` has no Datadog Ktor plugin and so auth calls produce no resource events. Dropping the +legacy branch therefore takes the denominator to zero and makes monitor `93408872` fire permanently. + +The whole SLI is being rebuilt on auth resource events instead. See +`2026-09-08-android-login-sli-rebuild.md`, and +`2026-09-07-ktor-datadog-instrumentation-gap.md` for why the resource events are missing. Both login +metrics are deleted as part of that work, so this cleanup should simply skip them. + ### Trigger condition Remove them once traffic from app versions at or below 14.3.2 is negligible. Check with: diff --git a/docs/plans/2026-09-07-ktor-datadog-instrumentation-gap.md b/docs/plans/2026-09-07-ktor-datadog-instrumentation-gap.md new file mode 100644 index 0000000000..96227f7283 --- /dev/null +++ b/docs/plans/2026-09-07-ktor-datadog-instrumentation-gap.md @@ -0,0 +1,305 @@ +# Datadog network instrumentation after the move off OkHttp + +Status: gap 1 and gap 2 implemented 2026-09-07. Gap 3 partially resolved as a side effect, with a +recorded decision not to pursue the rest. Written 2026-09-07. + +## What shipped + +`:authlib` now accepts a caller-supplied engine, and `:datadog-android` supplies one carrying +`DatadogInterceptor`, `TracingInterceptor` and `DatadogEventListener`. The auth host was added to +`setFirstPartyHosts` and to the engine's traced hosts. + +Verified: + +- `:app:compileDebugKotlin` passes, so the Metro graph resolves the new `@AuthHttpClientEngine` + binding. +- `:authlib:compileKotlinIosSimulatorArm64` and `:umbrella:linkDebugFrameworkIosSimulatorArm64` pass. +- **The exported Obj-C API changes by four added lines**, diffed against a clean-tree relink of the + framework: a new `AuthEnvironment` extension exposing a read-only `baseUrl`. Nothing removed, + nothing changed. `NetworkAuthRepository` keeps exactly one initializer, + `init(environment:additionalHttpHeadersProvider:)`, so Ugglan's call site compiles untouched. The + JVM-only `networkAuthRepositoryWithEngine` does not appear in the header at all. + +The remaining sections describe why this shape was chosen and what was rejected. + +Found while diagnosing the `Auth: login (Android)` SLO breach. See +`2026-09-08-android-login-sli-rebuild.md`. + +## What OkHttp used to give us + +`7d14888e81` ("Start removing okhttp related configuration", 2025-12-17) deleted +`OkHttpDatadogUtil.kt`, which applied three separate Datadog pieces to the one shared +`OkHttpClient`: + +| Piece | What it provided | +|---|---| +| `DatadogInterceptor` | a RUM `resource` event per request, plus automatic RUM error reporting on failures | +| `TracingInterceptor` (network interceptor) | APM span creation and trace-header propagation | +| `DatadogEventListener.Factory()` | the per-request timing breakdown: DNS, connect, SSL, first byte, download, size | + +`libs.datadog.sdk.okhttp` was dropped in the same commit. No OkHttp remains in Kotlin source today; +it survives only as the Ktor engine (`ktor-client-okhttp`, and `OkHttp` as `:authlib`'s engine +factory). + +## What we have now + +Two Ktor `HttpClient` construction sites, and only one is instrumented. + +**`:network-clients`** — `NetworkMetroProviders.kt` builds the shared base client and installs +`datadogKtorPlugin` via a `mobileMain` expect/actual (`dd-sdk-kotlin-multiplatform-ktor3` 1.7.0). +Serves Apollo GraphQL, file uploads, PDF downloads, Coil image fetching, and +`GetMemberAuthorizationCodeUseCase`. + +**`:authlib`** — `KtorConfiguration.kt` builds its own client with `ContentNegotiation`, Ktor +`Logging` and `defaultRequest`, and **no Datadog plugin**. The module has no Datadog dependency at +all. Serves `POST /member-login`, the `/member-login/{id}` status polling, `/oauth/token`, and the +OTP endpoints. + +### This is not a Ktor regression + +Worth stating plainly, because it changes what there is to fix: `:authlib` arrived with its own Ktor +client in `f918732be5` ("Bring in authlib") and has never had a Datadog dependency. It builds its own +engine via `httpClientEngineFactory()`, so it never received the Datadog-configured `OkHttpClient` +either. Auth calls were never instrumented. The Ktor migration did not break this; it made it harder +to notice, because now both clients are Ktor and look interchangeable. + +The Ktor migration did cost us one thing, and it is the timing breakdown. See gap 3. + +## The three gaps, with evidence + +Measured 7 days to 2026-09-07, `env:prod`, RUM app `4d7b8355-396d-406e-b543-30a073050e8f`. + +### Gap 1: `:authlib` traffic is invisible to RUM + +Resource events on `auth.prod.hedvigit.com`, by path: + +| Path | Events | Which client | +|---|---|---| +| `/member-authorization-codes` | 634 | shared base client | +| `/member-login/{id}` | 2 | `:authlib` | +| `/member-login` | 1 | `:authlib` | +| `/oauth/token` | 1 | `:authlib` | + +The 4 `:authlib` events all come from 12.11.6, an ancient build. Every login, OTP and token-exchange +call on every current version is absent. Auth failures surface only as `@error.source:logger` text +from `logcat(LogPriority.ERROR)` calls in the presenter, with no status code, no duration, no +correlation. + +That is why today's real incident, five `504 Gateway Timeout` responses from +`auth.prod.hedvigit.com/member-login`, was only visible as prose inside an error message. + +#### Why the obvious fix does not fit + +Installing `datadogKtorPlugin` in `:authlib`, mirroring `:network-clients`, does not work without a +structural change. Verified: + +- `:authlib` is `hedvig.multiplatform.library` and declares only `jvm()` and iOS targets. There is + no `androidTarget()` and no `androidMain`. The Android app therefore consumes authlib's **JVM** + variant, and its engine comes from `jvmMain`, which is `io.ktor.client.engine.okhttp.OkHttp`. +- `:auth-core-public`, which constructs `NetworkAuthRepository`, is `hedvig.jvm.library`. Also not + Android. +- The Datadog Ktor plugin publishes android and iOS variants only. The cached 1.7.0 artifacts are + `-android`, `-iosarm64` and `-iossimulatorarm64`, with no JVM variant. `dd-sdk-android-okhttp` is + an AAR, so it cannot go into a JVM source set either. + +So neither Datadog artifact can be added to either module as they are structured. Giving `:authlib` +an Android target would change which variant the app resolves, which is exactly the kind of change +we do not want for an observability hook. + +#### The fix that is actually small + +Inject a preconfigured engine from a module that *is* Android, and leave `:authlib` Datadog-free. +OkHttp is already the engine underneath on Android, so the Datadog OkHttp instrumentation can be +attached to it directly. + +1. `:authlib` — `NetworkAuthRepository`'s primary constructor becomes `internal` and takes + `engine: HttpClientEngine?`, with a `public` secondary constructor keeping the original two + parameters and delegating with `null`. `buildKtorClient` uses the engine when given and falls back + to `httpClientEngineFactory()` otherwise. + + **A default parameter will not do here.** Kotlin/Native does not carry default values into the + Obj-C API, so `engine: HttpClientEngine? = null` on the existing constructor exports + `initWithEnvironment:additionalHttpHeadersProvider:engine:` and breaks + `AuthenticationClientAuthLib.swift` in Ugglan at the next umbrella bump. `internal` declarations + are not exported, and a separate `public` secondary constructor keeps the original selector + byte-identical. + + A JVM-only `networkAuthRepositoryWithEngine` in `jvmMain` reaches the internal constructor from + inside the module. Living in `jvmMain` keeps it, and `HttpClientEngine`, off the iOS surface + entirely. `jvmMain` exposes `api(libs.ktor.client.core)` so JVM consumers can name the type. +2. `:auth-core-public` — `provideAuthRepository` takes a qualified `HttpClientEngine` and calls + `networkAuthRepositoryWithEngine`. It needs `implementation(libs.ktor.client.core)` added. The + `@AuthHttpClientEngine` qualifier goes in `:core-common-public` next to the existing + `@BaseHttpClient`. +3. `:datadog-android` — provide that engine, restoring what `7d14888e81` deleted: + +```kotlin +OkHttp.create { + config { + eventListenerFactory(DatadogEventListener.Factory()) + addInterceptor(DatadogInterceptor.Builder(tracedHosts).build()) + addNetworkInterceptor(TracingInterceptor.Builder(tracedHosts).build()) + } +} +``` + + `:datadog-android` is where `OkHttpDatadogUtil.kt` lived before deletion, so the Datadog + knowledge stays in the Datadog module. Re-add `dd-sdk-android-okhttp` to its dependencies and to + the version catalog. + +No dependency cycle: `:auth-core-public` needs only the `HttpClientEngine` type and the qualifier, +not `:datadog-android`. Metro merges the binding in `:app`. + +This is strictly better than the plugin route, for about the same effort: + +- **Gap 1 is fixed.** Auth calls become RUM resources with status codes and durations. +- **Gap 3 is fixed for auth.** `DatadogInterceptor` and `DatadogEventListener` sit at the same + OkHttp layer and share its key space, which is precisely what the Ktor plugin cannot do, so the + DNS, connect, SSL, first byte and download phases land. +- **Gap 2 is fixed for auth** in the same change, by putting the auth host in `tracedHosts` here and + in `setFirstPartyHosts` in `DatadogInitializer`, same module. +- **`:network-clients` is not touched at all.** It keeps the Ktor plugin, so the ~329k resource + events a week that work today carry no risk, and there is no double counting anywhere, because + authlib has no Ktor plugin to conflict with. + +iOS is deliberately out of scope: Ugglan ships its own tracking, so authlib on iOS keeps passing +null and behaves exactly as it does now. + +Effort: small and additive. Requires a release. + +### Gap 2: only the GraphQL host is treated as first-party + +Two independent allowlists both name only `urlGraphqlOctopus`: + +- `DatadogInitializer.kt`: `.setFirstPartyHosts(listOf(urlGraphqlOctopus.removePrefix("https://")))` +- `NetworkMetroProviders.mobile.kt`: `tracedHosts = mapOf(urlGraphqlOctopus... to DATADOG)` + +Consequence, observable today: an `apollo-router.prod.hedvigit.com` resource carries +`provider.type: "first party"`, while `/member-authorization-codes` on `auth.prod.hedvigit.com` +carries `provider.type: "unknown"` even though it goes through the instrumented client. No trace +headers are sent to the auth host, so client spans do not join the auth service's existing traces. + +**Fix:** add the auth host to both lists. This is client-side configuration only and adds no code to +any backend, so it stays inside the decided scope. It makes the auth service's existing APM traces +correlatable from the app side, which is free. + +Effort: two lines. Requires a release. + +### Gap 3: resource timing phases are gone, permanently + +A current resource event from 14.4.6 against the instrumented client: + +``` +duration 212556539 +status_code 200 +dns null +connect null +ssl null +first_byte null +download null +size null +``` + +`duration` and `status_code` survive. Every phase field is null, on both clients, for all traffic. +This was `DatadogEventListener.Factory()`'s contribution, and it worked by hooking OkHttp's +low-level `EventListener` socket callbacks. Ktor exposes no equivalent hook, and the Datadog Ktor +plugin does not provide one. + +Two corrections to the obvious first guesses, both checked rather than assumed. + +**The SDK does not have this, in any version.** Read the sources of the pinned +`dd-sdk-kotlin-multiplatform-ktor3` 1.7.0 out of the Gradle cache: `grep -ri timing` across the +whole plugin returns nothing. The latest published version is 1.8.0 (2026-08-10) and its changelog, +and every changelog before it, mentions trace propagation, RUM session id, resource attribute +providers and error handling, but never timings, phases, or an event listener. Bumping the plugin +does not get this. + +**Adding `DatadogEventListener` alongside the plugin cannot work.** This is the important one, +because it is the natural thing to try. From +`com/datadog/kmp/ktor/internal/plugin/DatadogKtorPlugin.kt`, `onSend`: + +```kotlin +val requestId = uuid4().toString() +request.attributes.put(DD_REQUEST_ID_ATTR, requestId) +rumMonitorProvider.invoke().startResource(key = requestId, ...) +``` + +The plugin keys every RUM resource with a freshly generated UUID that lives only in Ktor's attribute +bag. `DatadogEventListener` runs down at the OkHttp layer and has no access to that bag, so whatever +key it reports timings against, it can never be the plugin's UUID. The timings would be attached to a +resource RUM has never heard of and silently dropped. The two mechanisms have disjoint key spaces. + +### The path that does work on Android + +Not supplement the plugin, **replace** it, on Android only. + +Ktor's `OkHttpConfig` exposes `config(block: OkHttpClient.Builder.() -> Unit)` plus `addInterceptor` +/ `addNetworkInterceptor` and a `preconfigured: OkHttpClient?`. Its own default config only sets +`followRedirects`, `followSslRedirects` and `retryOnConnectionFailure`, so it does not install an +event listener and will not fight one. That means the exact trio from the deleted +`OkHttpDatadogUtil.kt` can be reattached to the engine, and `DatadogInterceptor` and +`DatadogEventListener` then share OkHttp's own key space, which is what makes the timings land. + +Android would go back to OkHttp-level instrumentation; iOS keeps the Ktor plugin on the Darwin +engine. The `mobileMain` / `iosMain` expect/actual seam for `installDatadogKtorPlugin` already exists +precisely for this kind of split, and `NetworkMetroProviders.jvm.kt` already shows the no-op shape. + +Mechanically, the cleanest seam is the engine factory rather than the plugin installer. +`installDatadogKtorPlugin` is typed `HttpClientConfig<*>`, and `HttpClient(httpClientEngineFactory())` +star-projects the engine config, so `engine { }` cannot reach `OkHttpConfig` from `commonMain` +without a cast. Instead, have the Android actual of `httpClientEngineFactory()` return a factory +that bakes the interceptors and the event listener in, and make `installDatadogKtorPlugin` a no-op +there. This needs `hedvigBuildConstants` threaded into the factory, which it does not take today. + +Costs and risks, all of which need a spike before committing: + +- Reintroduces the `dd-sdk-android-okhttp` dependency, dropped in `7d14888e81`. +- **Double counting is the main hazard.** Exactly one of the two mechanisms may be active per + platform. Both means two RUM resource events per request and a doubled denominator on anything + counting resources. +- Network error messages change shape. The plugin emits `"Ktor request error $method $url"`, which + is observable in RUM today; `DatadogInterceptor` does not. Any Datadog filter matching that string + breaks. +- Verify websockets and SSE still work through the engine, and that Ktor-level retries + (`HttpSend` interceptors, the auth retry) do not produce surprising resource counts. +- Android and iOS would report resources through different code paths, so subtle attribute + differences between platforms become possible. + +### Recommendation + +Split the two clients, because the answer differs. + +**For `:authlib`, timings come for free.** The gap 1 fix above already puts `DatadogInterceptor` and +`DatadogEventListener` on the auth engine together, so auth traffic gets full phase breakdown as a +side effect. Nothing extra to decide. + +**For `:network-clients`, leave it alone.** Recovering timings on the Apollo traffic means replacing +a working Ktor plugin with engine-level instrumentation on the client that carries ~329k events a +week, and that is where every risk in the list above lives: double counting, changed error message +shape, websockets and SSE to re-verify. `duration` plus `status_code` covers ordinary triage, and +missing DNS-versus-TLS attribution on GraphQL has not actually blocked anything. + +Revisit only if a real latency question needs phase attribution on GraphQL specifically. The path is +viable and now written down, so that decision can be made on evidence rather than rediscovered. + +Also worth noting while here: `size` is not an unconditional loss. The plugin does report it, as +`size = response.contentLength()` with a `TODO RUM-6382` for the missing-header case, so the nulls +we see mean our responses lack `Content-Length` (chunked or compressed), not that the field is +unsupported. + +And a small free win found in the same source: `traceHeaderTypesForHost` does suffix matching and +honours a `"*"` key, so gap 2 can be fixed with a wildcard instead of enumerating hosts, if that is +preferred. + +## Order of work + +Gap 1 and gap 2 are one small PR and one release. They are worth doing on their own merits: auth +currently has no network observability whatsoever, which is the reason the login SLO investigation +needed a full afternoon. + +They also turned out to be the whole fix for the login SLI. Because auth calls now produce resource +events, that SLI can be rebuilt as a filter change with no further app code, which is what +`2026-09-08-android-login-sli-rebuild.md` records. The earlier plan to rebuild it as a RUM operation +existed only because these calls were invisible. + +Gap 3 needs a decision recorded, not work. diff --git a/docs/plans/2026-09-08-android-login-sli-rebuild.md b/docs/plans/2026-09-08-android-login-sli-rebuild.md new file mode 100644 index 0000000000..ca01cd7168 --- /dev/null +++ b/docs/plans/2026-09-08-android-login-sli-rebuild.md @@ -0,0 +1,91 @@ +# Rebuilding the Android login SLI on auth resource events + +Status: definition agreed 2026-09-08. Blocked on the release that carries the auth instrumentation. + +This supersedes an earlier plan to rebuild the SLI as a RUM operation. That plan existed only because +`:authlib` emitted no resource events at all, which made any resource-based SLI impossible. Adding +the Datadog hooks to the auth engine removed that constraint, so the fix is now a filter change with +no app code behind it. + +## What was wrong + +SLO `Auth: login (Android)` (id `29588e73473d54f09814173755548b80`, monitor `93408872`) fired at +534.351% of its 7-day error budget on 2026-09-07 with no login outage behind it. + +Measured over the 7 days to 2026-09-07, `env:prod`: + +| Side | Metric | Events | Event type | Where from | +|---|---|---|---|---| +| denominator | `android.login.network.count` | 131 | `resource` | versions 12.11.6 to 14.3.2, Sweden | +| numerator | `android.login.network.error` | 7 | `error` | version 14.4.7, one device in US datacenters | + +7/131 = 5.34351%, against a 1% budget, is 534.351%. Four separate defects: + +1. The two sides counted different event types, so the numerator could never be a subset of the + denominator. +2. 96 of the 131 denominator events were `apollo-router` calls that merely happened while the login + screen was on top. Only 4 were auth calls. +3. Both sides were keyed on `@view.name`, a navigation implementation detail, which changed underneath + the SLI during the Nav2 to Nav3 migration. +4. Neither filter carried `@session.type:user`. + +## The new SLI + +Both sides are `@type:resource` on the auth host, so the bad-event set is a subset of the total set by +construction, and `@view.name` disappears entirely. + +Total events: + +``` +@type:resource @application.id:4d7b8355-396d-406e-b543-30a073050e8f @resource.url_host:auth.prod.hedvigit.com @resource.url_path:"/member-login" @session.type:user +``` + +Bad events: the same query plus `@resource.status_code:[500 TO 599]`. + +Both metrics use `event_type: resource` and group by `env`, matching the existing `android.*` metrics, +so the SLO keeps selecting `{env:prod}` and the filter carries no environment of its own. + +Proposed names: `android.login.attempt.count` and `android.login.attempt.failure`. New names rather +than reusing the old pair, so the old metrics stay intact for comparison across the transition. + +**The quoted exact path is load-bearing.** Verified 2026-09-08 against a real login on staging: the +quoted form matched the single `POST /member-login` and excluded the two `/member-login/{id}` status +polls. Unquoted, the polls would inflate the denominator by however many times BankID happened to be +polled, which varies per attempt. + +## Target and window + +Use a **30-day** window, not 7. + +Login-screen impressions are the closest available proxy for attempt volume: 1,547 over the 30 days +to 2026-09-08 in `env:prod`. At 99% over 30 days that is an error budget of roughly 15 events. The +current 7-day window gives a budget of one to three, which is why a single device on a single +afternoon breached it. Set the target from the first two weeks of real data rather than assuming 99% +carries over. + +## Known properties, recorded so nobody rediscovers them + +- A request that never reaches the server (no connectivity, refused TLS handshake) is reported by + `DatadogInterceptor` as `@type:error @error.source:network` and produces no resource, so it lands in + neither side. This is what the old filters were reaching for with their `-@error.stack:java.net.*` + exclusion lists, except it now falls out of the data model rather than a list of strings that has to + be kept current. +- The SLI measures whether login requests that reached the auth service came back non-5xx. It does not + measure whether the member actually got in. BankID abandonment, a failed status poll and a failed + token exchange all sit outside it. Answering that needs an operation spanning the whole attempt, + which is a product question rather than an availability one, and is deliberately out of scope. +- 4xx counts as good. A malformed personal number is not an availability failure. + +## Remaining steps + +1. Create the two RUM generated metrics on the filters above. Worth doing before the release so they + begin computing the moment the instrumentation ships, rather than whenever someone remembers. +2. Once they carry real traffic, update SLO `29588e73473d54f09814173755548b80` in place: swap its two + queries and move the timeframe to 30 days. Updating rather than replacing keeps the id, monitor + `93408872` and its `@slack-android-dev` routing, and anything else referencing the id. +3. Keep `android.login.network.count` and `android.login.network.error` computing for one full window + after the swap so the two definitions can be compared, then delete them. +4. Drop the blocker section from `2026-08-27-datadog-android-metric-recovery.md` once those two + metrics are gone. + +Until step 2 lands the monitor keeps firing on the broken definition. It is deliberately not muted.