Use searchcache results if summary file is old format

[DanielRyanSmith]

Over changes:

• adds a check before summary files are used to determine if the summaries are in the new format (they have the _v2 name suffix in their bucket URL).
• Relegates any queries that involve old summary files to the search cache, and will be processed at runtime if needed.
• Updates mock summary data used in test files to use the new summary format.

Note: The summary files in staging and production are currently being generated in the new summary file format and the _v2 name suffix, so this change will have less impact needing to compute search results without summary files.

Decision process for handling queries:

Major change explanations by file

api/query/query.go:

• New function validateSummaryVersions which checks each summary file URL associated with a test run and ensures it ends with the -summary_v2.json.gz suffix.
• Changes the struct SummaryResult to no longer hold an old format and new format for summary files, since we will no longer be using old format summary files.
• Rename getRedisKey function to getSummaryFileRedisKey to be more specific, as well as changing the existing redis key format.

api/query/search.go:

• Add a check to see if all summary files needed are in the new format. If not, treat the request as a "complex query" and use the search cache and to load the required data, or aggregate it as needed (this process does not use the summary files).
• Refactor some existing code to use Go best practices.
• Remove some logic to read old format summary files (non-complex queries), leaving only new format parsing.

shared/util.go

• Changed regex to require _v2 suffix rather than allowing _v2 as optional for summary files.

Testing file changes

api/query/query_test.go
api/query/search_test.go
shared/models_test.go
shared/test_run_query_medium_test.go
shared/util_test.go
webapp/components/test/fixtures/interop.json
util/populate_dev_data.go
webapp/components/test/fixtures/passrates.json
webapp/components/test/test-file-results.html
webapp/components/test/util/helpers.js

• Change tests to only test using the new summary file format.

A number of summary files exist in the repo to test against. These were previously in the old format and have been replaced with the new format.

webapp/static/24278ab617/chrome[experimental]-summary_v2.json.gz
webapp/static/24278ab617/chrome[experimental].json
webapp/static/24278ab617/chrome[stable]-summary_v2.json.gz
webapp/static/24278ab617/chrome[stable].json
webapp/static/24278ab617/edge[experimental]-summary_v2.json.gz
webapp/static/24278ab617/edge[experimental].json
webapp/static/24278ab617/edge[stable]-summary_v2.json.gz
webapp/static/24278ab617/edge[stable].json
webapp/static/24278ab617/firefox[experimental]-summary_v2.json.gz
webapp/static/24278ab617/firefox[stable]-summary_v2.json.gz
webapp/static/24278ab617/firefox[stable]-summary_v2.json.gz
webapp/static/24278ab617/firefox[stable].json
webapp/static/24278ab617/safari[experimental]-summary_v2.json.gz
webapp/static/24278ab617/safari[experimental].json
webapp/static/24278ab617/safari[stable]-summary_v2.json.gz
webapp/static/24278ab617/safari[stable].json

Docs file changes

api/README.md
docs/gcs.md

• Add suffix _v2 to summary file URL examples.

webapp/components/test-file-results.js

• Add a comment to appropriately explain the resultsURL method.

[DanielRyanSmith]

Thanks for taking a look James! I was hoping to view the changes deployed to a staging environment to see how they interact with a functional searchcache component. I usually add useful commenting and context for review when things feel more finalized, so I hope this wasn't too much of a mess to understand. 🙂

[jcscottiii]

Quick question: For all the old redis keys, do we add an expiration to them or will we need to do a one time cleanup of the old keys? Just thinking that we don't want to leave all the old info that won't be retrieved anymore.

[DanielRyanSmith]

We definitely will need to flush the cache prior to landing this change. I'm not sure of the protocol we have set up currently for cache data expiration, but this will certainly invalidate all of these old keys.

[KyleJu]

Good job! Just a few thoughts:

Overall it would be great if you could break down changes based on different services - one for searchcache and one for processor. (I understand the change to processor is rather minimal in this case). It easier to test/deploy/revert one service at a time.

On the design side, it seems to me that we have to validate new summary files for both webapp and searchcache? Does it mean that we have to direct all the traffic to searchcache first? or we only check it for some pages, like interop202x tests?

Alternatively, we can pack the changes in api/query/search.go to an API endpoint, e.g. /has-summary-file that checks if a file exist. The reason being that webapp can scale and handle traffic much better than searchcache and MUCH less resource intensive. Also by doing that, we can pretty much keep the searchcache logic intact, rather than change the isSimpleQ logic

[DanielRyanSmith]

Thanks for taking a look @jcscottiii and @KyleJu 😊
Sorry I took so long to respond!

Overall it would be great if you could break down changes based on different services - one for searchcache and one for processor. (I understand the change to processor is rather minimal in this case). It easier to test/deploy/revert one service at a time.

I did a bit of this

On the design side, it seems to me that we have to validate new summary files for both webapp and searchcache? Does it mean that we have to direct all the traffic to searchcache first?

The summary files are not validated on webapp, but the summary URL is sliced and used to determine the URL for getting single test data results, because the single test data is prefaced with the same name as the summary (I've updated the method in test-file-results.js with a comment description to better explain what's happening there, as it wasn't obvious). For any test run query outside of single test views, the back end is sent a request and determines how to process the query.

When determining isSimpleQ, there is some additional work having to look at the summary file URLs and see if they have the correct suffix. However, this is relatively negligible and the possible strain comes from having to disregard the old summary files and aggregate the test results at query time, which is something that would need to happen even if we tried to determine the summary file viability on the webapp.
(I hope this makes sense - it's a convoluted explanation 😅 )

[DanielRyanSmith]

@jcscottiii @KyleJu This PR is ready for review once again. I've tried to thoroughly explain the changes file-by-file in the description above. Hopefully that makes it a bit easier to understand the approach. Let me know if there's anything I can do to make this easier!

[jcscottiii]

I added comments about the custom error part. I forgot to mention the ability to wrap errors and assert for errors earlier. Sorry about that.

Wrapping errors allows you to take an error and add more information to it. Then with errors.Is, it can unwrap the error and do the type assertion for you on your custom error.

More details about it:

• https://go.dev/blog/go1.13-errors
• Working with Error Wrapping slides

I have also created a go playground for you to test out if you want. https://go.dev/play/p/SYd3AUfKX1H

[DanielRyanSmith]

Thanks for the error handling help @jcscottiii 😊 maybe looks better now.

[KyleJu]

Looks good! LGTM

[DanielRyanSmith]

Thanks for all the help @jcscottiii @KyleJu 😊