feat: [SDK-5156] add named events to the log pipeline - #24
Open
nan-li wants to merge 15 commits into
Open
Conversation
Adds SdkEvent, the closed set of named events, and LogEventRecorder, which ships them as INFO log records carrying event.name through whichever remote sink the host attaches. Events use the crash gate (log_level present and not NONE) rather than the severity filter, so each one ships default off behind its own catalog flag, read through the host's feature manager. The recorder owns the flag check, a bounded pre-sink queue for records made before HYDRATE installs the sink, and a session cap of 20 events per process. First event: sdk.device_gesture, gated by sdk_event_device_gesture_enabled (IMMEDIATE, so turning it off does not wait for a cold start).
…emetry Cuts the KDoc on SdkEvent, ISdkEventRecorder, LogEventRecorder and the factory to the constraints a first-time reader cannot infer from the code, keeps the explanatory test comments, and renames the recorder's internals, log lines, docs and test names to say remote telemetry rather than sink, matching what the rest of the module calls it.
Review follow-ups on the named-event recorder: - detach(telemetry) clears only the attached instance. iOS shares one recorder across remote loggers, and a logger that lost the install race is shut down without starting; its detach must not strand the winner. - reset() drops the queue for an app-id change; the cap stays per process. - ISdkEventGate replaces the function type and record gets two explicit overloads, so the Obj-C edge has no defaults or boxed booleans. - Faults (throwing gate, attach, emit) log at WARN; expected drops stay at DEBUG. The host logger is guarded so it cannot throw out of record(). - Attach and detach log a DEBUG line so a stray detach is reconstructible. - The cap is named per process; "session" means a foreground session in this SDK. Tests cover the ignored detach, reset, the overloads, a throwing host logger, and the cap under eight concurrent recorders.
…tity A Swift telemetry is wrapped anew each time it crosses into Kotlin, so the identity check in detach never matched the instance the host attached and a Swift-side detach was silently ignored. Equality bridges to isEqual: and is identity for Kotlin objects, so both hosts get the intended behaviour.
"Event" already means several things across the SDKs (custom events the app tracks, notification and in-app events), and inside this module a single log record is also called an event. These are the SDK reporting on itself through the observability pipeline, which is what the Android host already calls the pipeline, so the type takes that name. SdkEvent becomes ObservabilityEvent, ISdkEventRecorder becomes IObservabilityEventRecorder, ISdkEventGate becomes IObservabilityEventGate, LogEventRecorder becomes ObservabilityEventRecorder, and the factory method is createObservabilityEventRecorder. The flag key, the event.name attribute and its sdk.device_gesture value are unchanged.
…ll as off The KDoc only mentioned turning the event off. Turning it on is the rollout and turning it off is the kill switch; neither waits for a cold start. The flag test now covers both directions.
The catalog's polarity is "key present means on", and the other entries are bare nouns, so sdk_event_device_gesture_enabled becomes sdk_event_device_gesture. The flag does not exist in ConfigCat yet.
Not every event should need a catalog flag. A crash ships with no flag and only the pipeline gate, and an event wanted from the first launch cannot depend on a flags fetch that has not happened yet. Each ObservabilityEvent now carries an internal gate the recorder evaluates itself: RequiresFlag (off until the flag is on, the default for a measurement switched on per app), UnlessFlag (on until a kill-switch flag is present, a regular sdk_event_<name>_disabled catalog entry), or Always (only the pipeline gate, like a crash). The host interface shrinks to IFeatureFlagReader, "is this flag on", which replaces IObservabilityEventGate; a per-event cap or sample rate can be added to the policy later without touching it. The gesture stays on RequiresFlag with the same catalog entry. The catalog naming test now checks sdk_event_<name> for a required flag and sdk_event_<name>_disabled for a kill switch.
…remote telemetry The factory read as a list of what gets composed, with one entry that also made a threading decision the others keep beside the class that owns the coroutines. The recorder now defaults its scope the way LogTelemetryRemoteImpl does; tests still inject one.
The sealed gate hierarchy said two things, which flag and what it means, in three classes; those are now two fields on the event, with the evaluation on an internal FlagPolarity enum so both polarities stay testable before any event uses them. The recorder's Admission type and admit() existed only to launch outside the lock, but launch only schedules, so the decision and the launch now share one when block and only the debug line waits for the lock. No public surface changes.
"Polarity" names the mechanism; the enum describes what the flag does to the event, so it is FlagEffect with ENABLES and DISABLES.
nan-li
marked this pull request as ready for review
September 4, 2026 04:07
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review (Opus 5, GPT 5.6 Sol, Grok 4.6)
12 Kotlin files, +911 / −0. Happy path looks solid (flag gate, bounded queue, equality detach, swallow-on-fault). No criticals. Nothing calls record yet.
Act on
reset() after a full pre-attach queue permanently mutes the process (Opus + Grok). admitted++ happens when queuing, both defaults are 20, and reset() drops the queue without refunding. The documented app-id-change path can discard 20 never-shipped events and then hit the cap forever. resetDoesNotRefundTheCap uses processCap = 1, so it cannot tell “don’t refund shipped events” from “don’t refund discarded ones.” Refund the dropped queue size, or count the cap only for records that still exist.
Consider
- In-flight
scope.launchvs detach/shutdown (3/3).detachonly nulls the reference; already-admitted emits can land on a shut-down sink and vanish with no WARN. - Identity stamped at emit, not record (Opus + Grok; GPT’s reset-generation race is the same family). Timestamp is record-time;
ossdk.app_id/app.state/ IDs are merged inLogTelemetryRemoteImpl.emit, so a queued or in-flight event can export under the next app.
Noted
Producer attributes win the perEventFields + record.attributes merge (Opus). Equality detach is load-bearing for Swift wrappers and untested as such (Opus). Cross-record emit order holds only on the test scheduler (Opus + GPT). Cap is per recorder instance (Opus).
Dismissed
Gesture id PII: no production caller yet (3/3). “Cap stays across reset” after a shipped event is intended; the bug is charging discarded queued events.
Sent by Cursor Automation: PR Reviews
reset() emptied the pre-attach queue but kept the dropped events counted against the cap, so a reset after a full queue left the process unable to ship anything. It now behaves as a new process would: queue emptied, count restarted. clearStatics on iOS resets the shared recorder between unit tests, so the tests need this even though a mid-process app-id change is out of scope (SDK-5142).
The gesture ships gesture.result and, when something was copied, gesture.push_subscription_id. The earlier gesture.id_kind and gesture.id pair is gone, so the catalog entry and the recorder test's example attributes follow.
sdk_device_gesture_disabled turns the clipboard gesture off when present, so the gesture works before the first flags fetch and a customer can be opted out per app. With a catalog entry both hosts can ask the feature manager instead of scanning the raw flag list. IMMEDIATE so opting out lands on the next fetch, not the next cold start.
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.
Summary
Adds observability events to the shared log pipeline. An event is an OTLP log record with an
event.nameattribute, sent through the same remote telemetry as log lines and crash records. Unlike log lines it is not filtered by the remote log level; like crashes it ships whenever the pipeline is on.New in
com.onesignal.logger, all intended API for the two host SDKs:ObservabilityEvent, the closed list of events the SDK reports about itself. Each entry names the catalog flag that gates it and what that flag does (FlagEffect.ENABLES, the default: off until the flag is on;FlagEffect.DISABLES: a kill switch for an event wanted from the first launch; no flag: always on, only the pipeline gate applies, like a crash). The first entry issdk.device_gesture, off untilsdk_event_device_gestureis on (newFeatureFlag.SDK_EVENT_DEVICE_GESTURE, IMMEDIATE so turning it on or off does not need a cold start).IObservabilityEventRecorderwithrecord,attach,detachandreset. The host attaches whatever remote telemetry it installs; until then the recorder queues up to 20 records. It ships at most 20 events per process, anddetachonly clears the telemetry it is handed, so a host that shares one recorder across telemetry instances cannot detach the live one by mistake.resetdrops the queue on an app-id change.IFeatureFlagReaderincom.onesignal.features, the one question the recorder asks a host: is this flag on.LoggerFactory.createObservabilityEventRecorder(flags, logger).FeatureFlag.SDK_DEVICE_GESTURE_DISABLEDin the catalog, the inverted kill switch for the clipboard gesture itself (present means off, IMMEDIATE), so both hosts read it through their feature manager instead of scanning the raw flag list. Added for SDK-5088, which stacks on this branch.Faults (a throwing flag read, attach or telemetry) log at WARN; expected drops (flag off, cap reached, queue full) log at DEBUG. Nothing calls
recordyet; the gesture detector call site lands with SDK-5088.Expected volume: the gesture takes six background/foreground cycles inside 30 seconds, the recorder caps at 20 per process, and nothing ships until ConfigCat, which Turbine serves the SDK's flags from, turns
sdk_event_device_gestureon. That key and the gesture'ssdk_device_gesture_disabledare both defined there now.Motivation
Remote logging cannot carry a structured fact, and INFO lines never ship at the production default of ERROR. The clipboard gesture makes no request, so an event is the only way to learn whether anyone uses it.
Ticket SDK-5156. Design doc: "Named events on the log pipeline".
Testing
:kmp:testDebugUnitTest(199 tests) and:kmp:iosSimulatorArm64Test(201 tests) pass,spotlessCheckis clean.New tests cover the record shape and that a caller cannot overwrite
event.name, the flag read on every record, both flag effects and the no-flag case, the pre-attach queue and its bound, the per-process cap including under eight concurrent recorders, detach ignoring telemetry that is not attached, reset, fail-open behaviour when the flag read, the telemetry or the host logger throws, and the record on the wire throughLogTelemetryRemoteImpl.ObservabilityEventTestpins the naming rules:sdk.<name>for events,sdk_event_<name>for a flag that enables one,sdk_event_<name>_disabledfor a kill switch.FeatureFlagTestpins the gesture kill switch's key and activation mode.Host PRs: OneSignal-Android-SDK#2732 and OneSignal-iOS-SDK#1732, both pinned to this branch. Both should be re-pointed to the release tag after this merges.
Checklist