Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create Class to Easily Monitor Feature Flag State Changes #341

Open
lannuttia opened this issue Jan 14, 2025 · 5 comments
Open

Create Class to Easily Monitor Feature Flag State Changes #341

lannuttia opened this issue Jan 14, 2025 · 5 comments
Assignees

Comments

@lannuttia
Copy link

lannuttia commented Jan 14, 2025

Is your feature request related to a problem? Please describe.
I would like to be able to use a feature flag to subscribe/unsubscribe a Kafka consumer essentially enabling or disabling consumption of messages.

Describe the solution you'd like
A solution that I think would help with this is essentially a FeatureFlag class that has a __init__ that takes all the same arguments as the UnleashClient's is_enabled method as well as an UnleashClient reference. This would use a context manager to hook into the UnleashClient's scheduler and monitor for feature flag updates. On exit, the context manager could clean up the job that monitors for updates. A callback can be registered and deregistered from being ran when a feature flag's status changes with a register and deregister method, respectively.

Describe alternatives you've considered
For this specific use case, I can't really think of any other alternative that would work quite as well.

Additional context
I have a prototype for this feature (minus the context and fallback function) here:

"""This module provides utilities for effectively utilizing feature flags."""
from contextlib import AbstractContextManager
from types import TracebackType
from typing import Callable, Self
from weakref import WeakSet

from apscheduler.events import EVENT_JOB_EXECUTED, JobExecutionEvent
from apscheduler.job import Job
from UnleashClient import UnleashClient


class FeatureFlag(AbstractContextManager):
    """This is a class for utilizing feature flags.

    :param feature_name: The feature name.
    :type  feature_name: str

    :param client: The Unleash client.
    :type client: UnleashClient
    """
    def __init__(self, feature_name: str, client: UnleashClient):
        self.feature_name = feature_name
        self._client = client
        self._update_job: Job | None = None
        self._callbacks: WeakSet[Callable[[bool], None]] = WeakSet()
        self._current_value = self._get_value()

    def __enter__(self) -> Self:
        def update_listener(event: JobExecutionEvent) -> None:
            if event.exception or event.job_id != self._client.fl_job.id:
                return
            self.enabled = self._get_value()

        self._update_job = self._client.unleash_scheduler.add_listener(
            update_listener, EVENT_JOB_EXECUTED
        )
        return self

    def __exit__(
        self,
        exc_type: type[BaseException] | None,
        exc_value: BaseException | None,
        traceback: TracebackType | None,
    ) -> None:
        if isinstance(self._update_job, Job):
            self._update_job.remove()

    def _get_value(self) -> bool:
        return bool(self._client.is_enabled(self.feature_name))

    def register(self, callback: Callable[[bool], None]) -> None:
        """This registers a callback to be ran when the value of a feature flag is updated.

        :param callback: The callback to be registered.
        :type callback: Callable[[bool], None]

        :returns: Nothing
        :rtype: None
        """
        if callback not in self._callbacks:
            callback(self.enabled)
        return self._callbacks.add(callback)

    def deregister(self, callback: Callable[[bool], None]) -> None:
        """This deregisters a callback from being ran when the value of a feature flag is updated.

        :param callback: The callback to be deregistered.
        :type callback: Callable[[bool], None]

        :returns: Nothing
        :rtype: None
        """
        return self._callbacks.remove(callback)

    @property
    def enabled(self) -> bool:
        """The current value of the feature flag.

        :returns: The current value of the feature flag.
        :rtype: bool
        """
        return self._current_value

    @enabled.setter
    def enabled(self, value: bool) -> None:
        if self._current_value is value:
            return

        self._current_value = value
        for callback in self._callbacks:
            callback(self._current_value)

I have tested this out in another project that I am working on. I think I should be able to submit a pull request that adds this and associated automated tests. I am mostly just wanting to check and make sure that this is something that the project thinks would be useful before I go and spend time creating and submitting a pull request for this.

Example Usage

from threading import Event

