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

fix: Add logging to segments code #4625

Merged
merged 7 commits into from
Sep 26, 2024
Merged

fix: Add logging to segments code #4625

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1abd38f
Add logging to segments code
zachaysan Sep 12, 2024
9203e67
Add check for is_saas
zachaysan Sep 12, 2024
fb279ba
Revert "Add check for is_saas"
zachaysan Sep 13, 2024
c09fb4d
Check for instance before logging the info
zachaysan Sep 13, 2024
0c47c08
Set `LOG_LEVEL` to `info` in staging and production
zachaysan Sep 13, 2024
74687de
Uppercase log level
zachaysan Sep 16, 2024
e84ad26
Trigger build
zachaysan Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions api/segments/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
)
from flag_engine.segments import constants

from app.utils import is_saas
from audit.constants import (
SEGMENT_CREATED_MESSAGE,
SEGMENT_DELETED_MESSAGE,
Expand Down Expand Up @@ -246,6 +247,10 @@
cloned_rule.uuid = uuid.uuid4()
cloned_rule.id = None
cloned_rule.save()
if is_saas():
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to worry about only logging in SaaS - most people won't have the log level set to info anyway ... in fact, I'm not sure that we do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I sorta agree with you but this was the feedback from @rolodato so I deferred to his judgement.
  2. We don't log info in production? That's really surprising. How should I diagnose issues like this where we want to track down problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I've reverted the commit that adds the is_saas check, though I'm not really sure if this PR is useful if we're not logging info in production so I still need guidance on that one.

Copy link
Contributor

Choose a reason for hiding this comment

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

(2) based on this and this, I don't think we do.

You'll need to propose a solution :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two solutions come to mind:

  1. Enable logging info in production. I don't really see the downside of logging info. It's the log level that's been set at every other company I've been at and I can't be the only engineer that's surprised to see that it's not logged. Disabling debug level logging makes sense though.
  2. Switch the logging level to warning which sorta rubs me the wrong way since these aren't really warnings, but I can add comments next to each logging warning call that says basically "these aren't really warnings but we want to log them in production and production is currently set at the warning level" but something more pithier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I'm happy with (1) - can you add that to this PR? It should just be a case of setting that environment variable in the task definition JSON file (+ the one for staging too for consistency).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I think I've set the environment variables for staging and production properly, but I'm not really sure how to test the setting. Maybe we can verify once this is merged into main on staging?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, exactly.

logger.info(

Check warning on line 251 in api/segments/models.py

View check run for this annotation

Codecov / codecov/patch

api/segments/models.py#L251 

Added line #L251 was not covered by tests
f"Deep copying rule {self.id} for cloned rule {cloned_rule.id} for cloned segment {cloned_segment.id}"
)

# Conditions are only part of the sub-rules.
assert self.conditions.exists() is False
Expand All @@ -259,6 +264,11 @@
cloned_sub_rule.uuid = uuid.uuid4()
cloned_sub_rule.id = None
cloned_sub_rule.save()
if is_saas():
logger.info(

Check warning on line 268 in api/segments/models.py

View check run for this annotation

Codecov / codecov/patch

api/segments/models.py#L268 

Added line #L268 was not covered by tests
f"Deep copying sub rule {sub_rule.id} for cloned sub rule {cloned_sub_rule.id} "
f"for cloned segment {cloned_segment.id}"
)

cloned_conditions = []
for condition in sub_rule.conditions.all():
Expand All @@ -267,6 +277,12 @@
cloned_condition.uuid = uuid.uuid4()
cloned_condition.id = None
cloned_conditions.append(cloned_condition)
if is_saas():
logger.info(

Check warning on line 281 in api/segments/models.py

View check run for this annotation

Codecov / codecov/patch

api/segments/models.py#L281 

Added line #L281 was not covered by tests
f"Cloning condition {condition.id} for cloned condition {cloned_condition.uuid} "
f"for cloned segment {cloned_segment.id}"
)

Condition.objects.bulk_create(cloned_conditions)

return cloned_rule
Expand Down
10 changes: 10 additions & 0 deletions api/segments/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import logging
import typing

from django.conf import settings
Expand All @@ -8,11 +9,14 @@
from rest_framework.serializers import ListSerializer
from rest_framework_recursive.fields import RecursiveField

from app.utils import is_saas
from metadata.models import Metadata
from metadata.serializers import MetadataSerializer, SerializerWithMetadata
from projects.models import Project
from segments.models import Condition, Segment, SegmentRule

logger = logging.getLogger(__name__)


class ConditionSerializer(serializers.ModelSerializer):
delete = serializers.BooleanField(write_only=True, required=False)
Expand Down Expand Up @@ -89,6 +93,10 @@

# Create a version of the segment now that we're updating.
cloned_segment = instance.deep_clone()
if is_saas():
logger.info(

Check warning on line 97 in api/segments/serializers.py

View check run for this annotation

Codecov / codecov/patch

api/segments/serializers.py#L97 

Added line #L97 was not covered by tests
f"Updating cloned segment {cloned_segment.id} for original segment {instance.id}"
)

try:
self._update_segment_rules(rules_data, segment=instance)
Expand Down Expand Up @@ -122,6 +130,8 @@
return

count = self._calculate_condition_count(rules_data)
if is_saas():
logger.info(f"Segment {self.instance.id} has count of conditions {count}")

Check warning on line 134 in api/segments/serializers.py

View check run for this annotation

Codecov / codecov/patch

api/segments/serializers.py#L134 

Added line #L134 was not covered by tests

if count > settings.SEGMENT_RULES_CONDITIONS_LIMIT:
raise ValidationError(
Expand Down
Loading