feat(tempest): Add cron job and tasks #82454

tobias-wilfert · 2024-12-20T13:31:01Z

Fixes: https://github.com/getsentry/team-gdx/issues/31

codecov · 2024-12-20T13:59:51Z

Codecov Report

Attention: Patch coverage is 88.57143% with 8 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/tempest/tasks.py	87.69%	8 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #82454       +/-   ##
===========================================
+ Coverage   59.35%   80.51%   +21.16%     
===========================================
  Files        7015     7323      +308     
  Lines      306414   322093    +15679     
  Branches    21016    21016               
===========================================
+ Hits       181857   259343    +77486     
+ Misses     124151    62344    -61807     
  Partials      406      406

src/sentry/conf/server.py

vgrozdanic · 2024-12-23T09:18:33Z

src/sentry/models/projectkey.py

@@ -72,6 +72,8 @@ class UseCase(enum.Enum):
    PROFILING = "profiling"
    """ An internal project key for submitting escalating issues metrics."""
    ESCALATING_ISSUES = "escalating_issues"
+    """ An internal project key for submitting events from tempest."""
+    TEMPEST = "tempest"


you will need to create a django migration for this: https://develop.sentry.dev/backend/application-domains/database-migrations/

Django saves all these changes in migrations, although this will be no-op SQL. I suggest doing this in a separate PR, which will only contain this change and a migrations files created by django, as it will be easier to review it for owners-migrations team.

Do you mean just put the Migration in a separate PR or also this change itself?

So tried to make a migration for this with @shellmayr and got No changes detected ... one can make an empty migration but not sure what the point of that would be?

vgrozdanic · 2024-12-23T09:21:02Z

src/sentry/tempest/endpoints/tempest_credentials.py

-            serializer.save(created_by_id=request.user.id, project=project)
+            credentials = serializer.save(created_by_id=request.user.id, project=project)
+            # Make initial call to determine the latest item ID
+            fetch_latest_item_id.delay(credentials.id)


how exepensive is fetching of the latest item? It might happen that we manually call this task, and in the next few milliseconds periodic task collects this credentials, see that there is no last item and calls this task again.

Will we have problem if that happens? Or is it ok if we fetch that twice in a short period of time?

Fetching this twice in a short period of time should be fine.

If this turns out to unexpectedly cause issues we can always remove this and just defer the work to the actual task (than they would just need to do this on the first iteration).

src/sentry/tempest/tasks.py

vgrozdanic · 2024-12-23T09:31:21Z

src/sentry/tempest/tasks.py

+            dsn=ProjectKey.objects.get_or_create(
+                use_case=UseCase.TEMPEST, project=credentials.project
+            )[
+                0
+            ].get_dsn(),  # This does generate a dsn not sure if it will work in the grand scheme of things though.


nit: for a better code readability, i would extract this into a variable outside of this function call, and then just pass dsn as the variable. It will make function call and all the args easier to read

Also, how and when are we going to create this ProjectKey for Tempest related things?

Not sure what you mean with the second comment.

src/sentry/tempest/tasks.py

vgrozdanic · 2024-12-23T09:55:50Z

tests/sentry/tempest/test_tempest.py

+
+    @patch("sentry.tempest.tasks.fetch_latest_id_from_tempest")
+    def test_fetch_latest_item_id_task(self, mock_fetch):
+        mock_fetch.return_value = "20001"


Should this return a json object:

{ "latest_id": "20001" }

Inside fetch_latest_item_id we access this field?

Currently it just returns the number (no json) but that might be something we want to change 🤔

vgrozdanic · 2024-12-23T09:58:16Z

tests/sentry/tempest/test_tempest.py

+
+    @patch("sentry.tempest.tasks.fetch_latest_id_from_tempest")
+    def test_fetch_latest_item_id_error(self, mock_fetch):
+        mock_fetch.return_value = "Invalid credentials"


I don't know how response from tempest look like, but maybe it's better if we always return JSON objects, with key "error" if something bad happened:

{ "error": "Invalid credentials" }

That way we can always expect JSON response and just read the keys we need

Yeah that might be nicer.

src/sentry/tempest/tasks.py

tests/sentry/tempest/test_tempest.py

Add tempest cron job and tasks

e2e4f71

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 20, 2024

vercel bot deployed to Preview December 20, 2024 13:32 View deployment

vgrozdanic reviewed Dec 20, 2024

View reviewed changes

src/sentry/conf/server.py Show resolved Hide resolved