feat: use cached results from GCS in TA GQL #952
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #952 +/- ##
==========================================
- Coverage 96.25% 96.04% -0.22%
==========================================
Files 827 827
Lines 19090 19068 -22
==========================================
- Hits 18376 18313 -63
- Misses 714 755 +41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
❌ 5 Tests Failed:
View the top 3 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
|
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 2606 tests with View the full list of failed testspytest
|
this refactor simplifies the tests from test results aggregates and flake aggregates and adds some typing to the generate_test_results function. It also changes the arguments passed to the generate_test_results function so handling them is simpler and changes some things regarding error handling in that function.
we want the GQL endpoints to consume the cached query results from GCS and then do some filtering and ordering on top of that, and also do some extra caching of those results in redis. this commit also contains a bunch of reorganization and refactoring of the GQL code
when we miss the redis cache but find the test rollup results in GCS we want to queue up a task to cache it in redis so subsequent calls to the GQL endpoint for the same repo get to hit redis instead of GCS
joseph-sentry
force-pushed
the
joseph/cache-test-results
branch
from
a4a6e79
to
e3dd9a7
Compare
ajay-sentry
reviewed
Nov 7, 2024
| flake_rate: float | ||
| flake_rate_percent_change: float | None | ||
| flake_count_percent_change: float | None = None |
There was a problem hiding this comment.
dang you didn't like having it grouped huh
There was a problem hiding this comment.
all args with defaults must come after arguments with no defaults 😭
ajay-sentry
reviewed
Nov 7, 2024
| flake_rate_percent_change: float | None = None | ||
|
|
||
|
|
||
| def calculate_flake_aggregates(table: pl.DataFrame) -> pl.DataFrame: |
There was a problem hiding this comment.
is it possible to know what columns exist on the table at this point? Right now it's a little "magic"
There was a problem hiding this comment.
the names of the columns are in the worker repo where it's being written, come to think of it maybe all the serialization and reading stuff can be in shared but i don't know if that would make things worst, we only read in api and we only write in worker
ajay-sentry
reviewed
Nov 7, 2024
| ), | ||
| (pl.col("total_skip_count").sum()).alias("skips"), | ||
| (pl.col("total_fail_count").sum()).alias("fails"), | ||
| ((pl.col("avg_duration") >= pl.col("avg_duration").quantile(0.95)).sum()).alias( |
There was a problem hiding this comment.
do we still need to do the limit 100 here?
There was a problem hiding this comment.
indeed, that's being handled above but not here
ajay-sentry
reviewed
Nov 7, 2024
|
|
||
| merged_results: pl.DataFrame = pl.concat([past_aggregates, curr_aggregates]) | ||
|
|
||
| merged_results = merged_results.with_columns( |
There was a problem hiding this comment.
It was a bit confusing for me at first to see this since I didn't realize with_columns is essentially an append or upsert depending on the column existing
There was a problem hiding this comment.
i will leave a comment
ajay-sentry
reviewed
Nov 7, 2024
| first: int | None, | ||
| last: int | None, | ||
| ) -> None: | ||
| if interval not in {1, 7, 30}: |
There was a problem hiding this comment.
nit: do we have these magic numbers typed out as consts somewhere?
ajay-sentry
reviewed
Nov 7, 2024
| ).decode("ascii") | ||
|
|
||
|
|
||
| def validate( |
There was a problem hiding this comment.
Random thought here, does the order of these validations matter? Do we want it to matter?
There was a problem hiding this comment.
nope, and i don't think it should matter (maybe in past iterations it would've(
ajay-sentry
reviewed
Nov 7, 2024
|
|
||
| if flags: | ||
| table = table.filter( | ||
| pl.col("flags").list.eval(pl.element().is_in(flags)).list.any() |
There was a problem hiding this comment.
Claude was telling me we might be able to add these guards to this check, not sure if you think we need em tho
pl.col("flags").is_not_null() & # handle null rows
pl.col("flags").list.len() > 0 & # handle empty lists
There was a problem hiding this comment.
i think the is not null check makes sense
ajay-sentry
reviewed
Nov 7, 2024
| total_count = table.height | ||
|
|
||
| def ordering_expression(cursor_value: CursorValue, is_forward: bool) -> pl.Expr: | ||
| if is_forward: |
There was a problem hiding this comment.
what does forward imply here? It's also a little confusing that we use the value of is_forward and then change the value of is_forward in the same fn
There was a problem hiding this comment.
is_forward is just about determining the ordering of the table, but ordering_expression only reads is_forward, it's the outer function that sets the value and invokes ordering_expression so i ended up just moving it out
ajay-sentry
reviewed
Nov 7, 2024
|
|
||
| rows = [TestResultsRow(**row) for row in page_elements.rows(named=True)] | ||
|
|
||
| page: list[dict[str, str | TestResultsRow]] = [ |
There was a problem hiding this comment.
this should be pages right? More than one
There was a problem hiding this comment.
no, this is a single page, since it's after all the ordering and filtering
ajay-sentry
reviewed
Nov 8, 2024
| return await sync_to_async(generate_test_results_aggregates)( | ||
| repoid=repository.repoid, interval=convert_interval_to_timedelta(interval) | ||
| repoid=repository.repoid, interval=interval.value if interval else 30 |
There was a problem hiding this comment.
these 30s can be MeasurementInterval.INTERVAL_30_DAY right?
ajay-sentry
reviewed
Nov 8, 2024
| interval_start: int, | ||
| interval_end: int | None = None, | ||
| ) -> str: | ||
| key = f"test_results:{repoid}:{branch}:{interval_start}" |
There was a problem hiding this comment.
Since this is an int... does this have a reasonable enough level of precision so we actually have some cache hits sometimes?
Like if the timestamp is 1231232132 vs. 1231232135 it seems like that would trigger a new rollup to be cached/created
There was a problem hiding this comment.
there's no timestamp being use in the key, it's just the range of days that the cached values apply to, for example 1 day, 7 days, or 30 days
|
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
this commit refactors the code and tests related to ordering direction if we map ordering direction asc = 1 and desc = 0 and first = 1 and last = 0 then the ordering direction is an xor of those two variables ordering direction determines whether we're filtering by greater than or lesser than this commit also fixes a bug in the specification of slow tests
ajay-sentry
approved these changes
Nov 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
we want the GQL endpoints to consume the cached query results from GCS
and then do some filtering and ordering on top of that, and also do some
extra caching of those results in redis.
this commit also contains a bunch of reorganization and refactoring of
the GQL code