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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from Mar 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4a6f2ce
fix: correctly count metrics when a flag with dependencies has disabl…
thomasheartman Mar 7, 2024
576216f
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 7, 2024
fb3db22
fix: remove print
thomasheartman Mar 7, 2024
2f4a2ff
fix: remove unused variable declarations
thomasheartman Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 11 additions & 3 deletions UnleashClient/__init__.py
View file
Open in desktop
Expand Up @@ -364,9 +364,15 @@ def is_enabled(
if self.unleash_bootstrapped or self.is_initialized:
try:
feature = self.features[feature_name]
feature_check = feature.is_enabled(
context
) and self._dependencies_are_satisfied(feature_name, context)
dependency_check = self._dependencies_are_satisfied(
feature_name, context
)

if dependency_check:
feature_check = feature.is_enabled(context)
else:
feature.increment_stats(False)
feature_check = False
Comment on lines +371 to +375
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


if feature.only_for_metrics:
return self._get_fallback_value(
Expand Down Expand Up @@ -449,6 +455,8 @@ def get_variant(self, feature_name: str, context: Optional[dict] = None) -> dict
feature = self.features[feature_name]

if not self._dependencies_are_satisfied(feature_name, context):
feature.increment_stats(False)
feature._count_variant("disabled")
Comment on lines +458 to +459
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return DISABLED_VARIATION

variant_check = feature.get_variant(context)
Expand Down
33 changes: 32 additions & 1 deletion tests/unit_tests/test_client.py
View file
Open in desktop
Expand Up @@ -572,6 +572,7 @@ def test_uc_doesnt_count_metrics_for_dependency_parents(unleash_client):
time.sleep(1)

child = "ChildWithVariant"
parent = "Parent"
thomasheartman marked this conversation as resolved.
Show resolved Hide resolved
# Check a flag that depends on a parent
unleash_client.is_enabled(child)
unleash_client.get_variant(child)
Expand All @@ -581,7 +582,37 @@ def test_uc_doesnt_count_metrics_for_dependency_parents(unleash_client):
request = json.loads(responses.calls[-1].request.body)
assert request["bucket"]["toggles"][child]["yes"] == 2
assert request["bucket"]["toggles"][child]["variants"]["childVariant"] == 1
assert "Parent" not in request["bucket"]["toggles"]
assert parent not in request["bucket"]["toggles"]


@responses.activate
def test_uc_counts_metrics_for_child_even_if_parent_is_disabled(unleash_client):
# Set up API
responses.add(responses.POST, URL + REGISTER_URL, json={}, status=202)
responses.add(
responses.GET,
URL + FEATURES_URL,
json=MOCK_FEATURE_WITH_DEPENDENCIES_RESPONSE,
status=200,
)
responses.add(responses.POST, URL + METRICS_URL, json={}, status=202)

# Create Unleash client and check initial load
unleash_client.initialize_client()
time.sleep(1)

child = "WithDisabledDependency"
parent = "Disabled"
# Check a flag that depends on a disabled parent
unleash_client.is_enabled(child)
unleash_client.get_variant(child)

# Verify that the parent doesn't have any metrics registered
time.sleep(12)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

request = json.loads(responses.calls[-1].request.body)
assert request["bucket"]["toggles"][child]["no"] == 2
assert request["bucket"]["toggles"][child]["variants"]["disabled"] == 1
assert parent not in request["bucket"]["toggles"]


@responses.activate
Expand Down