fix(metrics): harden attribute snapshots, retry backoff, and config validation#742
Open
DanielVisca wants to merge 2 commits into
Open
fix(metrics): harden attribute snapshots, retry backoff, and config validation#742DanielVisca wants to merge 2 commits into
DanielVisca wants to merge 2 commits into
Conversation
…alidation Follow-ups from the #739 review: - Deep-snapshot metric attributes at capture so mutating a nested list/dict afterwards cannot rewrite an already-keyed series' wire attributes. - Retry failed flushes with exponential backoff (2x per consecutive failure, capped at 64x the flush interval, matching the shared JS logs policy) and drop the window after exactly 8 consecutive failures - the previous policy retried at the base cadence and dropped on the fourth failure while documenting three. - Validate the nested metrics config and degrade to defaults with a warning, so client.metrics.count() keeps the client's no-throw contract even with a hostile config dict. Generated-By: PostHog Code Task-Id: f9941012-0ef9-4c6c-9813-34d84f8e7d02
Contributor
posthog-python Compliance ReportDate: 2026-07-20 15:34:23 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
DanielVisca
marked this pull request as ready for review
July 20, 2026 15:26
DanielVisca
enabled auto-merge (squash)
July 20, 2026 15:26
DanielVisca
disabled auto-merge
July 20, 2026 15:27
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
posthog/metrics_capture.py:558
**Armed Timer Bypasses Backoff**
After a capture arms the normal timer, an explicit `flush()` failure reaches this call while `_flush_timer` is still set. `_arm_flush_timer(delay)` then returns without rescheduling, so the existing timer can retry at the base deadline instead of the calculated backoff, hammering the endpoint and exhausting the retry budget early.
### Issue 2 of 2
posthog/metrics_capture.py:173-178
**Fallback Shares Nested Attributes**
If one attribute value raises during `deepcopy`, the whole mapping falls back to a shallow copy. A normal mutable value in that same mapping, such as a tags list, then remains shared with the caller; later mutation changes the wire attributes after the series key was created, producing incorrectly labeled metric data.
Reviews (1): Last reviewed commit: "Merge branch 'main' into posthog-code/me..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) | ||
| self._merge_window_back(window) | ||
| self._arm_flush_timer() | ||
| self._arm_flush_timer(delay) |
Contributor
There was a problem hiding this comment.
After a capture arms the normal timer, an explicit flush() failure reaches this call while _flush_timer is still set. _arm_flush_timer(delay) then returns without rescheduling, so the existing timer can retry at the base deadline instead of the calculated backoff, hammering the endpoint and exhausting the retry budget early.
Rule Used: When implementing retry logic, include comprehensi... (source)
Learned From
PostHog/posthog#32651
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/metrics_capture.py
Line: 558
Comment:
**Armed Timer Bypasses Backoff**
After a capture arms the normal timer, an explicit `flush()` failure reaches this call while `_flush_timer` is still set. `_arm_flush_timer(delay)` then returns without rescheduling, so the existing timer can retry at the base deadline instead of the calculated backoff, hammering the endpoint and exhausting the retry budget early.
**Rule Used:** When implementing retry logic, include comprehensi... ([source](https://app.greptile.com/posthog-org-19734/-/custom-context?memory=24d1be3f-07fb-465d-b1a7-60dff301aed8))
**Learned From**
[PostHog/posthog#32651](https://github.com/PostHog/posthog/pull/32651)
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+173
to
+178
| self.attributes: Optional[dict] = copy.deepcopy(attributes) | ||
| except Exception: | ||
| # Un-copyable exotic values: a shallow snapshot still isolates the | ||
| # top-level dict, and the encoder stringifies whatever remains. | ||
| self.attributes = dict(attributes) | ||
| else: |
Contributor
There was a problem hiding this comment.
If one attribute value raises during deepcopy, the whole mapping falls back to a shallow copy. A normal mutable value in that same mapping, such as a tags list, then remains shared with the caller; later mutation changes the wire attributes after the series key was created, producing incorrectly labeled metric data.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/metrics_capture.py
Line: 173-178
Comment:
**Fallback Shares Nested Attributes**
If one attribute value raises during `deepcopy`, the whole mapping falls back to a shallow copy. A normal mutable value in that same mapping, such as a tags list, then remains shared with the caller; later mutation changes the wire attributes after the series key was created, producing incorrectly labeled metric data.
How can I resolve this? If you propose a fix, please make it concise.
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.
💡 Motivation and Context
Follow-ups from the #739 review — approved to unblock, but three considerations went unaddressed before merge:
Nested attribute values weren't snapshotted —
_SeriesStateshallow-copied the attributes dict, so mutating a nested list/dict aftercount()rewrote an already-keyed series' attributes on the wire. Attributes are now deep-copied at capture (with a shallow fallback for un-copyable exotic values, keeping the no-throw guarantee).Retry policy was fixed-cadence with an off-by-one drop — the window was retried at the base flush interval and dropped on the fourth failure while the release note said three. Failed flushes now back off exponentially (2× per consecutive failure, capped at 64× the flush interval — the same ceiling the shared JS logs implementation uses) and the window is dropped after exactly
_MAX_CONSECUTIVE_SEND_FAILURES = 8consecutive failures (~21 min of outage coverage at the default 10s interval). The series cap still bounds buffered memory throughout. Browser/Node parity for this policy is a follow-up on the JS side.Lazy metrics init escaped the client's no-throw contract —
Client("phc_...", metrics={"resource_attributes": "bad"})made the firstclient.metrics.count()raiseAttributeErrorinto application code. Nested config is now validated field-by-field (non-dict config/resource_attributes, non-numericflush_interval, non-integermax_series_per_flush, non-callablebefore_send) and degrades to defaults with a warning; theclient.metricsproperty additionally falls back to a default-config instance if construction fails (raising only in debug mode, matchingno_throw).💚 How did you test it?
TDD — new tests written first and confirmed failing against the merged code, then made green:
test_nested_attribute_values_snapshot_at_capture— mutates a list attribute after capture, asserts the two series keep distinct wire attributes.test_failed_flushes_back_off_exponentially_capped— pins the [2, 4, 8, 16, 32, 64, 64]× delay schedule and the reset-on-success.test_window_survives_failures_until_the_drop_limit/ updatedtest_window_dropped_after_consecutive_failures— pin the drop-at-exactly-8 boundary from both sides.test_hostile_metrics_config_does_not_raise_and_still_records— parameterized over five hostile configs, asserts the full public call chain records with defaults.Full local validation:
pytest posthog/test(1207 passed; the pre-existingtest_integration_stop_reasonfailure reproduces on a clean checkout),ruff check/format --check,mypy | mypy-baseline filter(clean),python -W error -c "import posthog",make public_api_check(no public surface change).📝 Checklist
If releasing new changes
sampo addto generate a changeset file (hand-written changeset, patch bump)🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Created with PostHog Code