-
Notifications
You must be signed in to change notification settings - Fork 324
feat(react-native): record automatic exception steps #4573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| '@posthog/core': minor | ||
| 'posthog-react-native': minor | ||
| --- | ||
|
|
||
| Add automatic exception steps to React Native, so a captured exception carries a timeline even where nobody called `addExceptionStep`. Set `errorTracking.exceptionSteps.automatic` to `true`, or to an object, to record a step for a screen change (`navigation`), an autocaptured tap (`taps`), or an app lifecycle transition (`lifecycle`). Every signal stays off by default, because each step adds bytes to every captured exception. | ||
|
|
||
| Automatic steps carry `$type`, which the error tracking timeline already renders, and they share the byte-bounded buffer and the native forwarding that manual steps use. A manual step stays untyped. An event that `before_send` dropped leaves no step. | ||
|
|
||
| `reset()` now clears the exception-step buffer, for automatic and manual steps alike. Exception steps are user-session state, so on a shared device the previous user's screen names and tap labels must not reach the next user's exception. The buffer still survives a capture, so every exception in one session carries the same steps. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,34 @@ | ||
| import { isArray, isNumber, isObject, isString, safeJsonStringify } from '@/utils' | ||
|
|
||
| export const EXCEPTION_STEP_INTERNAL_FIELDS = { | ||
| TYPE: '$type', | ||
| MESSAGE: '$message', | ||
| LEVEL: '$level', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we use level anywhere ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, nothing writes it. I added Core now adds only |
||
| TIMESTAMP: '$timestamp', | ||
| } as const | ||
|
|
||
| /** | ||
| * Only `$message` and `$timestamp` are reserved: the SDK owns their canonical values, so a caller | ||
| * cannot spoof them. `$type` and `$level` stay writable by callers who want to categorise their own | ||
| * steps, and the SDK sets `$type` itself only on automatic steps. | ||
| */ | ||
| const RESERVED_EXCEPTION_STEP_KEYS = new Set<string>([ | ||
| EXCEPTION_STEP_INTERNAL_FIELDS.MESSAGE, | ||
| EXCEPTION_STEP_INTERNAL_FIELDS.TIMESTAMP, | ||
| ]) | ||
|
|
||
| /** | ||
| * `$type` values the SDK sets on automatic steps. Every SDK uses these exact strings, so the error | ||
| * tracking timeline labels an automatic step the same way on web and on mobile. | ||
| */ | ||
| export const EXCEPTION_STEP_TYPES = { | ||
| NAVIGATION: 'navigation', | ||
| TAP: 'tap', | ||
| LIFECYCLE: 'lifecycle', | ||
| } as const | ||
|
|
||
| export type ExceptionStepType = (typeof EXCEPTION_STEP_TYPES)[keyof typeof EXCEPTION_STEP_TYPES] | ||
|
|
||
| export type ExceptionStep = { | ||
| [EXCEPTION_STEP_INTERNAL_FIELDS.MESSAGE]: string | ||
| [EXCEPTION_STEP_INTERNAL_FIELDS.TIMESTAMP]: string | number | ||
|
|
@@ -32,6 +51,62 @@ export const DEFAULT_EXCEPTION_STEPS_CONFIG: ResolvedExceptionStepsConfig = { | |
| max_bytes: 32768, // ~32KB | ||
| } | ||
|
|
||
| /** | ||
| * Which app signals the SDK turns into automatic steps. Every signal is opt-in, because a step adds | ||
| * bytes to each captured exception and the buffer already carries whatever the caller added by hand. | ||
| * | ||
| * NOTE: The SDK reads these signals from its own platform. A platform that cannot observe a signal | ||
| * ignores the flag rather than failing. | ||
| */ | ||
| export type AutomaticExceptionStepsConfig = { | ||
| /** Screen or route changes. @default false */ | ||
| navigation?: boolean | ||
| /** Taps and clicks the SDK already autocaptures. @default false */ | ||
| taps?: boolean | ||
| /** App lifecycle transitions such as open, foreground and background. @default false */ | ||
| lifecycle?: boolean | ||
| } | ||
|
|
||
| export type ResolvedAutomaticExceptionStepsConfig = { | ||
| navigation: boolean | ||
| taps: boolean | ||
| lifecycle: boolean | ||
| } | ||
|
|
||
| export const DEFAULT_AUTOMATIC_EXCEPTION_STEPS_CONFIG: ResolvedAutomaticExceptionStepsConfig = { | ||
| navigation: false, | ||
| taps: false, | ||
| lifecycle: false, | ||
| } | ||
|
|
||
| const ALL_AUTOMATIC_EXCEPTION_STEPS_CONFIG: ResolvedAutomaticExceptionStepsConfig = { | ||
| navigation: true, | ||
| taps: true, | ||
| lifecycle: true, | ||
| } | ||
|
|
||
| /** | ||
| * Resolves the automatic-steps config. `true` enables every signal, `false` and `undefined` disable | ||
| * every signal, and an object enables only the signals it sets. | ||
| */ | ||
| export function resolveAutomaticExceptionStepsConfig( | ||
| config?: boolean | AutomaticExceptionStepsConfig | null | ||
| ): ResolvedAutomaticExceptionStepsConfig { | ||
| if (config === true) { | ||
| return { ...ALL_AUTOMATIC_EXCEPTION_STEPS_CONFIG } | ||
| } | ||
|
|
||
| if (!config || !isObject(config)) { | ||
| return { ...DEFAULT_AUTOMATIC_EXCEPTION_STEPS_CONFIG } | ||
| } | ||
|
|
||
| return { | ||
| navigation: config.navigation ?? DEFAULT_AUTOMATIC_EXCEPTION_STEPS_CONFIG.navigation, | ||
| taps: config.taps ?? DEFAULT_AUTOMATIC_EXCEPTION_STEPS_CONFIG.taps, | ||
| lifecycle: config.lifecycle ?? DEFAULT_AUTOMATIC_EXCEPTION_STEPS_CONFIG.lifecycle, | ||
| } | ||
| } | ||
|
|
||
| export function resolveExceptionStepsConfig(config?: ExceptionStepsConfig | null): ResolvedExceptionStepsConfig { | ||
| if (!config) { | ||
| return { ...DEFAULT_EXCEPTION_STEPS_CONFIG } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1727,6 +1727,28 @@ | |
| ], | ||
| "path": "src/types.ts" | ||
| }, | ||
| { | ||
| "id": "AutomaticExceptionStepsConfig", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we specialize the react native config for this as I don't think it applies to node ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, done in 4042b7d. This file is back to what main has. Core now adds one field name and nothing else. |
||
| "name": "AutomaticExceptionStepsConfig", | ||
| "properties": [ | ||
| { | ||
| "description": "Screen or route changes.", | ||
| "type": "boolean", | ||
| "name": "navigation" | ||
| }, | ||
| { | ||
| "description": "Taps and clicks the SDK already autocaptures.", | ||
| "type": "boolean", | ||
| "name": "taps" | ||
| }, | ||
| { | ||
| "description": "App lifecycle transitions such as open, foreground and background.", | ||
| "type": "boolean", | ||
| "name": "lifecycle" | ||
| } | ||
| ], | ||
| "path": "../core/src/error-tracking/exception-steps.ts" | ||
| }, | ||
| { | ||
| "id": "BaseException", | ||
| "name": "BaseException", | ||
|
|
@@ -2175,6 +2197,13 @@ | |
| ], | ||
| "path": "../core/src/error-tracking/exception-steps.ts" | ||
| }, | ||
| { | ||
| "id": "ExceptionStepType", | ||
| "name": "ExceptionStepType", | ||
| "properties": [], | ||
| "path": "../core/src/error-tracking/exception-steps.ts", | ||
| "example": "(typeof EXCEPTION_STEP_TYPES)[keyof typeof EXCEPTION_STEP_TYPES]" | ||
| }, | ||
| { | ||
| "id": "ExpressErrorMiddleware", | ||
| "name": "ExpressErrorMiddleware", | ||
|
|
@@ -3851,6 +3880,25 @@ | |
| "path": "../core/src/error-tracking/coercers/promise-rejection-event.ts", | ||
| "example": "PromiseRejectionEventLike | EventWithDetailReason" | ||
| }, | ||
| { | ||
| "id": "ResolvedAutomaticExceptionStepsConfig", | ||
| "name": "ResolvedAutomaticExceptionStepsConfig", | ||
| "properties": [ | ||
| { | ||
| "type": "boolean", | ||
| "name": "navigation" | ||
| }, | ||
| { | ||
| "type": "boolean", | ||
| "name": "taps" | ||
| }, | ||
| { | ||
| "type": "boolean", | ||
| "name": "lifecycle" | ||
| } | ||
| ], | ||
| "path": "../core/src/error-tracking/exception-steps.ts" | ||
| }, | ||
| { | ||
| "id": "ResolvedExceptionStepsConfig", | ||
| "name": "ResolvedExceptionStepsConfig", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leaves RN the odd one out from our SDKs? iirc, web and other mobile SDKs don't clear exception steps on reset.
This was previously discussed here.
For exception tracking I think steps should be scoped to app session and not user session. The whole idea isn't understanding user behavior, it's understanding app behavior, and user changing is part of a normal app flow. Losing the steps around a login or logout flow is losing exactly the context you'd want when something breaks there?
cc @hpouillot wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, and I reverted it in 4042b7d. RN keeps steps across
reset()again, so it matches web, iOS, Android and Flutter. I added a test that locks the behavior in, plus a comment onreset()so the next person does not clear it by accident.Your app-behavior argument is the one that convinced me, and my privacy argument turned out weaker than I first thought. Automatic steps carry no
$el_textand no touch coordinates, only structural labels likeScreen: CartandTap: CheckoutButton, so what crossed the boundary was app shape rather than user content.For context on why I touched it at all: the review bot flagged the shared-device case, and automatic steps do change the volume, because the buffer now fills on every screen change and tap instead of only where an app calls
addExceptionStep. That is a difference in degree, not a reason to break consistency with four SDKs.Two notes from the thread you linked, for whoever makes the call.
The identity-change question reads as open rather than settled. @hpouillot said "I think I would scope it to the session not the device", then "so ideally we reset the buffer when user changes", then "but I don't mind releasing a first version without". You said "I don't have a strong opinion tbh" and made the app-level argument, and he answered "Yes ok make sense". So the current behavior looks like the v1 choice, and hugues leaned the other way at the time.
There is also a middle option in that thread, and it is hugues's: "we could also create an automatic exception step 'user changed'?" You deferred it partly because you did not want to "mix up completely manual + semi-automatic api at the sdk level just yet". This PR adds exactly that semi-automatic API, so that objection has lapsed. An identity step at
reset()keeps the login and logout context you want, and it marks the boundary so nobody reads user A's steps as user B's.I am happy to add that step here, or to leave it out and keep this PR to automatic steps only. Your call, and hugues's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that a "user changed" step would be valuable here as well, esp now that we're adding automatic exception steps