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

Jump to bottom

Adds DPanic and Panic level logging (writes log, then panics) #3689

Merged
merged 3 commits into from
Dec 9, 2022
Merged

Adds DPanic and Panic level logging (writes log, then panics) #3689

merged 3 commits into from
Dec 9, 2022

Conversation

paulnpdev
Copy link
Member

@paulnpdev paulnpdev commented Dec 4, 2022 •
edited by alexshtin
Loading

What changed?
New methods on logger to allow panics to create consistent logger entries.

Why?
Tired of panic strings that don't have enough information.

How did you test it?
New tests on logger.

Potential risks
Since no code is yet using these new methods, risk should be minimal.

Is hotfix candidate?
No.

@paulnpdev paulnpdev requested a review from a team as a code owner December 4, 2022 20:46
@paulnpdev paulnpdev enabled auto-merge (squash) December 4, 2022 20:46
yux0
yux0 reviewed Dec 6, 2022
View reviewed changes
alexshtin
alexshtin approved these changes Dec 7, 2022
View reviewed changes
Copy link
Member

@alexshtin alexshtin left a comment

Choose a reason for hiding this comment

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

Tired of panic strings that don't have enough information.

you mean, log strings, that don't have correct call stack and with DPanic we will get the right call stack to the actual error.

@alexshtin alexshtin added the release/1.20.0 PR must be highlighted in release notes label Dec 7, 2022
@paulnpdev
Copy link
Member Author

Tired of panic strings that don't have enough information.

you mean, log strings, that don't have correct call stack and with DPanic we will get the right call stack to the actual error.

both

@paulnpdev paulnpdev disabled auto-merge December 9, 2022 20:52
@paulnpdev paulnpdev merged commit 4c4ef46 into master Dec 9, 2022
@paulnpdev paulnpdev deleted the logger-panic branch December 9, 2022 20:52
pull bot pushed a commit to forkmeplease/temporal that referenced this pull request Dec 9, 2022
@paulnpdev
Adds DPanic and Panic level logging (writes log, then panics) (tempor…
55cdd69 
…alio#3689)
pull bot pushed a commit to forkmeplease/temporal that referenced this pull request Dec 9, 2022
@paulnpdev
Adds DPanic and Panic level logging (writes log, then panics) (tempor…
ad1176c 
…alio#3689)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexshtin alexshtin alexshtin approved these changes

@yux0 yux0 yux0 approved these changes

Assignees
No one assigned
Labels
release/1.20.0 PR must be highlighted in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paulnpdev @alexshtin @yux0