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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: exception log message format #394

Merged
merged 13 commits into from Sep 14, 2021
Merged

fix: exception log message format #394

merged 13 commits into from Sep 14, 2021

Conversation

daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Sep 11, 2021

Previously, logging an exception with logger.exception would append traceback information as part of the log message, making it hard to read. This PR removes the extra exception info from the LogEntry

Fixes #382 馃

@daniel-sanche daniel-sanche requested review from as code owners Sep 11, 2021
@product-auto-label product-auto-label bot added the api: logging label Sep 11, 2021
@google-cla google-cla bot added the cla: yes label Sep 11, 2021
@daniel-sanche daniel-sanche changed the title fix: exception log message format [DRAFT] fix: exception log message format Sep 11, 2021
Copy link

@minherz minherz left a comment

From the change list it is hard to understand the justification for duplicating the record before formatting. Since the expected use of the record is for logging only, full duplication of the object seems unnecessary.

record = copy(record)
record.exc_info = None
Copy link

@minherz minherz Sep 12, 2021

Choose a reason for hiding this comment

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

since the record object is used for logging, what is a reason to create a full copy of the record object? why not to reset the .exc_info on the argument directly?
Otherwise, consider to use a distinct variable for the copy instead of "reusing" record.

Copy link
Contributor Author

@daniel-sanche daniel-sanche Sep 14, 2021

Choose a reason for hiding this comment

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

I was doing some experimenting, but ended up modifying the record object directly

@daniel-sanche daniel-sanche added the kokoro:force-run label Sep 14, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Sep 14, 2021
@daniel-sanche daniel-sanche changed the title [DRAFT] fix: exception log message format fix: exception log message format Sep 14, 2021
@daniel-sanche daniel-sanche merged commit c426bf5 into main Sep 14, 2021
18 of 19 checks passed
@daniel-sanche daniel-sanche deleted the parse-exceptions branch Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants