(redo) feat: CheckpointLogger v2: cleaner usage, reliability counters, more UploadFlow metrics #152
Conversation
matt-codecov
requested review from
trent-codecov,
dana-yaish,
giovanni-guidini,
scott-codecov and
joseph-sentry
matt-codecov
changed the title
Codecov Report
@@ Coverage Diff @@
## main #152 +/- ##
========================================
Coverage 98.43% 98.43%
========================================
Files 346 347 +1
Lines 27069 27281 +212
========================================
+ Hits 26645 26855 +210
- Misses 424 426 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov Report
Changes have been made to critical files, which contain lines commonly executed in production. Learn more @@ Coverage Diff @@
## main #152 +/- ##
==========================================
- Coverage 98.46% 98.40% -0.06%
==========================================
Files 373 373
Lines 27665 27860 +195
==========================================
+ Hits 27239 27417 +178
- Misses 426 443 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #152 +/- ##
========================================
Coverage 98.43% 98.43%
========================================
Files 346 347 +1
Lines 27069 27281 +212
========================================
+ Hits 26645 26855 +210
- Misses 424 426 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov Report
@@ Coverage Diff @@
## main #152 +/- ##
========================================
Coverage 98.43% 98.43%
========================================
Files 346 347 +1
Lines 27069 27281 +212
========================================
+ Hits 26645 26855 +210
- Misses 424 426 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| into the enum's value (e.g. "MEMBER_NAME" -> "MyEnum.MEMBER_NAME") | ||
| """ | ||
| value = f"{cls.__name__}.{value}" | ||
| return super().__new__(cls, value) |
There was a problem hiding this comment.
Is this where the de-serialization magic is happening such that we can have a string like "MyEnum.A" and it get's deserialized back into the actual enum member?
There was a problem hiding this comment.
i think technically it doesn't deserialize into the enum member, it deserializes into a str which can be downcast to the enum member which inherits a comparison function from str, but yes
each enum value is an instance of the enum class that gets created at startup, and it appears _generate_next_value_() determines what gets passed into __new__() when using auto() to declare the enum member. so we get the member name ("A") from _generate_next_value_(), we prepend the class name ("MyEnum") in __new__(), and then we pass that value to the superclass constructors (i think this calls __new__() from both str and Enum)
defining __str__() doesn't work, the json encoder is looking specifically for a str or subclass thereof, it doesn't try to cast
undo of #151 / redo of #100 with bug fixed
top commit is just the bugfix, bottom commit is #100 unmodified
the bug was: we serialize dicts of checkpoints data between worker tasks, and that requires
BaseFlowto inherit fromstr. #100 forgot and removed that inheritance. this PR adds that inheritance back, adds a regression test, and makes the serialized representation of a checkpoint include the enum name so we can actually validate them as intendedoriginal description from #100
this PR adds some features to
CheckpointLogger:@subflows()decorator@success_events()and@failure_events()decorators@reliability_countersdecorator{flow}.events.{checkpoint}){flow}.total.begunwhen the first checkpoint is logged{flow}.total.failedwhen any@failure_events()checkpoint is logged{flow}.total.succeededwhen any@success_events()checkpoint is logged{flow}.total.endedwhen any terminal checkpoint (success of failure) is loggedsome reliability metrics this enables:
{flow}.total.ended / {flow}.total.begun: should be 1, but if it isn't that suggests there are exit conditions that you haven't instrumented{flow}.total.succeeded / {flow}.total.begun: success rate. denominator maybe could beendedinstead{flow}.total.failed / {flow}.total.begun: failure rate. denominator maybe could beendedinstead{flow}.events.SPECIFIC_ERROR / {flow}.total.failed: % of failures caused bySPECIFIC_ERRORthis PR also instruments more of the exit conditions of the upload flow. there are more non-error exit conditions (no pending jobs, not sending notifs, stale PR head) as well as error exit conditions (no valid bot, git service errors, too many retries...)
sentry is still the backend for subflow durations and statsd is still the backend for the reliability counters, but i'm looking to change both in future iterations.
before
the checkpoints are defined in one place, but the relationships between them (the subflows) are peppered throughout the code.
now
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.