Bug 1846013 Allows users to fetch results from all frameworks - #9761
Bug 1846013 Allows users to fetch results from all frameworks#9761gopar wants to merge 1 commit into
Conversation
✅ Deploy Preview for treeherder ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| # Build common result dictionary (contains only data both test versions use) | ||
| is_complete = base_runs_count and new_runs_count | ||
| resolved_framework = ( | ||
| framework or base_sig.get("framework_id") or new_sig.get("framework_id") |
There was a problem hiding this comment.
Who should win here when framework is empty? base or new?
There was a problem hiding this comment.
The expected logic for this task is that if the framework parameter is omitted by the frontend, the system should default to displaying comparison data for all available frameworks in PerfCompare (e.g., awsy, browsertime, build_metrics, devtools). I hope this helps clarify things.
However, it’s worth noting that previous attempts to implement this triggered performance issues due to the high volume of compare results being returned. I'm not certain whether our past performance issues on this have been resolved yet. It may be too early to tackle this now, so we'll probably need to consult Sparky before taking next steps.
There was a problem hiding this comment.
@gmierz when you get a chance can you chime in. thanks!
There was a problem hiding this comment.
We haven't resolved or looked into those issues. There's another issue here too which is that we're going to be mixing results from frameworks that have data which is useless to most devs (e.g. build metrics and mozharness).
I think we should land this change, but instead of having the framework be empty for all data, use all as the option so that we don't have people inadvertently querying for all the data if they omit the framework. Main reason for that is in case this causes too much load on our backend. With that, we can then test out what it looks like in PerfCompare but we shouldn't allow/expose to users there yet.
| query_set = PerformanceSignature.objects.filter(parent_signature_id=None).prefetch_related( | ||
| "performancealert" | ||
| ) | ||
| if framework_id is not None: | ||
| query_set = query_set.filter(framework_id=framework_id) | ||
| query_set = ( | ||
| PerformanceSignature.objects.prefetch_related("performancealert") | ||
| .filter(framework_id=framework_id, parent_signature_id=None) | ||
| .values("suite", "test") | ||
| query_set.values("suite", "test") | ||
| .annotate(repositories=GroupConcat("repository_id", distinct=True)) | ||
| .annotate(platforms=GroupConcat("platform_id", distinct=True)) | ||
| .annotate(total_alerts=Count("performancealert")) |
There was a problem hiding this comment.
So i don't see any tests related to this endpoint. Is it in use? When I tried writing a test I got some sql errors and it was because GroupConcat is a MySql feature not in postgres. is this dead code that should be removed then?
gopar
changed the title
| return Response(data=query_params.errors, status=HTTP_400_BAD_REQUEST) | ||
|
|
||
| framework_id = query_params.validated_data["framework"] | ||
| query_set = PerformanceSignature.objects.filter(parent_signature_id=None).prefetch_related( |
There was a problem hiding this comment.
To my knowledge, this endpoint (TestSuiteHealthViewSet) is not utilized in PerfCompare, so modifications here should not be necessary.
ec19781 to
f70ad16
Compare
Archaeopteryx
added
the
perfherder
4 participants
Bug: API - make framework parameter optional