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.

Sign up for GitHub

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

[bugfix] Improve handling of requests in httpjson logger #3354

Closed
wants to merge 3 commits into from
Closed

[bugfix] Improve handling of requests in httpjson logger #3354

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6028f1c
Add simple error checking in httpjson
ekouts Dec 20, 2024
120cf87
Handle 429 in httpjson logger
ekouts Dec 20, 2024
a28d273
Fix import
ekouts Dec 20, 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
21 changes: 16 additions & 5 deletions reframe/core/logging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,7 +680,6 @@ def emit(self, record):
return

if self._debug:
import time
ts = int(time.time() * 1_000)
dump_file = f'httpjson_record_{ts}.json'
with open(dump_file, 'w') as fp:
Expand All @@ -689,10 +688,22 @@ def emit(self, record):
return

try:
requests.post(
self._url, data=json_record,
headers=self._headers
)
while True:
response = requests.post(
self._url, data=json_record,
headers=self._headers
)
if response.status_code == 200:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should go for the more inclusive:

Suggested change
if response.status_code == 200:
if response.ok:

break

if response.status_code == 429:
time.sleep(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this can slow down the pipeline what about spreading the wait with a cyclic interval list, say [.1, .2, .4, .8, 1.6, 3.2]?

continue

raise LoggingError(
f'logging failed: HTTP response code '
f'{response.status_code}'
)
Comment on lines +703 to +706
Copy link
Contributor Author

@ekouts ekouts Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should Reframe crash in this case, or try to print a warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

How does the outpupt look like when we raise LoggingError? I see that currently we do raise it, but I'm not sure it's the right thing. I would rather go for a warning instead (remember to include the name of the handler in the message).

except requests.exceptions.RequestException as e:
raise LoggingError('logging failed') from e

Expand Down
Loading