fix: correctly count metrics when a flag with dependencies has disabled parents #304

thomasheartman · 2024-03-07T07:00:54Z

Description

This PR fixes a bug where we didn't count metrics correctly: if the dependencies were not satisfied during a get_variant call, we wouldn't track any metrics for the flag at all. During an is_enabled call, we'd track the result of the child flag's evaluation without taking into account the dependencies.

Note that the behavior was still correct. This is only about metrics.

The root cause is related to how we check dependencies and act on that result: the Unleash client checks dependencies as part of its is_enabled and get_variant calls, but the metrics are tracked in the individual Feature objects.

The current implementation was the quickest way I could fix the issue. I think we should restructure it (probably; either now or later) to avoid calling internal methods of the Feature object. However, it serves as a nice way to show what's wrong and how we can fix it. (Maybe just make that _count_variant method non-internal?)

Due to the way the code is organized already, it might make sense to move the dependencies check into the Feature class instead of performing it in the Unleash Client. However, that would require us to have some way to access those dependencies from the dependent feature. This probably means more restructuring. I'm not sure that's worth it. Maybe it's better to leave as is? I'll take any input on it 🙋🏼

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Unit tests

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

…ed parents

for more information, see https://pre-commit.ci

tests/unit_tests/test_client.py

thomasheartman · 2024-03-07T07:02:58Z

UnleashClient/__init__.py

+                    feature.increment_stats(False)
+                    feature._count_variant("disabled")


We never counted stats for variants at all if dependencies were not satisfied.

thomasheartman · 2024-03-07T07:04:04Z

UnleashClient/__init__.py

+                if dependency_check:
+                    feature_check = feature.is_enabled(context)
+                else:
+                    feature.increment_stats(False)
+                    feature_check = False


If the flag is enabled itself, but its dependencies are not satisfied, we'd count it as a "yes" previously (by checking "is_enabled" first.

coveralls · 2024-03-07T07:06:10Z

coverage: 97.031% (+0.1%) from 96.905%
when pulling 2f4a2ff on fix/metrics-counting-for-flags-with-disabled-parents
into 31c33eb on main.

sighphyre

It's quick and dirty and I'm okay with that. This code seems some refactor love but that, I think, is too big to girl scout in this PR.

Would really appreciate it if we could spend a 30 mins to hour to see if we can reduce the sleep in the test in the spirit of not making the test suite worse but the fix looks cool

sighphyre · 2024-03-07T08:00:25Z

tests/unit_tests/test_client.py

+    unleash_client.get_variant(child)
+
+    # Verify that the parent doesn't have any metrics registered
+    time.sleep(12)


I think this is my only objection here. These tests are very painful to run since they all trigger long sleeps. This one adds a full 13 seconds

The client is being newed up with a 10 second metrics send interval. Can we see what happens to the test suite if we reduce that to 1 and trim the sleep down to say 2 seconds?

Yeah, sure. I'll have a look. Think I might have had a look before, but can't remember why I didn't follow through in that case. Will check.

So, there's a suggestion in 1abe74e. It worked when I last ran it locally, but I'm suspecting it might be a little flaky. Not sure that's a cost we wanna pay. Have a look.

And of course, we can move it out into a separate PR too. But just wanted some input on it first.

Yeah, sure. I'll have a look. Think I might have had a look before, but can't remember why I didn't follow through in that case. Will check.

I think I have too and failed to improve this a few times. Looks like the build server hates that change. I think these are accidentally doing more than just pausing execution.

Okay, let's not do that then, this looks to be bigger than just this PR, I just to make sure it wasn't an easy fix we weren't doing.

Let's remove the commit, I'm okay for this to go in without the timing change

Summary of in-person chat: I think I have a way to fix this. Will take those changes into a new PR (#305 tentatively)

thomasheartman and others added 2 commits March 7, 2024 07:55

fix: correctly count metrics when a flag with dependencies has disabl…

4a6f2ce

…ed parents

[pre-commit.ci] auto fixes from pre-commit.com hooks

576216f

for more information, see https://pre-commit.ci

thomasheartman commented Mar 7, 2024

View reviewed changes

thomasheartman requested review from sighphyre and kwasniew March 7, 2024 07:08

thomasheartman added 2 commits March 7, 2024 08:39

fix: remove print

fb3db22

fix: remove unused variable declarations

2f4a2ff

sighphyre approved these changes Mar 7, 2024

View reviewed changes

thomasheartman force-pushed the fix/metrics-counting-for-flags-with-disabled-parents branch from 410c5f8 to 2f4a2ff Compare March 7, 2024 10:30

thomasheartman enabled auto-merge (squash) March 7, 2024 10:36

thomasheartman merged commit 232da69 into main Mar 7, 2024
70 checks passed

thomasheartman deleted the fix/metrics-counting-for-flags-with-disabled-parents branch March 7, 2024 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: correctly count metrics when a flag with dependencies has disabled parents #304

fix: correctly count metrics when a flag with dependencies has disabled parents #304

thomasheartman commented Mar 7, 2024 •

edited

thomasheartman Mar 7, 2024

thomasheartman Mar 7, 2024

coveralls commented Mar 7, 2024 •

edited

sighphyre left a comment

sighphyre Mar 7, 2024

thomasheartman Mar 7, 2024

thomasheartman Mar 7, 2024

thomasheartman Mar 7, 2024

sighphyre Mar 7, 2024

thomasheartman Mar 7, 2024

		feature.increment_stats(False)
		feature._count_variant("disabled")

fix: correctly count metrics when a flag with dependencies has disabled parents #304

fix: correctly count metrics when a flag with dependencies has disabled parents #304

Conversation

thomasheartman commented Mar 7, 2024 • edited

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 7, 2024 • edited

sighphyre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasheartman commented Mar 7, 2024 •

edited

coveralls commented Mar 7, 2024 •

edited