From eba34f5d125052c57af720f66f9c288484adcc95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Fri, 16 Aug 2024 08:48:49 +0100 Subject: [PATCH] Bug 1910864/1887138 - Fix how we handle profiler urls on the perfherder backend (#8156) * Bug 1910864 - Return None for profiler urls instead of "N/A" Perfherder frontend has a check if the profiler urls are null, but it doesn't check if it's "N/A" or not. That's why this was causing us to see all the invalid links in the perfherder when user clicks on "Copy Summary" or file a bug directly through perfherder. By changing this to `None`, this API endpoint will return `null` to the frontend, which can successfully understand and omit. * Bug 1887138 - Refactor how we compute the profile urls on the backend There were two issues with how we compute the profile urls in the backend: 1. When we encounter an error, we were returning an error string. This is not good because frontend doesn't know how to handle the error strigs and it thinks that they are simply urls. Then it was trying to append the error strings on the back of profiler urls, which was causing invalid urls in the perf alerts. 2. We had a caching mechanism for the taskcluster metadata. But this caching mechanism wasn't really caching things by adding task id to the cache key. This was resulting a single cache for the whole "push" or multiple pushes. We were caching the latest metadata, and use that last metadata for all the alerts, which is wrong because all individual alerts might have differnet metadata. The good thing is, the taskcluseter metadata was already available to us inside the `alert` variable since we computed it prior this function. So we didn't have to do anything special, just read that from the dictionary. There is one remaining piece I would like to fix still, which is how we cache `tc_root_url`. But that's actually okay for now because a single push should only have one `tc_root_url`. And I'm not so sure how to pass that information to the other function where we genereate the profile urls. --- treeherder/webapp/api/performance_data.py | 13 ++----------- treeherder/webapp/api/performance_serializers.py | 5 ++--- treeherder/webapp/api/utils.py | 14 ++++++++++---- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/treeherder/webapp/api/performance_data.py b/treeherder/webapp/api/performance_data.py index 60b4cc620..8e1b682de 100644 --- a/treeherder/webapp/api/performance_data.py +++ b/treeherder/webapp/api/performance_data.py @@ -5,7 +5,6 @@ from urllib.parse import urlencode import django_filters from django.conf import settings -from django.core.cache import cache from django.db import transaction from django.db.models import CharField, Count, Q, Subquery, Value, Case, When from django.db.models.functions import Concat @@ -483,19 +482,11 @@ class PerformanceAlertSummaryViewSet(viewsets.ModelViewSet): if summary["id"] == int(pk): for alert in summary["alerts"]: if alert["is_regression"]: - taskcluster_metadata = ( - cache.get("task_metadata") if cache.get("task_metadata") else {} - ) alert["profile_url"] = get_profile_artifact_url( - alert, taskcluster_metadata - ) - prev_taskcluster_metadata = ( - cache.get("prev_task_metadata") - if cache.get("prev_task_metadata") - else {} + alert, metadata_key="taskcluster_metadata" ) alert["prev_profile_url"] = get_profile_artifact_url( - alert, prev_taskcluster_metadata + alert, metadata_key="prev_taskcluster_metadata" ) return self.get_paginated_response(serializer.data) diff --git a/treeherder/webapp/api/performance_serializers.py b/treeherder/webapp/api/performance_serializers.py index fdbfabd1c..322979b81 100644 --- a/treeherder/webapp/api/performance_serializers.py +++ b/treeherder/webapp/api/performance_serializers.py @@ -208,7 +208,6 @@ class PerformanceAlertSerializer(serializers.ModelSerializer): try: taskcluster_metadata = get_tc_metadata(alert, alert.summary.push) cache.set("tc_root_url", alert.summary.repository.tc_root_url, FIVE_DAYS) - cache.set("task_metadata", taskcluster_metadata, FIVE_DAYS) return taskcluster_metadata except ObjectDoesNotExist: return {} @@ -222,10 +221,10 @@ class PerformanceAlertSerializer(serializers.ModelSerializer): return {} def get_profile_url(self, alert): - return "N/A" + return None def get_prev_profile_url(self, alert): - return "N/A" + return None def get_classifier_email(self, performance_alert): return getattr(performance_alert.classifier, "email", None) diff --git a/treeherder/webapp/api/utils.py b/treeherder/webapp/api/utils.py index e0a615b94..ee0b6ed39 100644 --- a/treeherder/webapp/api/utils.py +++ b/treeherder/webapp/api/utils.py @@ -68,14 +68,19 @@ def get_artifact_list(root_url, task_id): return artifacts.get("artifacts", []) -def get_profile_artifact_url(alert, task_metadata): +def get_profile_artifact_url(alert, metadata_key): tc_root_url = cache.get("tc_root_url", "") - # Return a string to tell that task_id wasn't found + # Get the taskcluster metadata we'll use. It's determined by the caller. + task_metadata = alert[metadata_key] + + # Return None if task_id wasn't found if not task_metadata.get("task_id") or not tc_root_url: - return "task_id not found" + return None + # If the url was already cached, don't calculate again, just return it if cache.get(task_metadata.get("task_id")): return cache.get(task_metadata.get("task_id")) + artifacts_json = get_artifact_list(tc_root_url, task_metadata.get("task_id")) profile_artifact = [ artifact @@ -86,7 +91,8 @@ def get_profile_artifact_url(alert, task_metadata): ] if not profile_artifact: - return "Artifact not available" + return None + task_url = f"{tc_root_url}/api/queue/v1/task/{task_metadata['task_id']}" # There's only one profile relevant for performance per task artifact_url = (