From 71828c28d424c34da6d0392651739a364cd57e79 Mon Sep 17 00:00:00 2001 From: Marian Lobur Date: Fri, 9 Apr 2021 18:39:12 +0200 Subject: [PATCH] feat(logging): make toLogEntry function public (#3863) This will allows clients of the library to easily migrate from using Logger abstraction to calling WriteLogEntries directly. --- logging/examples_test.go | 23 ++++ logging/logging.go | 44 +++++-- logging/logging_test.go | 181 +++++++++++++++++++++++++++++ logging/logging_unexported_test.go | 151 +----------------------- 4 files changed, 241 insertions(+), 158 deletions(-) diff --git a/logging/examples_test.go b/logging/examples_test.go index 531c48b8d34..db5d5c57763 100644 --- a/logging/examples_test.go +++ b/logging/examples_test.go @@ -22,7 +22,9 @@ import ( "os" "cloud.google.com/go/logging" + vkit "cloud.google.com/go/logging/apiv2" "go.opencensus.io/trace" + logpb "google.golang.org/genproto/googleapis/logging/v2" ) func ExampleNewClient() { @@ -106,6 +108,27 @@ func ExampleHTTPRequest() { lg.Log(httpEntry) } +func ExampleToLogEntry() { + e := logging.Entry{ + Payload: "Message", + } + le, err := logging.ToLogEntry(e, "my-project") + if err != nil { + // TODO: Handle error. + } + client, err := vkit.NewClient(context.Background()) + if err != nil { + // TODO: Handle error. + } + _, err = client.WriteLogEntries(context.Background(), &logpb.WriteLogEntriesRequest{ + Entries: []*logpb.LogEntry{le}, + LogName: "stdout", + }) + if err != nil { + // TODO: Handle error. + } +} + func ExampleLogger_LogSync() { ctx := context.Background() client, err := logging.NewClient(ctx, "my-project") diff --git a/logging/logging.go b/logging/logging.go index b845561cf24..da097a7bbfb 100644 --- a/logging/logging.go +++ b/logging/logging.go @@ -137,9 +137,7 @@ type Client struct { // By default NewClient uses WriteScope. To use a different scope, call // NewClient using a WithScopes option (see https://godoc.org/google.golang.org/api/option#WithScopes). func NewClient(ctx context.Context, parent string, opts ...option.ClientOption) (*Client, error) { - if !strings.ContainsRune(parent, '/') { - parent = "projects/" + parent - } + parent = makeParent(parent) opts = append([]option.ClientOption{ option.WithScopes(WriteScope), }, opts...) @@ -172,6 +170,13 @@ func NewClient(ctx context.Context, parent string, opts ...option.ClientOption) return client, nil } +func makeParent(parent string) string { + if !strings.ContainsRune(parent, '/') { + return "projects/" + parent + } + return parent +} + // 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". @@ -803,7 +808,7 @@ func jsonValueToStructValue(v interface{}) *structpb.Value { // and will block, it is intended primarily for debugging or critical errors. // Prefer Log for most uses. func (l *Logger) LogSync(ctx context.Context, e Entry) error { - ent, err := l.toLogEntry(e) + ent, err := toLogEntryInternal(e, l.client, l.client.parent) if err != nil { return err } @@ -818,7 +823,7 @@ func (l *Logger) LogSync(ctx context.Context, e Entry) error { // Log buffers the Entry for output to the logging service. It never blocks. func (l *Logger) Log(e Entry) { - ent, err := l.toLogEntry(e) + ent, err := toLogEntryInternal(e, l.client, l.client.parent) if err != nil { l.client.error(err) return @@ -894,7 +899,26 @@ func deconstructXCloudTraceContext(s string) (traceID, spanID string, traceSampl return } -func (l *Logger) toLogEntry(e Entry) (*logpb.LogEntry, error) { +// ToLogEntry takes an Entry structure and converts it to the LogEntry proto. +// A parent can take any of the following forms: +// projects/PROJECT_ID +// folders/FOLDER_ID +// billingAccounts/ACCOUNT_ID +// organizations/ORG_ID +// for backwards compatibility, a string with no '/' is also allowed and is interpreted +// as a project ID. +// +// ToLogEntry is implied when users invoke Logger.Log or Logger.LogSync, +// but its exported as a pub function here to give users additional flexibility +// when using the library. Don't call this method manually if Logger.Log or +// Logger.LogSync are used, it is intended to be used together with direct call +// to WriteLogEntries method. +func ToLogEntry(e Entry, parent string) (*logpb.LogEntry, error) { + // We have this method to support logging agents that need a bigger flexibility. + return toLogEntryInternal(e, nil, makeParent(parent)) +} + +func toLogEntryInternal(e Entry, client *Client, parent string) (*logpb.LogEntry, error) { if e.LogName != "" { return nil, errors.New("logging: Entry.LogName should be not be set when writing") } @@ -913,7 +937,7 @@ func (l *Logger) toLogEntry(e Entry) (*logpb.LogEntry, error) { // https://cloud.google.com/appengine/docs/flexible/go/writing-application-logs. traceID, spanID, traceSampled := deconstructXCloudTraceContext(traceHeader) if traceID != "" { - e.Trace = fmt.Sprintf("%s/traces/%s", l.client.parent, traceID) + e.Trace = fmt.Sprintf("%s/traces/%s", parent, traceID) } if e.SpanID == "" { e.SpanID = spanID @@ -927,7 +951,11 @@ func (l *Logger) toLogEntry(e Entry) (*logpb.LogEntry, error) { } req, err := fromHTTPRequest(e.HTTPRequest) if err != nil { - l.client.error(err) + if client != nil { + client.error(err) + } else { + return nil, err + } } ent := &logpb.LogEntry{ Timestamp: ts, diff --git a/logging/logging_test.go b/logging/logging_test.go index f4386bedddd..495303ed94a 100644 --- a/logging/logging_test.go +++ b/logging/logging_test.go @@ -19,10 +19,13 @@ package logging_test import ( "context" "encoding/json" + "errors" "flag" "fmt" "log" "math/rand" + "net/http" + "net/url" "os" "strings" "sync" @@ -41,6 +44,7 @@ import ( "google.golang.org/api/iterator" "google.golang.org/api/option" mrpb "google.golang.org/genproto/googleapis/api/monitoredres" + logpb "google.golang.org/genproto/googleapis/logging/v2" "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -247,6 +251,183 @@ func TestContextFunc(t *testing.T) { } } +func TestToLogEntry(t *testing.T) { + u := &url.URL{Scheme: "http"} + tests := []struct { + name string + in logging.Entry + want logpb.LogEntry + wantError error + }{ + { + name: "BlankLogEntry", + in: logging.Entry{}, + want: logpb.LogEntry{}, + }, { + name: "Already set Trace", + in: logging.Entry{Trace: "t1"}, + want: logpb.LogEntry{Trace: "t1"}, + }, { + name: "No X-Trace-Context header", + in: logging.Entry{ + HTTPRequest: &logging.HTTPRequest{ + Request: &http.Request{URL: u, Header: http.Header{"foo": {"bar"}}}, + }, + }, + want: logpb.LogEntry{}, + }, { + name: "X-Trace-Context header with all fields", + in: logging.Entry{ + TraceSampled: false, + HTTPRequest: &logging.HTTPRequest{ + Request: &http.Request{ + URL: u, + Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/000000000000004a;o=1"}}, + }, + }, + }, + want: logpb.LogEntry{ + Trace: "projects/P/traces/105445aa7843bc8bf206b120001000", + SpanId: "000000000000004a", + TraceSampled: true, + }, + }, { + name: "X-Trace-Context header with all fields; TraceSampled explicitly set", + in: logging.Entry{ + TraceSampled: true, + HTTPRequest: &logging.HTTPRequest{ + Request: &http.Request{ + URL: u, + Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/000000000000004a;o=0"}}, + }, + }, + }, + want: logpb.LogEntry{ + Trace: "projects/P/traces/105445aa7843bc8bf206b120001000", + SpanId: "000000000000004a", + TraceSampled: true, + }, + }, { + name: "X-Trace-Context header with all fields; TraceSampled from Header", + in: logging.Entry{ + HTTPRequest: &logging.HTTPRequest{ + Request: &http.Request{ + URL: u, + Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/000000000000004a;o=1"}}, + }, + }, + }, + want: logpb.LogEntry{ + Trace: "projects/P/traces/105445aa7843bc8bf206b120001000", + SpanId: "000000000000004a", + TraceSampled: true, + }, + }, { + name: "X-Trace-Context header with blank trace", + in: logging.Entry{ + HTTPRequest: &logging.HTTPRequest{ + Request: &http.Request{ + URL: u, + Header: http.Header{"X-Cloud-Trace-Context": {"/0;o=1"}}, + }, + }, + }, + want: logpb.LogEntry{ + TraceSampled: true, + }, + }, { + name: "X-Trace-Context header with blank span", + in: logging.Entry{ + HTTPRequest: &logging.HTTPRequest{ + Request: &http.Request{ + URL: u, + Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/;o=0"}}, + }, + }, + }, + want: logpb.LogEntry{ + Trace: "projects/P/traces/105445aa7843bc8bf206b120001000", + }, + }, { + name: "X-Trace-Context header with missing traceSampled aka ?o=*", + in: logging.Entry{ + HTTPRequest: &logging.HTTPRequest{ + Request: &http.Request{ + URL: u, + Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/0"}}, + }, + }, + }, + want: logpb.LogEntry{ + Trace: "projects/P/traces/105445aa7843bc8bf206b120001000", + }, + }, { + name: "X-Trace-Context header with all blank fields", + in: logging.Entry{ + HTTPRequest: &logging.HTTPRequest{ + Request: &http.Request{ + URL: u, + Header: http.Header{"X-Cloud-Trace-Context": {""}}, + }, + }, + }, + want: logpb.LogEntry{}, + }, { + name: "Invalid X-Trace-Context header but already set TraceID", + in: logging.Entry{ + HTTPRequest: &logging.HTTPRequest{ + Request: &http.Request{ + URL: u, + Header: http.Header{"X-Cloud-Trace-Context": {"t3"}}, + }, + }, + Trace: "t4", + }, + want: logpb.LogEntry{ + Trace: "t4", + }, + }, { + name: "Already set TraceID and SpanID", + in: logging.Entry{Trace: "t1", SpanID: "007"}, + want: logpb.LogEntry{ + Trace: "t1", + SpanId: "007", + }, + }, { + name: "Empty request produces an error", + in: logging.Entry{ + HTTPRequest: &logging.HTTPRequest{ + RequestSize: 128, + }, + }, + wantError: errors.New("logging: HTTPRequest must have a non-nil Request"), + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + e, err := logging.ToLogEntry(test.in, "projects/P") + if err != nil && test.wantError == nil { + t.Fatalf("Unexpected error: %+v: %v", test.in, err) + } + if err == nil && test.wantError != nil { + t.Fatalf("Error is expected: %+v: %v", test.in, test.wantError) + } + if test.wantError != nil { + return + } + if got := e.Trace; got != test.want.Trace { + t.Errorf("TraceId: %+v: got %q, want %q", test.in, got, test.want.Trace) + } + if got := e.SpanId; got != test.want.SpanId { + t.Errorf("SpanId: %+v: got %q, want %q", test.in, got, test.want.SpanId) + } + if got := e.TraceSampled; got != test.want.TraceSampled { + t.Errorf("TraceSampled: %+v: got %t, want %t", test.in, got, test.want.TraceSampled) + } + }) + } +} + // compareEntries compares most fields list of Entries against expected. compareEntries does not compare: // - HTTPRequest // - Operation diff --git a/logging/logging_unexported_test.go b/logging/logging_unexported_test.go index a6413b8bc68..6d1ac9c5e50 100644 --- a/logging/logging_unexported_test.go +++ b/logging/logging_unexported_test.go @@ -27,7 +27,6 @@ import ( "github.com/golang/protobuf/proto" durpb "github.com/golang/protobuf/ptypes/duration" structpb "github.com/golang/protobuf/ptypes/struct" - "google.golang.org/api/logging/v2" "google.golang.org/api/support/bundler" mrpb "google.golang.org/genproto/googleapis/api/monitoredres" logtypepb "google.golang.org/genproto/googleapis/logging/type" @@ -181,7 +180,6 @@ func TestToProtoStruct(t *testing.T) { } func TestToLogEntryPayload(t *testing.T) { - var logger Logger for _, test := range []struct { in interface{} wantText string @@ -210,7 +208,7 @@ func TestToLogEntryPayload(t *testing.T) { }, }, } { - e, err := logger.toLogEntry(Entry{Payload: test.in}) + e, err := toLogEntryInternal(Entry{Payload: test.in}, nil, "") if err != nil { t.Fatalf("%+v: %v", test.in, err) } @@ -228,153 +226,6 @@ func TestToLogEntryPayload(t *testing.T) { } } -func TestToLogEntryTrace(t *testing.T) { - logger := &Logger{client: &Client{parent: "projects/P"}} - // Verify that we get the trace from the HTTP request if it isn't - // provided by the caller. - u := &url.URL{Scheme: "http"} - - tests := []struct { - name string - in Entry - want logging.LogEntry - }{ - {"BlankLogEntry", Entry{}, logging.LogEntry{}}, - {"Already set Trace", Entry{Trace: "t1"}, logging.LogEntry{Trace: "t1"}}, - { - "No X-Trace-Context header", - Entry{ - HTTPRequest: &HTTPRequest{ - Request: &http.Request{URL: u, Header: http.Header{"foo": {"bar"}}}, - }, - }, - logging.LogEntry{}, - }, - { - "X-Trace-Context header with all fields", - Entry{ - TraceSampled: false, - HTTPRequest: &HTTPRequest{ - Request: &http.Request{ - URL: u, - Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/000000000000004a;o=1"}}, - }, - }, - }, - logging.LogEntry{Trace: "projects/P/traces/105445aa7843bc8bf206b120001000", SpanId: "000000000000004a", TraceSampled: true}, - }, - { - "X-Trace-Context header with all fields; TraceSampled explicitly set", - Entry{ - TraceSampled: true, - HTTPRequest: &HTTPRequest{ - Request: &http.Request{ - URL: u, - Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/000000000000004a;o=0"}}, - }, - }, - }, - logging.LogEntry{Trace: "projects/P/traces/105445aa7843bc8bf206b120001000", SpanId: "000000000000004a", TraceSampled: true}, - }, - { - "X-Trace-Context header with all fields; TraceSampled from Header", - Entry{ - HTTPRequest: &HTTPRequest{ - Request: &http.Request{ - URL: u, - Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/000000000000004a;o=1"}}, - }, - }, - }, - logging.LogEntry{Trace: "projects/P/traces/105445aa7843bc8bf206b120001000", SpanId: "000000000000004a", TraceSampled: true}, - }, - { - "X-Trace-Context header with blank trace", - Entry{ - HTTPRequest: &HTTPRequest{ - Request: &http.Request{ - URL: u, - Header: http.Header{"X-Cloud-Trace-Context": {"/0;o=1"}}, - }, - }, - }, - logging.LogEntry{TraceSampled: true}, - }, - { - "X-Trace-Context header with blank span", - Entry{ - HTTPRequest: &HTTPRequest{ - Request: &http.Request{ - URL: u, - Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/;o=0"}}, - }, - }, - }, - logging.LogEntry{Trace: "projects/P/traces/105445aa7843bc8bf206b120001000"}, - }, - { - "X-Trace-Context header with missing traceSampled aka ?o=*", - Entry{ - HTTPRequest: &HTTPRequest{ - Request: &http.Request{ - URL: u, - Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/0"}}, - }, - }, - }, - logging.LogEntry{Trace: "projects/P/traces/105445aa7843bc8bf206b120001000"}, - }, - { - "X-Trace-Context header with all blank fields", - Entry{ - HTTPRequest: &HTTPRequest{ - Request: &http.Request{ - URL: u, - Header: http.Header{"X-Cloud-Trace-Context": {""}}, - }, - }, - }, - logging.LogEntry{}, - }, - { - "Invalid X-Trace-Context header but already set TraceID", - Entry{ - HTTPRequest: &HTTPRequest{ - Request: &http.Request{ - URL: u, - Header: http.Header{"X-Cloud-Trace-Context": {"t3"}}, - }, - }, - Trace: "t4", - }, - logging.LogEntry{Trace: "t4"}, - }, - { - "Already set TraceID and SpanID", - Entry{Trace: "t1", SpanID: "007"}, - logging.LogEntry{Trace: "t1", SpanId: "007"}, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - e, err := logger.toLogEntry(test.in) - if err != nil { - t.Fatalf("Unexpected error:: %+v: %v", test.in, err) - } - if got := e.Trace; got != test.want.Trace { - t.Errorf("TraceId: %+v: got %q, want %q", test.in, got, test.want.Trace) - } - if got := e.SpanId; got != test.want.SpanId { - t.Errorf("SpanId: %+v: got %q, want %q", test.in, got, test.want.SpanId) - } - if got := e.TraceSampled; got != test.want.TraceSampled { - t.Errorf("TraceSampled: %+v: got %t, want %t", test.in, got, test.want.TraceSampled) - } - }) - } -} - func TestFromHTTPRequest(t *testing.T) { // The test URL has invalid UTF-8 runes. const testURL = "http://example.com/path?q=1&name=\xfe\xff"