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

feat(logging): make toLogEntry function public #3863

Merged
merged 2 commits into from Apr 9, 2021
+241 −158
Merged

feat(logging): make toLogEntry function public #3863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
008f2f1
feat(logging): make toLogEntry function public
loburm Mar 25, 2021
bc1f8e3
Merge branch 'master' into log-public
loburm Apr 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 23 additions & 0 deletions logging/examples_test.go
View file
Open in desktop
Expand Up @@ -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() {
Expand Down Expand Up @@ -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")
Expand Down
44 changes: 36 additions & 8 deletions logging/logging.go
View file
Open in desktop
Expand Up @@ -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...)
Expand Down Expand Up @@ -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".
Expand Down Expand Up @@ -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
}
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

If we think there is a big enough use case for something like this I wonder if we should make an optional Parent field on Entry? It could fall back to the parent specified on the client if not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Afraid that it can be a little bit dangerous. Unless you think that's completely OK to fail ToLogEntry if we need this field for traceID?

Copy link
Contributor

@0xSage 0xSage Mar 29, 2021 •
edited

Choose a reason for hiding this comment

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

Entry already has parent as a substring in its LogName field.

Might I suggest creating ToLogEntry as a method on Entry:
func (e *Entry) ToLogEntry(client ...*Client). It's still not dependent on client like you wanted. And function signature is cleaner, and we dont need to create new internal functions.

Unless, we dont like using variadic parameters to enable optional parameters in Go @codyoss ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LogName can't be used, because toLogEntry is failing whenever we set that field. Unless you want me to remove that check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strange, I have received a comment, but don't see it here.

I think it makes sense to remove that check in cases where there's no client/logger. And to update the corresponding comments. Ofc when there is a logger/client, then users can't set the logname (same behavior as before).
Also LogName and Resource fields are required in order to write to Cloud Logging. As a part of your implementation, can you also write checks to ensure that if ppl use ToLogEntry, they are creating valid LogEntries?

  1. (Related to previous comment). Golang doesn't support optional arguments. client ...*Client means that we can pass multiple clients and that's a very confusing in this context.
  2. I think different behavior when you call with Client and without is also very confusing.
  3. LogName and resource are optional. They can be set on WriteLogEntries level or even ignored completely.

Copy link
Contributor

Choose a reason for hiding this comment

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

I deleted this comment because after thinking through various modifications to make this PR work, I don't think this logic should be introduced at the client library at this veneer layer. Agree that using a variadic param to hack things isn't the right way to go.

if e.LogName != "" {
return nil, errors.New("logging: Entry.LogName should be not be set when writing")
}
Expand All @@ -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
Expand All @@ -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,
Expand Down
181 changes: 181 additions & 0 deletions logging/logging_test.go
View file
Open in desktop
Expand Up @@ -19,10 +19,13 @@ package logging_test
import (
"context"
"encoding/json"
"errors"
"flag"
"fmt"
"log"
"math/rand"
"net/http"
"net/url"
"os"
"strings"
"sync"
Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand Down