Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions posthog/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
FlagDefinitionCacheProvider as FlagDefinitionCacheProvider,
)
from posthog.request import (
DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES as DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES,
disable_connection_reuse as disable_connection_reuse,
enable_keep_alive as enable_keep_alive,
set_socket_options as set_socket_options,
Expand Down Expand Up @@ -322,6 +323,9 @@ def get_tags() -> Dict[str, Any]:
attributed to the person normally.
feature_flags_request_timeout_seconds: Timeout in seconds for feature flag
and remote config requests.
feature_flags_request_max_retries: Number of retries for feature flag
requests after network, transport, or timeout failures. Defaults to 1.
Set to 0 to disable retries.
super_properties: Properties merged into every captured event.
enable_exception_autocapture: Automatically capture uncaught exceptions.
log_captured_exceptions: Also log exceptions captured by error tracking.
Expand Down Expand Up @@ -365,6 +369,7 @@ def get_tags() -> Dict[str, Any]:
disable_geoip = True # type: bool
is_server = True # type: bool
feature_flags_request_timeout_seconds = 3 # type: int
feature_flags_request_max_retries = DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES # type: int
super_properties = None # type: Optional[Dict]
enable_exception_autocapture = False # type: bool
log_captured_exceptions = False # type: bool
Expand Down Expand Up @@ -1156,6 +1161,7 @@ def setup() -> Client:
disable_geoip=disable_geoip,
is_server=is_server,
feature_flags_request_timeout_seconds=feature_flags_request_timeout_seconds,
feature_flags_request_max_retries=feature_flags_request_max_retries,
super_properties=super_properties,
# TODO: Currently this monitoring begins only when the Client is initialised (which happens when you do something with the SDK)
# This kind of initialisation is very annoying for exception capture. We need to figure out a way around this,
Expand Down
16 changes: 16 additions & 0 deletions posthog/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
AI_EVENTS_ENDPOINT,
EVENTS_ENDPOINT,
APIError,
DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES,
QuotaLimitError,
RequestsConnectionError,
RequestsTimeout,
Expand Down Expand Up @@ -240,6 +241,7 @@ def __init__(
is_server=True,
historical_migration=False,
feature_flags_request_timeout_seconds=3,
feature_flags_request_max_retries=DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES,
super_properties=None,
enable_exception_autocapture=False,
log_captured_exceptions=False,
Expand Down Expand Up @@ -296,6 +298,9 @@ def __init__(
historical_migration: Mark events as historical migration imports.
feature_flags_request_timeout_seconds: Timeout in seconds for feature
flag and remote config requests.
feature_flags_request_max_retries: Number of retries for feature flag
requests after network, transport, or timeout failures. Defaults
to 1. Set to 0 to disable retries.
super_properties: Properties merged into every captured event.
enable_exception_autocapture: Automatically capture uncaught
exceptions.
Expand Down Expand Up @@ -376,6 +381,9 @@ def __init__(
self.feature_flags_request_timeout_seconds = (
feature_flags_request_timeout_seconds
)
self.feature_flags_request_max_retries = max(
0, feature_flags_request_max_retries
)
self.poller: Optional[Poller] = None
self.distinct_ids_feature_flags_reported = SizeLimitedDict(MAX_DICT_SIZE, set)
self.flag_fallback_cache_url = flag_fallback_cache_url
Expand Down Expand Up @@ -895,10 +903,18 @@ def _get_flags_decision(
if flag_keys_to_evaluate:
request_data["flag_keys_to_evaluate"] = flag_keys_to_evaluate

flag_request_options: Dict[str, Any] = {}
if (
self.feature_flags_request_max_retries
!= DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES
):
flag_request_options["max_retries"] = self.feature_flags_request_max_retries

resp_data = flags(
self.api_key,
self.host,
timeout=self.feature_flags_request_timeout_seconds,
**flag_request_options,
**request_data,
)
Comment on lines +906 to 919

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The flag_request_options dict and its conditional guard are superfluous. flags() already defaults to DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES, so passing the stored value unconditionally produces identical behaviour for every configuration — but is simpler and more obviously correct. The extra dict and branch also make the default code path invisible to tests: patch_flags.call_args.kwargs["max_retries"] would raise KeyError when the default is in use, forcing the test to cover only the non-default path.

Suggested change
flag_request_options: Dict[str, Any] = {}
if (
self.feature_flags_request_max_retries
!= DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES
):
flag_request_options["max_retries"] = self.feature_flags_request_max_retries
resp_data = flags(
self.api_key,
self.host,
timeout=self.feature_flags_request_timeout_seconds,
**flag_request_options,
**request_data,
)
resp_data = flags(
self.api_key,
self.host,
timeout=self.feature_flags_request_timeout_seconds,
max_retries=self.feature_flags_request_max_retries,
**request_data,
)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


Expand Down
65 changes: 36 additions & 29 deletions posthog/request.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import logging
import re
import socket
import time
from dataclasses import dataclass
from datetime import date, datetime, timezone
from gzip import GzipFile
Expand Down Expand Up @@ -41,8 +42,8 @@
if hasattr(socket, attr):
KEEP_ALIVE_SOCKET_OPTIONS.append((socket.SOL_TCP, getattr(socket, attr), value))

# Status codes that indicate transient server errors worth retrying
RETRY_STATUS_FORCELIST = [408, 500, 502, 503, 504]
DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES = 1
FEATURE_FLAGS_RETRY_BACKOFF_SECONDS = 0.3


def _mask_tokens_in_url(url: str) -> str:
Expand Down Expand Up @@ -90,23 +91,14 @@ def _build_session(socket_options: Optional[SocketOptions] = None) -> requests.S
def _build_flags_session(
socket_options: Optional[SocketOptions] = None,
) -> requests.Session:
"""
Build a session for feature flag requests with POST retries.
"""Build a session for feature flag requests.

Feature flag requests are idempotent (read-only), so retrying POST
requests is safe. This session retries on transient server errors
(408, 5xx) and network failures with exponential backoff
(0.5s, 1s delays between retries).
/flags retries are handled explicitly in ``flags()`` so that only
transport failures are retried. HTTP status responses must surface as API
errors without retrying.
"""
adapter = HTTPAdapterWithSocketOptions(
max_retries=Retry(
total=2,
connect=2,
read=2,
backoff_factor=0.5,
status_forcelist=RETRY_STATUS_FORCELIST,
allowed_methods=["POST"],
),
max_retries=Retry(total=0, connect=0, read=0, status=0),
socket_options=socket_options,
)
session = requests.Session()
Expand Down Expand Up @@ -306,26 +298,41 @@ def _process_response(
raise APIError(res.status_code, res.text, retry_after=retry_after)


def _feature_flags_retry_delay(failed_attempt: int) -> float:
return FEATURE_FLAGS_RETRY_BACKOFF_SECONDS * (2**failed_attempt)


def flags(
api_key: str,
host: Optional[str] = None,
gzip: bool = False,
timeout: int = 15,
max_retries: int = DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES,
**kwargs,
) -> Any:
"""Post the kwargs to the flags API endpoint with automatic retries."""
res = post(
api_key,
host,
"/flags/?v=2",
gzip,
timeout,
session=_get_flags_session(),
**kwargs,
)
return _process_response(
res, success_message="Feature flags evaluated successfully"
)
"""Post the kwargs to the flags API endpoint with transport retries."""
retries = max(0, max_retries)
failed_attempt = 0

while True:
try:
res = post(
api_key,
host,
"/flags/?v=2",
gzip,
timeout,
session=_get_flags_session(),
**kwargs,
)
return _process_response(
res, success_message="Feature flags evaluated successfully"
)
except (requests.exceptions.ConnectionError, requests.exceptions.Timeout):
if failed_attempt >= retries:
raise
time.sleep(_feature_flags_retry_delay(failed_attempt))
failed_attempt += 1


def remote_config(
Expand Down
15 changes: 15 additions & 0 deletions posthog/test/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,21 @@ def test_basic_capture_with_feature_flags_returns_active_only(self, patch_flags)
device_id=None,
)

@mock.patch("posthog.client.flags")
def test_feature_flags_request_max_retries_is_forwarded_when_configured(
self, patch_flags
):
patch_flags.return_value = {"featureFlags": {}, "featureFlagPayloads": {}}
client = Client(
FAKE_TEST_API_KEY,
feature_flags_request_max_retries=0,
personal_api_key=FAKE_TEST_API_KEY,
)

client.get_all_flags("distinct_id")

self.assertEqual(patch_flags.call_args.kwargs["max_retries"], 0)
Comment on lines +987 to +1000

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Non-parameterised test for the max_retries forwarding. The custom instructions prefer parameterised tests, and there are at least three distinct cases worth covering: the default value (1), zero (disable), and a non-default positive value (e.g. 3). A single case also doesn't verify that the default is forwarded at all, since the current conditional in client.py omits max_retries from the call when its value equals the default.

Context Used: Do not attempt to comment on incorrect alphabetica... (source)


@mock.patch("posthog.client.flags")
def test_basic_capture_with_feature_flags_and_disable_geoip_returns_correctly(
self, patch_flags
Expand Down
Loading
Loading