Handle processing_service_name parameters in requests from workers#1117
Handle processing_service_name parameters in requests from workers#1117carlosgjs wants to merge 8 commits intoRolnickLab:mainfrom
Conversation
👷 Deploy request for antenna-ssec pending review.Visit the deploys page to approve it
|
✅ Deploy Preview for antenna-preview canceled.
|
📝 WalkthroughWalkthroughAdds a new optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API (Django)
participant JobView as JobViewSet
participant Logger as Logger
Client->>API: GET /jobs?processing_service_name=worker-A
API->>JobView: dispatch request -> list/tasks/result
JobView->>JobView: extract "processing_service_name"
JobView->>Logger: _log_processing_service_name(context, "list/tasks/result")
Logger-->>JobView: acknowledged
JobView-->>API: return 200 response
API-->>Client: 200 OK (payload)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request adds logging capabilities to track which processing services are making requests to job-related API endpoints. The changes introduce an optional processing_service_name query parameter to the list, tasks, and result endpoints, which is logged when provided. The PR also consolidates OpenAPI parameter definitions by moving them from ami/utils/requests.py to ami/jobs/schemas.py.
Changes:
- Added
processing_service_namequery parameter to job list, tasks, and result endpoints - Created
_log_processing_service_name()helper function to log service names from requests - Removed unused OpenAPI parameter imports and definitions from
ami/utils/requests.py
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| ami/utils/requests.py | Removed unused OpenAPI imports and parameter definitions that were moved to ami/jobs/schemas.py |
| ami/jobs/schemas.py | Added processing_service_name_param OpenAPI parameter definition |
| ami/jobs/views.py | Imported new parameter, added logging calls in list/tasks/result endpoints, implemented _log_processing_service_name() helper function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| processing_service_name_param = OpenApiParameter( | ||
| name="processing_service_name", | ||
| description="Inform the name of the calling processing service", | ||
| required=False, | ||
| type=str, | ||
| ) |
There was a problem hiding this comment.
The new processing_service_name parameter lacks test coverage. Since the repository has comprehensive test coverage for job endpoints (see existing tests for tasks and result endpoints in ami/jobs/tests.py), tests should be added to verify that the parameter is correctly logged when provided and that the endpoints work correctly when the parameter is omitted.
| ] | ||
| ) | ||
| def list(self, request, *args, **kwargs): | ||
| _ = _log_processing_service_name(request, "list requested", logger) |
There was a problem hiding this comment.
The context string format is inconsistent across different endpoints. The "list" endpoint uses "list requested" without job information, while "tasks" uses "tasks ({batch}) requested for job {job.pk}" and "result" uses "result received for job {job.pk}". Consider standardizing the format to either include job information consistently or use a more descriptive context that produces clearer log messages. For example: "Job {job.pk}: {action}" format would be clearer.
| _ = _log_processing_service_name(request, "list requested", logger) | |
| _ = _log_processing_service_name(request, "Jobs: list requested", logger) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ami/jobs/views.py`:
- Around line 335-351: The function _log_processing_service_name can return None
when the "processing_service_name" query param is missing, so update its return
type annotation from -> str to include None (either -> str | None for Python
3.10+ or -> Optional[str] with "from typing import Optional" for older
versions); ensure you add the Optional import if used and run a quick type-check
to confirm no other callers require changes.
| return default_threshold | ||
|
|
||
|
|
||
| project_id_doc_param = OpenApiParameter( |
There was a problem hiding this comment.
Thanks for finding a good home for these parameters!
3 participants
Summary
This pull request introduces a new optional query parameter,
processing_service_name, to several job-related API endpoints. This parameter allows clients to specify the name of the processing service making the request, which is then logged for auditing and debugging purposes. The change also removes unused OpenAPI parameter definitions inami/utils/requests.py(previously moved toami/jobs/schemas.py).The parameter is optional for easier deployment of this and the corresponding change in the worker, see:
RolnickLab/ami-data-companion#108
Related Issues
Closes #1112
Support for #1087
Testing Logs
Logs:
Deployment Notes
Include instructions if this PR requires specific steps for its deployment (database migrations, config changes, etc.)
Checklist
Summary by CodeRabbit