from UnleashClient import UnleashClient, FeatureFlag
with UnleashClient(...) as client, FeatureFlag("feature-name", client) as feature_flag:
    def _handle_feature_flag(enabled: bool) -> None:
        if enabled:
            print("enabled")
        else:
            print("disabled")
    feature_flag.register(_handle_feature_flag)
    Event().wait()
@lannuttia
Copy link
Author

Adding a note here that after a couple hours of letting this idea set, I don't think I like the names of the register and deregister methods. I am thinking that subscribe and unsubscribe might be better names for those methods. If someone has better ideas for the names of the class and/or methods, I would love to hear them. I am not particularly thrilled about the names that I currently have in the description and could easily be convinced to use different names.

@sighphyre
Copy link
Member

Hey @lannuttia, so I think some of this is really useful and some of it I want to push back gently on. We already have an event system in the client, which I think anything like this should tap into. I don't think it's unreasonable to send events when a given feature flag changes.

We already send those events on the .NET and Java SDKs. So I'd rather we use a pattern like that, since we're desperately trying to maintain some consistency between our SDKs. If that was in place, I want to say what you're trying to do here would be a few lines of code for end users.

Out of curiosity, how do you see this playing with usage metrics?

@lannuttia
Copy link
Author

I hadn't put too much thought into usage metrics to be honest. That's an interesting thing to think about though. I would think that we would want to have there be an impression per registered callback but I don't really know.

As for the event system, I do not know how exactly I would hook into that in order to functionally accomplish what I accomplished above off the top of my head. I'm sure it's possible though and based on my admittedly limited understanding of this project, that does seem like the right spot for this.

@lannuttia
Copy link
Author

It appears that the event_callback is called per-impression on a feature flag or a variant. I mention that more as a note to myself that event_callback doesn't seem like something that I could hook into to accomplish this.

Do you have a general idea of how the project would want functionality to be exposed? If I am understanding what you are thinking, you are thinking we allow someone to optionally provide a callback that gets called when a feature flag's state changes. That callback can then send that event using blinker or some analogous event handling mechanism or be used directly without blinker if the user doesn't need to register multiple handlers.

Coming back to usage metrics, I would think about trying to use a getter for the enabled property on the UnleashEvent class for feature flag state changes. If we did that, we should be able to just have it lazily run is_enabled and have the impressions handled that way. If we did that, I think we could also use the @lru_cache(maxsize=1) decorator on the getter to prevent registering more than 1 impression per event.

@sighphyre
Copy link
Member

I hadn't put too much thought into usage metrics to be honest. That's an interesting thing to think about though.

Yeah I hadn't either to be honest. We could also just skip the metrics entirely for this feature

I would think that we would want to have there be an impression per registered callback but I don't really know.

It's a bit early in the morning here but I think this makes sense.

So idle thoughts but I think passing the event emitter to the fetch code, would let us fire a event if the etag is different.

Then the existing event handling code can do something like this:

def receive_data(sender, args):
   if args.type is EventType.FeaturesUpdated:
      feature = next(feature for feature in args.features if feature["name"] == "Some-feature-we-care-about")
      //check if the feature is enabled, forward the event onto whatever you care about

Coming back to usage metrics, I would think about trying to use a getter for the enabled property on the UnleashEvent class for feature flag state changes. If we did that, we should be able to just have it lazily run is_enabled and have the impressions handled that way. If we did that, I think we could also use the @lru_cache(maxsize=1) decorator on the getter to prevent registering more than 1 impression per event.

That would probably work but I'd suggest just moving a chunk of the is_enabled call into a separate method that does the check for whether or not the feature is enabled without tacitly doing metrics. Honestly I'm not sure on the best way to handle metrics here but whatever we choose should be carefully chosen. Caches have a tendency to invalidate when you least expect and it's giving me nightmares trying to remotely debug people's metrics already

@gastonfournier I know you were more or less leaving this to me but I could use a third smart brain here to thrash out the design details with us

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Investigating
Development

No branches or pull requests

3 participants