Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework callback logging #2078

Merged
merged 35 commits into from
May 23, 2024
Merged
Changes from 29 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
8d4a8f8
Decoupled scan malware code + lowering retry period for high priority…
jimleroyer Nov 20, 2023
8b3ad4e
Extract common email retry handling logic into its own function
jimleroyer Nov 21, 2023
4b40f65
Cleaned up import
jimleroyer Nov 21, 2023
dab3a12
Forgot to provide default value to optional fn arg
jimleroyer Nov 21, 2023
5ec97ae
Fixed test import
jimleroyer Nov 21, 2023
a6a418a
Isolated retry task param builder in a class
jimleroyer Nov 22, 2023
fd12319
Merge remote-tracking branch 'origin/main' into feat/decrease-retry-f…
jimleroyer Nov 22, 2023
1cef267
Cleaned up import
jimleroyer Nov 22, 2023
f4a6470
Fixed moved refs
jimleroyer Nov 22, 2023
17482f5
Trying a different strategy to fix circular import
jimleroyer Nov 22, 2023
e124f21
Fixing another bad import ref
jimleroyer Nov 23, 2023
e5bfea8
Introducing celery utils module instead of using celery root one
jimleroyer Nov 23, 2023
c7f6057
Merge remote-tracking branch 'origin/main' into feat/decrease-retry-f…
jimleroyer Nov 23, 2023
ec41793
Cover edge cases + modified tests
jimleroyer Nov 24, 2023
e084c8f
Merge remote-tracking branch 'origin/main' into feat/decrease-retry-f…
jimleroyer Nov 24, 2023
5433518
Formatting
jimleroyer Nov 24, 2023
85f5140
Sort imports
jimleroyer Nov 24, 2023
9e4bfe6
Make notification_process_type param optional
jimleroyer Nov 24, 2023
d064161
Merge remote-tracking branch 'origin/main' into feat/decrease-retry-f…
jimleroyer Dec 7, 2023
c3e5ee6
Fixed edge case when template not associated with notification obj
jimleroyer Dec 7, 2023
3b33d29
Merge remote-tracking branch 'origin/main' into feat/decrease-retry-f…
jimleroyer Dec 11, 2023
bcd3f16
Fixing params order
jimleroyer Dec 11, 2023
4e236d4
Fixing regression tests
jimleroyer Dec 11, 2023
7264ae8
More tests
jimleroyer Dec 12, 2023
cc991f2
Added null protection against a potential NPE
jimleroyer Dec 12, 2023
5bf854b
Formatting
jimleroyer Dec 12, 2023
2ea5a4b
Fix imports
jimleroyer Dec 12, 2023
91738a7
Merge remote-tracking branch 'origin/main' into feat/decrease-retry-f…
jimleroyer Jan 16, 2024
6bcfc54
Moved logging for callback prior to actual call
jimleroyer Jan 16, 2024
612fb69
Merge branch 'main' into feat/rework-callback-logging
jimleroyer Feb 28, 2024
6fdcab8
Merge branch 'main' into feat/rework-callback-logging
jimleroyer Feb 28, 2024
23aecfd
Merge branch 'main' into feat/rework-callback-logging
jimleroyer Mar 12, 2024
c0aab71
Merge branch 'main' into feat/rework-callback-logging
jimleroyer May 22, 2024
1b9dc88
Format service_callback_tasks.py
jimleroyer May 23, 2024
9211ccd
Merge remote-tracking branch 'origin/main' into feat/rework-callback-…
jimleroyer May 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions app/celery/service_callback_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ def send_complaint_to_service(self, complaint_data):
def _send_data_to_service_callback_api(self, data, service_callback_url, token, function_name):
notification_id = data["notification_id"] if "notification_id" in data else data["id"]
try:
current_app.logger.info(
"{} sending {} to {}".format(
function_name,
notification_id,
service_callback_url
)
)
response = request(
method="POST",
url=service_callback_url,
Expand All @@ -69,14 +76,6 @@ def _send_data_to_service_callback_api(self, data, service_callback_url, token,
},
timeout=60,
)
current_app.logger.info(
"{} sending {} to {}, response {}".format(
function_name,
notification_id,
service_callback_url,
response.status_code,
)
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to log the response here if there's an error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @sastels for the long delayed response. I am revisiting this. From what I read here, an error seems to raise a RequestException and would then be logged as a warning, hence covering the response status. Most likely the status code would be contained within the e itself I assume. Do you think my read of the code makes sense here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's a good point 💯

response.raise_for_status()
except RequestException as e:
current_app.logger.warning(
Expand Down
Loading