Feat(metrics): Crash free breakdown on metrics backend [INGEST-249] #28945
Conversation
src/sentry/release_health/metrics.py
Outdated
| start: datetime, | ||
| environments: Optional[Sequence[EnvironmentName]] = None, | ||
| ) -> Callable[[datetime], CrashFreeBreakdown]: | ||
| def dummy_fn(end: datetime) -> CrashFreeBreakdown: |
There was a problem hiding this comment.
Can we call this something else like generate_defaults or something?
src/sentry/release_health/metrics.py
Outdated
| conditions.append(Condition(Column(environment_key), Op.IN, environment_values)) | ||
|
|
||
| def query_stats(end: datetime) -> CrashFreeBreakdown: | ||
| def _get_data(entity: EntityKey, metric_name: str) -> Tuple[int, int]: |
| timedelta(days=1), | ||
| timedelta(days=2), | ||
| timedelta(days=7), | ||
| timedelta(days=14), |
There was a problem hiding this comment.
- What is the reason behind looping over a tuple here?
- Not a big fan of looping over the timedelta, so much repitition, why don't you just loop over the number of days and thencall timedelta once outside of the loop
There was a problem hiding this comment.
tuples are cheaper to construct
There was a problem hiding this comment.
Sure but how expensive really is it to have a list of 4 items?
There was a problem hiding this comment.
I get the argument that lists are supposed to be the homogenous container and tuples the heterogenous but I don't buy it. Not a lot of code works this way and it seems like a style guideline that was invented long after tuples and lists were. For all practical purposes tuples are immutable lists as long as mypy doesn't get in your way about it.
There was a problem hiding this comment.
FWIW I deliberately did not touch this code but simply copied it over from sessions.py.
src/sentry/release_health/metrics.py
Outdated
| try: | ||
| item_start = start + offset | ||
| if item_start > now: | ||
| if last is None or (item_start - last).days > 1: |
There was a problem hiding this comment.
item_start - last > timedelta(days=1)
There was a problem hiding this comment.
I deliberately did not touch this code but simply copied it over from sessions.py.
|
Can you please a description to the PR? |
| timedelta(days=1), | ||
| timedelta(days=2), | ||
| timedelta(days=7), | ||
| timedelta(days=14), |
There was a problem hiding this comment.
tuples are cheaper to construct
| break | ||
| rv.append(query_fn(item_start)) | ||
| last = item_start | ||
| except QueryOutsideRetentionError: |
There was a problem hiding this comment.
instead of having dummy_fn, what about letting the tag indexer exceptions bubble up until here?
There was a problem hiding this comment.
I wanted to do the whole "does tag exist" dance once instead of in every loop iteration.
There was a problem hiding this comment.
Okay, makes sense. I think we may want an in-process cache around indexer anyway, because we have some endpoints that just call one method on the releasehealth service after the other.
There was a problem hiding this comment.
Yes, the in-memory cache should be part of the indexer service IMO. I'm writing all my code under the assumption that lookups will be optimized there.
| return { | ||
| "date": end, | ||
| "total_users": users_total, | ||
| "crash_free_users": 100 - users_crashed / float(users_total) * 100 |
There was a problem hiding this comment.
I would unify how this is calculated everywhere because if i recall correctly @untitaker might have been doing this differently?
There was a problem hiding this comment.
No I did exactly this... we're just porting the formulas from sessions.py where it was also duplicated but also consistent.
not opposed to deduping in any case
There was a problem hiding this comment.
Let's dedup when we do https://getsentry.atlassian.net/browse/INGEST-456.
Port get_crash_free_breakdown from sentry.snuba.sessions to metrics.
https://getsentry.atlassian.net/browse/INGEST-249