Navigation Menu

Skip to content

Commit

Permalink
fix(logging): do not panic in library code (#3076)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
0xSage committed Oct 29, 2020
1 parent e760c85 commit 529be97
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 19 deletions.
33 changes: 15 additions & 18 deletions logging/logging.go
Expand Up @@ -47,7 +47,6 @@ import (
"github.com/golang/protobuf/proto"
"github.com/golang/protobuf/ptypes"
structpb "github.com/golang/protobuf/ptypes/struct"
tspb "github.com/golang/protobuf/ptypes/timestamp"
"google.golang.org/api/option"
"google.golang.org/api/support/bundler"
mrpb "google.golang.org/genproto/googleapis/api/monitoredres"
Expand Down Expand Up @@ -174,26 +173,20 @@ func NewClient(ctx context.Context, parent string, opts ...option.ClientOption)
return client, nil
}

var unixZeroTimestamp *tspb.Timestamp

func init() {
var err error
unixZeroTimestamp, err = ptypes.TimestampProto(time.Unix(0, 0))
if err != nil {
panic(err)
}
}

// Ping reports whether the client's connection to the logging service and the
// authentication configuration are valid. To accomplish this, Ping writes a
// log entry "ping" to a log named "ping".
func (c *Client) Ping(ctx context.Context) error {
unixZeroTimestamp, err := ptypes.TimestampProto(time.Unix(0, 0))
if err != nil {
return err
}
ent := &logpb.LogEntry{
Payload: &logpb.LogEntry_TextPayload{TextPayload: "ping"},
Timestamp: unixZeroTimestamp, // Identical timestamps and insert IDs are both
InsertId: "ping", // necessary for the service to dedup these entries.
}
_, err := c.client.WriteLogEntries(ctx, &logpb.WriteLogEntriesRequest{
_, err = c.client.WriteLogEntries(ctx, &logpb.WriteLogEntriesRequest{
LogName: internal.LogPath(c.parent, "ping"),
Resource: monitoredResource(c.parent),
Entries: []*logpb.LogEntry{ent},
Expand Down Expand Up @@ -695,12 +688,12 @@ type HTTPRequest struct {
CacheLookup bool
}

func fromHTTPRequest(r *HTTPRequest) *logtypepb.HttpRequest {
func fromHTTPRequest(r *HTTPRequest) (*logtypepb.HttpRequest, error) {
if r == nil {
return nil
return nil, nil
}
if r.Request == nil {
panic("HTTPRequest must have a non-nil Request")
return nil, errors.New("logging: HTTPRequest must have a non-nil Request")
}
u := *r.Request.URL
u.Fragment = ""
Expand All @@ -723,7 +716,7 @@ func fromHTTPRequest(r *HTTPRequest) *logtypepb.HttpRequest {
if r.Latency != 0 {
pb.Latency = ptypes.DurationProto(r.Latency)
}
return pb
return pb, nil
}

// fixUTF8 is a helper that fixes an invalid UTF-8 string by replacing
Expand Down Expand Up @@ -803,7 +796,7 @@ func jsonValueToStructValue(v interface{}) *structpb.Value {
}
return &structpb.Value{Kind: &structpb.Value_ListValue{ListValue: &structpb.ListValue{Values: vals}}}
default:
panic(fmt.Sprintf("bad type %T for JSON value", v))
return &structpb.Value{Kind: &structpb.Value_NullValue{}}
}
}

Expand Down Expand Up @@ -933,11 +926,15 @@ func (l *Logger) toLogEntry(e Entry) (*logpb.LogEntry, error) {
e.TraceSampled = e.TraceSampled || traceSampled
}
}
req, err := fromHTTPRequest(e.HTTPRequest)
if err != nil {
l.client.error(err)
}
ent := &logpb.LogEntry{
Timestamp: ts,
Severity: logtypepb.LogSeverity(e.Severity),
InsertId: e.InsertID,
HttpRequest: fromHTTPRequest(e.HTTPRequest),
HttpRequest: req,
Operation: e.Operation,
Labels: e.Labels,
Trace: e.Trace,
Expand Down
14 changes: 13 additions & 1 deletion logging/logging_unexported_test.go
Expand Up @@ -400,7 +400,10 @@ func TestFromHTTPRequest(t *testing.T) {
CacheHit: true,
CacheValidatedWithOriginServer: true,
}
got := fromHTTPRequest(req)
got, err := fromHTTPRequest(req)
if err != nil {
t.Errorf("got %v", err)
}
want := &logtypepb.HttpRequest{
RequestMethod: "GET",

Expand Down Expand Up @@ -429,6 +432,15 @@ func TestFromHTTPRequest(t *testing.T) {
if _, err := proto.Marshal(got); err != nil {
t.Fatalf("Unexpected proto.Marshal error: %v", err)
}

// fromHTTPRequest returns nil if there is no Request property (but does not panic)
reqNil := &HTTPRequest{
RequestSize: 100,
}
got, err = fromHTTPRequest(reqNil)
if got != nil && err == nil {
t.Errorf("got %+v\nwant %+v", got, want)
}
}

func TestMonitoredResource(t *testing.T) {
Expand Down

0 comments on commit 529be97

Please sign in to comment.