Conversation
…get rid of vulnerability
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #30 +/- ##
==========================================
+ Coverage 90.06% 91.54% +1.48%
==========================================
Files 13 14 +1
Lines 292 426 +134
Branches 20 29 +9
==========================================
+ Hits 263 390 +127
- Misses 24 28 +4
- Partials 5 8 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ed earlier unit tests
calebsyring
left a comment
calebsyring
left a comment
There was a problem hiding this comment.
Looking good! Left some more comments. If you want to discuss in a Slack huddle, let me know.
| if uptime_log.resp_code is None: | ||
| uptime_log.resp_code = 0 | ||
| if uptime_log.latency_secs is None: | ||
| uptime_log.latency_secs = -1 |
There was a problem hiding this comment.
This is interesting. If we don't have a response code or latency secs (because there was some kind of error that means we didn't get a response), just setting response code to zero and latency secs to -1 doesn't give the user much information.
I think I'd like to put some kind of string representation of the exception in an attribute on UptimeLog.
There was a problem hiding this comment.
I think this is also a whole nother feature that should be done when we implement the check assertions feature
src/critic/tasks/run_checks.py
Outdated
| ) # this will handle exceptions from http, but not 404 or other errors | ||
|
|
||
|
|
||
| def run_checks(monitor: UptimeMonitorModel, http_client: httpx.Client): |
There was a problem hiding this comment.
This needs to be an invokable mu task. See https://github.com/level12/mu/blob/master/examples/tasker/app.py
Keep in mind that we can't pass UptimeMonitorModel objects through the AWS infrastructure, so this probably just needs to take a project_id and slug and get the uptime monitor from the db.
There was a problem hiding this comment.
Should work better now
pyproject.toml
Outdated
| 'hatch', | ||
| 'ruff', | ||
| "pytest-httpx>=0.36.0", | ||
| "ruff==0.14.14", |
There was a problem hiding this comment.
I was getting an issue with nox, when I didnt do this, but I can revert this
There was a problem hiding this comment.
Yes, please revert the ruff pin.
src/critic/models.py
Outdated
|
|
||
|
|
||
| class UptimeLog(BaseModel): | ||
| monitor_id: str # for now we just combine the monitor and slug |
There was a problem hiding this comment.
I think this comment should say "combine the project_id and slug"
| monitor_table = dynamodb.Table(UptimeMonitorTable.namespace(UPTIME_MONITOR)) | ||
|
|
||
| monitor.next_due_at = monitor.next_due_at + timedelta(minutes=monitor.frequency_mins) | ||
|
|
||
| # if paused update the time and return | ||
| if monitor.state == MonitorState.paused: | ||
| monitor_table.update_item( | ||
| Key={'project_id': monitor.project_id, 'slug': monitor.slug}, | ||
| UpdateExpression='SET next_due_at = :n', | ||
| ExpressionAttributeValues={':n': monitor.next_due_at.isoformat()}, | ||
| ) |
There was a problem hiding this comment.
Part of the point of the ddb lib / tables file is to keep query syntax out of logic files.
Let's add an update_item method to the Table base class in the ddb lib.
| monitor_id: str = monitor.project_id + monitor.slug | ||
| uptime_log = UptimeLog( | ||
| monitor_id=(monitor_id), | ||
| timestamp=logtime_stamp, |
There was a problem hiding this comment.
It's all normalized to UTC, so this can just be timestamp=datetime.now(UTC). You don't need to take into the next_due_at timezone.
| return cls.model(**deserialize(item)) | ||
|
|
||
| @classmethod | ||
| def query(cls, partition_value: str | int) -> list[BaseModel]: |
There was a problem hiding this comment.
I'm not sure this is necessary. It may be in the future. But where you use it below it looks like you could just use the get method.
There was a problem hiding this comment.
I cannot because I need the sort key which is generated in the run_checks , i supposed I might be able to mock it, but I dont see its important to do it that way and get rid of a potentially useful util. I imagine you will eventually be using query sometime in the future
| from polyfactory.factories.pydantic_factory import ModelFactory | ||
|
|
||
| from critic.models import UptimeMonitorModel | ||
|
|
||
|
|
||
| class UptimeMonitorFactory(ModelFactory[UptimeMonitorModel]): | ||
| __model__ = UptimeMonitorModel |
There was a problem hiding this comment.
Please delete this. We'll use libs/testing.py instead.
| in_data = { | ||
| 'project_id': '6033aa47-a9f7-4d7f-b7ff-a11ba9b34474', | ||
| 'slug': 'my-monitor', | ||
| 'url': 'https://example.com/health', | ||
| 'frequency_mins': 5, | ||
| 'consecutive_fails': 0, | ||
| 'next_due_at': '2025-11-10T20:35:00Z', | ||
| 'timeout_secs': 30, | ||
| 'assertions': {'status_code': 200, 'body_contains': 'OK'}, | ||
| 'failures_before_alerting': 2, | ||
| 'alert_slack_channels': ['#ops'], | ||
| 'alert_emails': ['alerts@example.com'], | ||
| 'realert_interval_mins': 60, | ||
| } | ||
| in_data = UptimeMonitorModel(**in_data) |
There was a problem hiding this comment.
In this file we should probably be using UptimeMonitorFactory.build instead of these dicts. Initially the dicts functioned as a sort of model test, but model tests should be separate.
But I think you're going to end up removing this test if you remove the unnecessary query method anyway.
There was a problem hiding this comment.
I agree, but this can be addressed on another pr
| @@ -0,0 +1,108 @@ | |||
| import logging | |||
There was a problem hiding this comment.
These tests should go in test_tasks.py
There was a problem hiding this comment.
I think it would be better to modularize the tests. It seems neater and if there are more tasks added, that file is going to be way too big
|
Closing in favor of #61 |
2 participants
Overview
run_checkMu Task for Lambda #28.Tasks
run_checks