diff --git a/reddit_decider/__init__.py b/reddit_decider/__init__.py index a783d88..0e18099 100644 --- a/reddit_decider/__init__.py +++ b/reddit_decider/__init__.py @@ -1,17 +1,18 @@ import logging -from typing import Any, Callable, Dict, Optional +from typing import Any, Callable, Dict, IO, Optional +from baseplate import RequestContext from baseplate import Span from baseplate.clients import ContextFactory -from baseplate.frameworks.pyramid import BaseplateRequest from baseplate.lib import config from baseplate.lib.events import DebugLogger from baseplate.lib.events import EventLogger from baseplate.lib.file_watcher import FileWatcher +from baseplate.lib.file_watcher import T from baseplate.lib.file_watcher import WatchedFileNotAvailableError -import rust_decider +import rust_decider # type: ignore logger = logging.getLogger(__name__) @@ -69,7 +70,7 @@ def to_dict(self) -> Dict: } -def init_decider_parser(file): +def init_decider_parser(file: IO) -> Any: return rust_decider.init("darkmode overrides targeting fractional_availability value", file.name) def validate_decider(decider: Optional[Any]) -> None: @@ -108,7 +109,7 @@ def __init__( else: self._event_logger = DebugLogger() - def _get_decider(self): + def _get_decider(self) -> Optional[T]: try: decider = self._config_watcher.get_data() validate_decider(decider) @@ -142,6 +143,9 @@ def get_variant( :return: Variant name if a variant is assigned, None otherwise. """ decider = self._get_decider() + if not decider: + logger.warning("Encountered error in _get_decider()") + return None ctx = rust_decider.make_ctx(self._decider_context.to_dict()) ctx_err = ctx.err() @@ -223,7 +227,7 @@ def __init__( event_logger: Optional[EventLogger] = None, timeout: Optional[float] = None, backoff: Optional[float] = None, - request_field_extractor: Callable[[BaseplateRequest], Dict[str, str]] = None + request_field_extractor: Optional[Callable[[RequestContext], Dict[str, str]]] = None ): self._filewatcher = FileWatcher(path=path, parser=init_decider_parser, timeout=timeout, backoff=backoff) self._event_logger = event_logger @@ -283,69 +287,3 @@ def make_object_for_context(self, name: str, span: Span) -> Decider: context_name=name, event_logger=self._event_logger, ) - - -def decider_client_from_config( - app_config: config.RawConfig, - event_logger: EventLogger, - prefix: str = "experiments.", - request_field_extractor: Callable[[BaseplateRequest], Dict[str, str]] = None -) -> DeciderContextFactory: - """Configure and return an :py:class:`DeciderContextFactory` object. - - The keys useful to :py:func:`decider_client_from_config` should be prefixed, e.g. - ``experiments.path``, etc. - - Supported keys: - - ``path`` (optional) - The path to the experiment configuration file generated by the - experiment configuration fetcher daemon. - ``timeout`` (optional) - The time that we should wait for the file specified by ``path`` to - exist. Defaults to `None` which is `infinite`. - ``backoff`` (optional) - The base amount of time for exponential backoff when trying to find the - experiments config file. Defaults to no backoff between tries. - ``request_field_extractor`` (optional) used to populate - "app_name" & "build_number" fields in DeciderContext() - - :param raw_config: The application configuration which should have - settings for the experiments client. - :param event_logger: The EventLogger to be used to log bucketing events. - :param prefix: the prefix used to filter keys (defaults to "experiments."). - - """ - assert prefix.endswith(".") - config_prefix = prefix[:-1] - - cfg = config.parse_config( - app_config, - { - config_prefix: { - "path": config.Optional(config.String, default="/var/local/experiments.json"), - "timeout": config.Optional(config.Timespan), - "backoff": config.Optional(config.Timespan), - } - }, - ) - options = getattr(cfg, config_prefix) - - # pylint: disable=maybe-no-member - if options.timeout: - timeout = options.timeout.total_seconds() - else: - timeout = None - - if options.backoff: - backoff = options.backoff.total_seconds() - else: - backoff = None - - return DeciderContextFactory( - path=options.path, - event_logger=event_logger, - timeout=timeout, - backoff=backoff, - request_field_extractor=request_field_extractor - ) diff --git a/reddit_experiments/__init__.py b/reddit_experiments/__init__.py index a5f962c..1748991 100644 --- a/reddit_experiments/__init__.py +++ b/reddit_experiments/__init__.py @@ -3,12 +3,14 @@ import warnings from enum import Enum +from typing import Callable from typing import Dict from typing import Optional from typing import overload from typing import Sequence from typing import Set +from baseplate import RequestContext from baseplate import Span from baseplate.clients import ContextFactory from baseplate.lib import config @@ -19,6 +21,7 @@ from baseplate.lib.file_watcher import WatchedFileNotAvailableError from reddit_edgecontext import User +from reddit_decider import DeciderContextFactory from reddit_experiments.providers import parse_experiment from reddit_experiments.providers.base import Experiment @@ -396,3 +399,69 @@ def experiments_client_from_config( backoff = None return ExperimentsContextFactory(options.path, event_logger, timeout=timeout, backoff=backoff) + + +def decider_client_from_config( + app_config: config.RawConfig, + event_logger: EventLogger, + prefix: str = "experiments.", + request_field_extractor: Optional[Callable[[RequestContext], Dict[str, str]]] = None, +) -> DeciderContextFactory: + """Configure and return an :py:class:`DeciderContextFactory` object. + + The keys useful to :py:func:`decider_client_from_config` should be prefixed, e.g. + ``experiments.path``, etc. + + Supported keys: + + ``path`` (optional) + The path to the experiment configuration file generated by the + experiment configuration fetcher daemon. + ``timeout`` (optional) + The time that we should wait for the file specified by ``path`` to + exist. Defaults to `None` which is `infinite`. + ``backoff`` (optional) + The base amount of time for exponential backoff when trying to find the + experiments config file. Defaults to no backoff between tries. + ``request_field_extractor`` (optional) used to populate + "app_name" & "build_number" fields in DeciderContext() + + :param raw_config: The application configuration which should have + settings for the experiments client. + :param event_logger: The EventLogger to be used to log bucketing events. + :param prefix: the prefix used to filter keys (defaults to "experiments."). + + """ + assert prefix.endswith(".") + config_prefix = prefix[:-1] + + cfg = config.parse_config( + app_config, + { + config_prefix: { + "path": config.Optional(config.String, default="/var/local/experiments.json"), + "timeout": config.Optional(config.Timespan), + "backoff": config.Optional(config.Timespan), + } + }, + ) + options = getattr(cfg, config_prefix) + + # pylint: disable=maybe-no-member + if options.timeout: + timeout = options.timeout.total_seconds() + else: + timeout = None + + if options.backoff: + backoff = options.backoff.total_seconds() + else: + backoff = None + + return DeciderContextFactory( + path=options.path, + event_logger=event_logger, + timeout=timeout, + backoff=backoff, + request_field_extractor=request_field_extractor, + ) diff --git a/tests/decider_tests.py b/tests/decider_tests.py index 75fc2b5..8abd7d9 100644 --- a/tests/decider_tests.py +++ b/tests/decider_tests.py @@ -12,49 +12,10 @@ from reddit_edgecontext import User from reddit_decider import Decider -from reddit_decider import decider_client_from_config from reddit_decider import DeciderContext from reddit_decider import DeciderContextFactory from reddit_decider import init_decider_parser - - -@mock.patch("reddit_decider.FileWatcher") -class DeciderClientFromConfigTests(unittest.TestCase): - def setUp(self): - super().setUp() - self.event_logger = mock.Mock(spec=DebugLogger) - self.mock_span = mock.MagicMock(spec=ServerSpan) - self.mock_span.context = None - - def test_make_clients(self, file_watcher_mock): - decider_ctx_factory = decider_client_from_config( - {"experiments.path": "/tmp/test"}, self.event_logger - ) - self.assertIsInstance(decider_ctx_factory, DeciderContextFactory) - file_watcher_mock.assert_called_once_with( - path="/tmp/test", parser=init_decider_parser, timeout=None, backoff=None - ) - - def test_timeout(self, file_watcher_mock): - decider_ctx_factory = decider_client_from_config( - {"experiments.path": "/tmp/test", "experiments.timeout": "60 seconds"}, - self.event_logger, - ) - self.assertIsInstance(decider_ctx_factory, DeciderContextFactory) - file_watcher_mock.assert_called_once_with( - path="/tmp/test", parser=init_decider_parser, timeout=60.0, backoff=None - ) - - def test_prefix(self, file_watcher_mock): - decider_ctx_factory = decider_client_from_config( - {"r2_experiments.path": "/tmp/test", "r2_experiments.timeout": "60 seconds"}, - self.event_logger, - prefix="r2_experiments.", - ) - self.assertIsInstance(decider_ctx_factory, DeciderContextFactory) - file_watcher_mock.assert_called_once_with( - path="/tmp/test", parser=init_decider_parser, timeout=60.0, backoff=None - ) +from reddit_experiments import decider_client_from_config @mock.patch("reddit_decider.FileWatcher") diff --git a/tests/experiment_tests.py b/tests/experiment_tests.py index cf884a3..bd8b139 100644 --- a/tests/experiment_tests.py +++ b/tests/experiment_tests.py @@ -12,6 +12,9 @@ from reddit_edgecontext import AuthenticationToken from reddit_edgecontext import User +from reddit_decider import DeciderContextFactory +from reddit_decider import init_decider_parser +from reddit_experiments import decider_client_from_config from reddit_experiments import EventType from reddit_experiments import Experiments from reddit_experiments import experiments_client_from_config @@ -894,3 +897,42 @@ def test_experiments_with_different_name_updated_cache(self): self.assertTrue("test2" in self.experiments_factory._global_cache) self.assertFalse("test1" in self.experiments_factory._global_cache) self.assertEqual(self.experiments_factory._global_cache["test2"].owner, "test2") + + +@mock.patch("reddit_decider.FileWatcher") +class DeciderClientFromConfigTests(unittest.TestCase): + def setUp(self): + super().setUp() + self.event_logger = mock.Mock(spec=DebugLogger) + self.mock_span = mock.MagicMock(spec=ServerSpan) + self.mock_span.context = None + + def test_make_clients(self, file_watcher_mock): + decider_ctx_factory = decider_client_from_config( + {"experiments.path": "/tmp/test"}, self.event_logger + ) + self.assertIsInstance(decider_ctx_factory, DeciderContextFactory) + file_watcher_mock.assert_called_once_with( + path="/tmp/test", parser=init_decider_parser, timeout=None, backoff=None + ) + + def test_timeout(self, file_watcher_mock): + decider_ctx_factory = decider_client_from_config( + {"experiments.path": "/tmp/test", "experiments.timeout": "60 seconds"}, + self.event_logger, + ) + self.assertIsInstance(decider_ctx_factory, DeciderContextFactory) + file_watcher_mock.assert_called_once_with( + path="/tmp/test", parser=init_decider_parser, timeout=60.0, backoff=None + ) + + def test_prefix(self, file_watcher_mock): + decider_ctx_factory = decider_client_from_config( + {"r2_experiments.path": "/tmp/test", "r2_experiments.timeout": "60 seconds"}, + self.event_logger, + prefix="r2_experiments.", + ) + self.assertIsInstance(decider_ctx_factory, DeciderContextFactory) + file_watcher_mock.assert_called_once_with( + path="/tmp/test", parser=init_decider_parser, timeout=60.0, backoff=None + )