2686 add checkpoint with repoid #800
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #800 +/- ##
==========================================
- Coverage 97.99% 97.98% -0.01%
==========================================
Files 443 443
Lines 36513 36538 +25
==========================================
+ Hits 35780 35803 +23
- Misses 733 735 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #800 +/- ##
==========================================
- Coverage 97.99% 97.98% -0.01%
==========================================
Files 443 443
Lines 36513 36538 +25
==========================================
+ Hits 35780 35803 +23
- Misses 733 735 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #800 +/- ##
==========================================
- Coverage 97.99% 97.98% -0.01%
==========================================
Files 443 443
Lines 36513 36538 +25
==========================================
+ Hits 35780 35803 +23
- Misses 733 735 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #800 +/- ##
==========================================
- Coverage 97.99% 97.98% -0.01%
==========================================
Files 443 443
Lines 36513 36538 +25
==========================================
+ Hits 35780 35803 +23
- Misses 733 735 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| def log_counters(obj: T) -> None: | ||
| metrics.incr(f"{klass.__name__}.events.{obj.name}") | ||
| PROMETHEUS_HANDLER.log_checkpoints(flow=klass.__name__, checkpoint=obj.name) | ||
| def log_counters(obj: T, context: CheckpointContext = None) -> None: |
There was a problem hiding this comment.
| def log_counters(obj: T, context: CheckpointContext = None) -> None: | |
| def log_counters(obj: T, context: CheckpointContext | None = None) -> None: |
the same pattern repeats a bunch of times below. If the default is None, then None has to appear in the type as well.
| ): | ||
| self.cls = cls | ||
| self.data = data if data else {} | ||
| self.kwargs_key = _kwargs_key(self.cls) | ||
| self.strict = strict | ||
| self.context = context if context else {} |
There was a problem hiding this comment.
This should just be context in this case, as defaulting to {} does not really make sense.
| @@ -67,28 +126,46 @@ class PrometheusCheckpointLoggerHandler: | |||
| methods in this class are mainly used by the CheckpointLogger class. | |||
| """ | |||
|
|
|||
| def log_begun(self, flow: str): | |||
| def log_begun(self, flow: str, repo_id: int = None): | |||
There was a problem hiding this comment.
| def log_begun(self, flow: str, repo_id: int = None): | |
| def log_begun(self, flow: str, repo_id: int | None = None): |
same here, the argument type needs to match up with the default value.
| kwargs: MutableMapping[str, Any], | ||
| strict: bool = False, | ||
| context: CheckpointContext = None, |
There was a problem hiding this comment.
for this particular case, you could just pick repo_id (aka repoid?) out of the kwargs.
There was a problem hiding this comment.
I don't think that would work off the bat as the repoid has been specified as a keyword parameter, thus not belonging to **kwargs when it's supplied to the checkpoints functionality, for instance here https://github.com/codecov/worker/blob/5514b97f08b80b0615ea4122e1c3edab89dd3d45/tasks/upload.py#L300-L301. Unless you meant re-adding it to the kwargs object before we supply it. As an add-on, I created the context to a) type the supplied params and b) serve as an object that has items additional to the checkpoint flow - I'm open to a different approach, I chose this one for readability + separation of concerns. (Although I will rename repo_id to repoid to be consistent with our model definition)
There was a problem hiding this comment.
Addressed your other suggestions, thanks 🙏
| @dataclass | ||
| class CheckpointContext: | ||
| repoid: int | ||
|
|
||
|
|
There was a problem hiding this comment.
thoughts on saving this in self.data["context"] and making it a TypedDict instead of a dataclass? if it's in self.data["context"] then you only have to check it once at the start of the flow and it'll be passed along like the rest of self.data automatically. i think TypedDict can be auto-serialized to JSON which is necessary to pass it in celery task arguments
i think the type of self.data would have to change to something like Optional[MutableMapping[T | str, int | CheckpointContext]]. a little messy.
| cls: type[T], | ||
| kwargs: MutableMapping[str, Any], | ||
| strict: bool = False, | ||
| context: CheckpointContext | None = None, |
There was a problem hiding this comment.
if you take the TypedDict/self.data["context"] suggestion, you won't need to take this in from_kwargs. you will need to do a little extra work though
def from_kwargs(...):
# Copy so we don't modify the passed-in kwargs
data = kwargs.get(_kwargs_key(cls), {}).copy()
deserialized_data = {}
# Remove the "context" key. All remaining keys should be castable to checkpoints
deserialized_data["context"] = data.pop("context", CheckpointContext())
# for loop can remain the same
for checkpoints, timestamp in data.items():
...There was a problem hiding this comment.
So I had to play with this a bit to make it work. The kwargs.get(_kwargs_key(cls), {}).copy() is an empty object the first time a flow is ran, so instead I did
def from_kwargs(
cls: type[T],
kwargs: MutableMapping[str, Any],
strict: bool = False,
) -> CheckpointLogger[T]:
context = kwargs.pop("context", CheckpointContext())
data = kwargs.get(_kwargs_key(cls), {})
# kwargs has been deserialized into a Python dictionary, but our enum values
# are deserialized as simple strings. We need to ensure the strings are all
# proper enum values as best we can, and then downcast to enum instances.
deserialized_data = {}
deserialized_data["context"] = context
for checkpoint, timestamp in data.items():it could be prettier but that's what I could think, wdyt?
| @patch("helpers.checkpoint_logger.prometheus.CHECKPOINT_ENABLED_REPOSITORIES") | ||
| def test_reliability_counters_with_context(self, mock_object): | ||
| repoid = 123 | ||
| mock_object.return_value = repoid |
There was a problem hiding this comment.
mock_object is the feature object, not its check_value() function. i wonder why it appears to work anyway?
We're extending our checkpoint metrics to add repo specific metrics. The original approach considered adding a repo_id label to the existing checkpoint logger, but this is an ask for a specific repository. This approach isn't necessarily scalable with many repositories as repo_id has high cardinality, which isn't very well suited for prometheus. The better approach would be to leverage sql metrics and extend their functionality, but this approach was chosen for times sake.
This PR
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.