Skip to content

DM-47333: Logger.exception() missing 1 required positional argument #213

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

Merged
merged 1 commit into from
Nov 1, 2024
+4 −4
Merged

DM-47333: Logger.exception() missing 1 required positional argument #213

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions python/activator/activator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,9 +347,9 @@ def next_visit_handler() -> tuple[str, int]:
try:
try:
expected_visit = parse_next_visit(flask.request)
except ValueError as msg:
_log.exception()
return f"Bad Request: {msg}", 400
except ValueError as e:
_log.exception("Bad Request: %s", e)
Copy link
Member

@kfindeisen kfindeisen Nov 4, 2024

Choose a reason for hiding this comment

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

Sorry, but this seems incorrect to me -- Logger.exception normally does not need to take the exception object, because that's pulled automatically from the handling context. Doesn't this lead to duplicate exception messages?

Copy link
Collaborator Author

@hsinfang hsinfang Nov 4, 2024

Choose a reason for hiding this comment

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

But it does require an argument. Or do you mean removing this line here and just letting error handler handle later?

Copy link
Member

@timj timj Nov 4, 2024

Choose a reason for hiding this comment

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

No, he's saying:

_log.exception("Bad request")

instead.

For example if you include the exception string:

WARNING:test:Message one
ERROR:test:Bad code: Message 2
Traceback (most recent call last):
  File "/Users/timj/work/lsstsw/build/daf_butler/y.py", line 9, in <module>
    raise RuntimeError("Message 2")
RuntimeError: Message 2
Finish

so you get "Message 2" in the first line of the log output and the final line of the output.

Copy link
Collaborator Author

@hsinfang hsinfang Nov 4, 2024

Choose a reason for hiding this comment

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

I see. Thank you for explaining.
New PR: #214
Would one of you review please?

return f"Bad Request: {e}", 400
process_visit(expected_visit)
return "Pipeline executed", 200
except GracefulShutdownInterrupt:
Expand Down Expand Up @@ -524,7 +524,7 @@ def process_visit(expected_visit: FannedOutVisit):


def invalid_visit(e: InvalidVisitError) -> tuple[str, int]:
_log.exception()
_log.exception("Invalid visit: %s", e)
return f"Cannot process visit: {e}.", 422


Expand Down
Loading