From 85bcfd596791519a58e5b238c966b3d66e8cb837 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Thu, 25 Jul 2024 14:06:17 +0200 Subject: [PATCH 1/2] chore(features) Expand documentation for sentry.features Expand the documentation for features.handler and features.manager. --- src/sentry/features/handler.py | 24 +++++++++++++++++++----- src/sentry/features/manager.py | 21 ++++++++++----------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/src/sentry/features/handler.py b/src/sentry/features/handler.py index 182b8104a92e1d..6299c123d69fa0 100644 --- a/src/sentry/features/handler.py +++ b/src/sentry/features/handler.py @@ -17,6 +17,13 @@ class FeatureHandler: + """ + Base class for defining custom logic for feature decisions. + + Subclasses should implement `has` and contain the logic + necessary for the feature check. + """ + features: MutableSet[str] = set() def __call__(self, feature: Feature, actor: User) -> bool | None: @@ -50,13 +57,20 @@ def batch_has( raise NotImplementedError -# It is generally better to extend BatchFeatureHandler if it is possible to do -# the check with no more than the feature name, organization, and actor. If it -# needs to unpack the Feature object and examine the flagged entity, extend -# FeatureHandler directly. +class BatchFeatureHandler(FeatureHandler): + """ + Base class for feature handlers that apply to an organization + and an optional collection of `objects` (e.g. projects). + Subclasses are expected to implement `_check_for_batch` and perform a feature check + using only the organization. + + It is generally better to extend BatchFeatureHandler if it is possible to do + the check with no more than the feature name, organization, and actor. If it + needs to unpack the Feature object and examine the flagged entity, extend + FeatureHandler directly. + """ -class BatchFeatureHandler(FeatureHandler): @abc.abstractmethod def _check_for_batch( self, feature_name: str, entity: Organization | User, actor: User diff --git a/src/sentry/features/manager.py b/src/sentry/features/manager.py index 568fbf5905e6ca..9bea37be4ffe11 100644 --- a/src/sentry/features/manager.py +++ b/src/sentry/features/manager.py @@ -77,23 +77,22 @@ def has_for_batch( actor: User | None = None, ) -> Mapping[Project, bool]: """ - Determine in a batch if a feature is enabled. + Determine if a feature is enabled for a batch of objects. - This applies the same procedure as ``FeatureManager.has``, but with a - performance benefit where the objects being checked all belong to the - same organization. The objects are entities (e.g., projects) with the - common parent organization, as would be passed individually to ``has``. + This method enables checking a feature for an organization and a collection + of objects (e.g. projects). Feature handlers for batch checks are expected to + subclass `features.BatchFeatureHandler` and implement `has_for_batch` or + `_check_for_batch`. BatchFeatureHandlers will receive a `FeatureCheckBatch` + that contains the organization and object list. Feature handlers that depend only on organization attributes, and not on attributes of the individual objects being checked, will generally - perform faster if this method is used in preference to ``has``. + perform faster if this method is used in instead of ``has``. - The return value is a dictionary with the objects as keys. Each value - is what would be returned if the key were passed to ``has``. + The return value is a dictionary with the objects as keys, and each + value is the result of the feature check on the organization. - The entity handler can handle both batch project/organization - contexts so it'll likely have an entirely different implementation - of this functionality. + This method *does not* work with the `entity_handler`. >>> FeatureManager.has_for_batch('projects:feature', organization, [project1, project2], actor=request.user) """ From 8b8dc48b08bdaf43cae52edb09e7c0ab8957a914 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 29 Jul 2024 12:10:38 -0400 Subject: [PATCH 2/2] Expand docs more. --- src/sentry/features/__init__.py | 13 +++++++++---- src/sentry/features/handler.py | 5 ++++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/sentry/features/__init__.py b/src/sentry/features/__init__.py index d5bb5da067926f..b26f0064ca3d95 100644 --- a/src/sentry/features/__init__.py +++ b/src/sentry/features/__init__.py @@ -23,12 +23,17 @@ # NOTE: There is no currently established convention for features which do not # fall under these scopes. Use your best judgment for these. # +# - Decide if your feature needs to be exposed in API responses or not +# If your feature is not used in the frontend, it is recommended that you don't +# expose the feature flag as feature flag checks add latency and bloat to organization +# details and project details responses. +# # - Set a default for your features. # -# Feature defaults are configured in the sentry.conf.server.SENTRY_FEATURES -# module variable. This is the DEFAULT value for a feature, the default may be -# overridden by the logic in the exposed feature.manager.FeatureManager -# instance. See the ``has`` method here for a detailed understanding of how +# Feature defaults are configured with the `default` parameter. Default values +# can also be defined in `settings.SENTRY_FEATURES`. Default values +# are used if no registered handler makes a decision for the feature. +# See the ``has`` method here for a detailed understanding of how # the default values is overridden. # # - Use your feature. diff --git a/src/sentry/features/handler.py b/src/sentry/features/handler.py index 6299c123d69fa0..4eae09c4441610 100644 --- a/src/sentry/features/handler.py +++ b/src/sentry/features/handler.py @@ -22,6 +22,9 @@ class FeatureHandler: Subclasses should implement `has` and contain the logic necessary for the feature check. + + Generally FeatureHandlers are only implemented in `getsentry.features` + as we don't programatically release features in self-hosted. """ features: MutableSet[str] = set() @@ -68,7 +71,7 @@ class BatchFeatureHandler(FeatureHandler): It is generally better to extend BatchFeatureHandler if it is possible to do the check with no more than the feature name, organization, and actor. If it needs to unpack the Feature object and examine the flagged entity, extend - FeatureHandler directly. + FeatureHandler instead. """ @abc.abstractmethod