Add cronjob for syncing heartbeats with heartbeat-missing incidents - #1954
Add cronjob for syncing heartbeats with heartbeat-missing incidents#1954hmpf wants to merge 10 commits into
Conversation
Oopsies discovered
|
Test results 8 files 1 856 suites 3m 43s ⏱️ Results for commit 43455d1. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1954 +/- ##
==========================================
+ Coverage 88.79% 89.11% +0.31%
==========================================
Files 152 154 +2
Lines 7276 7469 +193
==========================================
+ Hits 6461 6656 +195
+ Misses 815 813 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hmpf
changed the title
9edb660 to
2716a93
Compare
9819f90 to
2526843
Compare
2716a93 to
210f86e
Compare
8ebdaa5 to
39de931
Compare
210f86e to
4fee3d0
Compare
|
Warning 2 of 38 new test names are missing convention keywords (
Why am I seeing this?Test names should follow a loose given/when/then pattern with keywords like Examples:
|
4fee3d0 to
f708f73
Compare
9b35864 to
fbe1b88
Compare
a220d49 to
8e9fc88
Compare
fbe1b88 to
0e6c980
Compare
8e9fc88 to
8c75ae1
Compare
27163f8 to
e99c801
Compare
stveit
left a comment
stveit
left a comment
There was a problem hiding this comment.
Posting this as "comment" since one comment is a nitpick and the other might be a "request change", but im not entirely sure about the intentions of the code
| def TestCreateStatelessIncident(TestCase): | ||
| def test_create_stateless_incident(self): | ||
| def test_it_should_always_return_a_stateless_incident(self): |
There was a problem hiding this comment.
This test class was b0rked before this PR ....
There was a problem hiding this comment.
A.K.A. Always start with empty tests, run them and observe they are failing (so you know they have been discovered by the test runner) - then implement them.
| def _close_incidents_whose_sources_are_alive_again(timestamp): | ||
| argus = SourceSystem.objects.get(name="argus") | ||
| still_dead_sources = SourceSystem.objects.dead(timestamp - _FUDGE) | ||
| outdated_incidents = Incident.objects.heartbeat_incidents().open() |
There was a problem hiding this comment.
I would call this rather open_heartbeat_incidents - because we don't know if they are outdated yet
There was a problem hiding this comment.
Call what? Which of the four lines?
There was a problem hiding this comment.
outdated_incidents -> open_heartbeat_incidents
|
|
||
|
|
||
| def _create_incidents_for_dead_sources(timestamp: Optional[datetime] = None): | ||
| # Create existing incidents whose sources have become dead |
There was a problem hiding this comment.
This comment confuses me - are you creating them or are they existing? And this should be a docstring
| timestamp: Optional[datetime] = None, | ||
| level: int = DEFAULT_LEVEL, | ||
| ): | ||
| assert isinstance(source, SourceSystem), "source is not a SourceSystem" |
There was a problem hiding this comment.
| assert isinstance(source, SourceSystem), "source is not a SourceSystem" | |
| assert isinstance(source, SourceSystem), f"source {source} is not a SourceSystem" |
| return qs | ||
|
|
||
| def dead(self, timestamp: Optional[datetime] = None): | ||
| """Find sources that have missed heartbeats |
There was a problem hiding this comment.
Might be helpful to mention in the docstring what the timestamp means in this context
|
|
||
| @tag("db") | ||
| class TestIncidentQuerySetTagMethods(TestCase): | ||
| def test_when_no_incident_has_source_with_key_return_empty_queryset(self): |
github-project-automation
Bot
moved this from ❓ Ready for review
to ♻ Changes requested
in Argus development, public
hmpf
moved this from ♻ Changes requested
to ❓ Ready for review
in Argus development, public
| DEFAULT_LEVEL = 4 | ||
| _FUDGE = timedelta(seconds=DEFAULT_FUDGE_FACTOR) | ||
| HEARTBEAT_FUDGE = timedelta(seconds=DEFAULT_FUDGE_FACTOR) | ||
| INCIDENT_DESCRIPTION_TEMPLATE = "Missing heartbeat from source {source}, dead?" |
There was a problem hiding this comment.
| INCIDENT_DESCRIPTION_TEMPLATE = "Missing heartbeat from source {source}, dead?" | |
| INCIDENT_DESCRIPTION_TEMPLATE = "Missing heartbeat from source {source} ({type}), dead?" |
There was a problem hiding this comment.
Nope. If you look at where it is used, line 172:
.. and at what str does for a source, you will see that explicitness here not necessary.
Co-authored-by: Johanna England <johanna.england@sikt.no>
5d0a13f to
43455d1
Compare
|
4 participants
Scope and purpose
Fixes #1947
Depends on #1962
Revieweing tips
The first commit does the heavy lifting, the second adds the actual command.
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to Argus can be found in the
Development docs.