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

Jump to bottom

logging: library code should not crash #1862

Closed
ghasemloo opened this issue Mar 18, 2020 · 4 comments · Fixed by #3076
Closed

logging: library code should not crash #1862

ghasemloo opened this issue Mar 18, 2020 · 4 comments · Fixed by #3076
Assignees
@0xSage
Labels
api: logging Issues related to the Cloud Logging API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@ghasemloo
Copy link

ghasemloo commented Mar 18, 2020 •
edited

Client

Stackdriver Logging

Environment

all

Code

replace panics

0d861fa

Expected behavior

library code should not crash (except at startup)

Actual behavior

library code crashes when request is nil
@ghasemloo ghasemloo added the triage me I really want to be triaged. label Mar 18, 2020
@ghasemloo ghasemloo changed the title logging: library code should not clash logging: library code should not crash Mar 18, 2020
@codyoss codyoss added api: logging Issues related to the Cloud Logging API. type: question Request for information or clarification. Not an issue. and removed triage me I really want to be triaged. labels Mar 20, 2020
@codyoss
Copy link
Member

codyoss commented Mar 20, 2020

From looking at both of these they seem like actual valid reasons to panic. I could be missing something though. Did you trigger these panics while your code was running?

@codyoss codyoss added the needs more info This issue needs more information from the customer to proceed. label Mar 23, 2020
@codyoss
Copy link
Member

codyoss commented Apr 3, 2020

Closing due to lack of response. Please reopen if you have more information to share.

@codyoss codyoss closed this as completed Apr 3, 2020
@ghasemloo
Copy link
Author

Yes, it did, the request was nil I think.

Based on Google's internal go style library code should not panic: go/gocomments#dont-panic
Filed here instead of internally since this seems to be the source of truth for the code.

@ghasemloo
Copy link
Author

@jba fyi

@codyoss codyoss reopened this Apr 14, 2020
@codyoss codyoss removed the needs more info This issue needs more information from the customer to proceed. label Apr 28, 2020
@codyoss codyoss self-assigned this May 12, 2020
@codyoss codyoss added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: question Request for information or clarification. Not an issue. labels May 12, 2020
@codyoss codyoss assigned simonz130 and unassigned codyoss Sep 29, 2020
@0xSage 0xSage assigned 0xSage and unassigned simonz130 Oct 9, 2020
0xSage pushed a commit that referenced this issue Oct 23, 2020
fix(logging): do not panic in library code (#1862)
04b40d4
@0xSage 0xSage mentioned this issue Oct 23, 2020
Merged
gcf-merge-on-green bot pushed a commit that referenced this issue Oct 29, 2020
@0xSage
fix(logging): do not panic in library code (#3076)
529be97 
fixes: #1862 

Changes: 
- No longer panic on User introduced errors. Instead we log the error and let program proceed
- We log.Fatalf("ptypes.TimestampProto(time.Unix(0,0)) failed: %v", err) to exit program, since it's a fatal error/likely not induced by Users
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@0xSage 0xSage

Labels
api: logging Issues related to the Cloud Logging API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(logging): do not panic in library code googleapis/google-cloud-go
4 participants
@codyoss @ghasemloo @simonz130 @0xSage