fix(data-imports): keep exhausted Notion 5xx retries out of error tracking - #73485
Conversation
…cking Notion's `_request` already retries a 5xx internally with backoff (tenacity) before re-raising `NotionRetryableError`. When those retries exhaust, the error was falling through to `_handle_import_error`'s default branch, which logs it as a tracked exception even though Temporal will retry the whole activity and the failure is self-recovering. Add `get_retryable_errors()` to `NotionSource`, matching the stable "Notion API error (retryable)" prefix, so this classifies the same way it already does for Mixpanel/Intercom/MongoDB sources. Generated-By: PostHog Code Task-Id: 180d4aa8-841d-4c96-a131-4099938361fc
|
Hey @Gilbert09! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
🤖 CI report✅ Bundle size — no changeUncompressed size of every built Total: 64.39 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 281.5 KiB | ../node_modules/.pnpm/posthog-js@1.407.2/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 236.0 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 226.1 KiB | ../node_modules/.pnpm/posthog-js@1.407.2/node_modules/posthog-js/dist/module.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.8 KiB | src/lib/api.ts |
| 94.0 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.18 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.18 MiB · 17 files | no change | ████░░░░░░ 38.1% of 5.72 MiB |
| Deferred (lazy) | 2.07 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 713.8 KiB | dist/toolbar/toolbar-app-HLTHZTTF.css |
| 545.0 KiB | dist/toolbar/chunk-chunk-6OSYGHA4.js |
| 484.2 KiB | dist/toolbar/chunk-chunk-FFAAFRZ5.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-LYC3RDNL.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-THUTYU3T.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-JD7XW4XC.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-3VBOQT7S.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — no change
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1352.29 MiB · no change
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Small, well-tested error-classification fix that follows an established, widely-used pattern (get_retryable_errors) already present in five other sources; low risk, outside risky territory, with a regression test proving the marker matches the actual raised message.
- 👍 on the PR from hex-security-app[bot].
- Diff includes an incidental generated-file change in products/experiments/frontend/generated/api.schemas.ts (additive read-only fields is_system/was_impersonated/client) not mentioned in the description — appears to be generated-file drift, not authored behavior, and is purely additive/non-behavioral so not a blocker.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 6L, 1F substantive, 26L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (26L, 3F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 8f47666 · reviewed head 5d915b1 |
1 participant
Problem
Error tracking caught a
NotionRetryableError(Notion API error (retryable): status=522, url=https://api.notion.com/v1/comments) in the data-warehouse import pipeline (issue).The stack trace confirms it originates in
notion.py's_request(line 188), reached via the comments fan-out stream (_comments_stream→get_rows→ the shared import pipeline)._requestalready retries a 5xx internally with exponential backoff via tenacity (5 attempts) before giving up and re-raisingNotionRetryableError. When those internal retries are exhausted, the exception falls through to_handle_import_error's default branch, which logs it withlogger.aexception(...)— capturing it as a tracked exception — before re-raising so Temporal retries the whole activity.A 522 (origin timed out, per Cloudflare's status code in front of Notion's API) is a transient infrastructure condition, not something PostHog's code or the customer's config can fix. Since Temporal already retries the activity and the failure is self-recovering, this doesn't need to reach error tracking as noise.
Decision
Classified as a fixable improvement, not a user/upstream error: the source already has a mechanism for exactly this (
get_retryable_errors()onResumableSource, used to keep self-recovering, internally-retried failures out of error tracking), butNotionSourcedidn't implement it. Mixpanel, Intercom, and MongoDB sources already use this same mechanism for their own transient/self-recovering errors, so this follows established precedent rather than introducing a new pattern.Changes
get_retryable_errors()toNotionSource, matching the stable"Notion API error (retryable)"prefix that_requestraises on any 5xx.How did you test this code?
Added
test_retryable_marker_matches_raised_messagetotest_notion_source.py, mirroring the equivalent Mixpanel test — asserts the marker returned byget_retryable_errors()matches the exact message_requestraises on a 5xx. This is the regression: without the fix, an exhausted-retry 5xx would keep being logged as a tracked exception.Ran:
uv run pytest products/warehouse_sources/backend/temporal/data_imports/sources/notion/tests/test_notion_source.py— 22 passedruff check --fix/ruff formaton both changed files — cleanuv run mypy --cache-fine-grained .(repo-wide) — clean, no issues in 16783 source filesAutomatic notifications
Docs update
N/A — internal error-classification change, no user-facing behavior change.
🤖 Agent context
Autonomy: Fully autonomous
Triaged a webhook-delivered error tracking issue using the PostHog MCP error-tracking tools (
query-error-tracking-issue,query-error-tracking-issue-events) to pull the full stack trace and confirm the failure genuinely originates in the Notion source's_request. Readnotion.pyandsource.pyto understand the retry/classification flow, and found theget_retryable_errors()precedent already used by the Mixpanel, Intercom, and MongoDB sources for the same class of problem (self-recovering errors already retried internally). Invoked/writing-testsbefore adding the regression test. Checked for duplicate open PRs by exception type, message phrase, and module path, and reviewed the maintainer's own open PR queue — found one Notion-related draft (#71645) but it addresses 401/403 auth-expiry classification, an unrelated code path.