fix: Add logging to segments code #4625
Conversation
zachaysan
requested review from
matthewelwell
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: api/segments/models.py
📄 File: api/segments/serializers.py (Click to Expand)
Did you find this useful? React with a 👍 or 👎 |
Docker builds report
|
|
Uffizzi Preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4625 +/- ##
=======================================
Coverage 97.17% 97.17%
=======================================
Files 1162 1162
Lines 40267 40275 +8
=======================================
+ Hits 39130 39138 +8
Misses 1137 1137 ☔ View full report in Codecov by Sentry. |
api/segments/models.py
Outdated
| @@ -246,6 +247,10 @@ def deep_clone(self, cloned_segment: Segment) -> "SegmentRule": | |||
| cloned_rule.uuid = uuid.uuid4() | |||
| cloned_rule.id = None | |||
| cloned_rule.save() | |||
| if is_saas(): | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
- I sorta agree with you but this was the feedback from @rolodato so I deferred to his judgement.
- 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Two solutions come to mind:
- 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.
- 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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
This reverts commit 9203e67.
| @@ -19,6 +19,10 @@ | |||
| ], | |||
| "essential": true, | |||
| "environment": [ | |||
| { | |||
| "name": "LOG_LEVEL", | |||
| "value": "info" | |||
There was a problem hiding this comment.
It's probably fine, but for consistency, I think this should be INFO ?
There was a problem hiding this comment.
Sounds good. Updated.
|
Tested in staging 
|
Changes
Adds logging to help diagnose a strange bug that came up in production around conditions being created in large numbers.
How did you test this code?
The existing test suite should cover the code running ok.