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

feat(logging): OpenTelemetry trace/span ID integration for Go logging library #10030

Merged
merged 8 commits into from May 16, 2024
4 changes: 2 additions & 2 deletions logging/go.mod
Expand Up @@ -11,6 +11,8 @@ require (
github.com/google/go-cmp v0.6.0
github.com/googleapis/gax-go/v2 v2.12.4
go.opencensus.io v0.24.0
go.opentelemetry.io/otel/sdk v1.24.0
go.opentelemetry.io/otel/trace v1.24.0
golang.org/x/oauth2 v0.20.0
google.golang.org/api v0.180.0
google.golang.org/genproto v0.0.0-20240401170217-c3f982113cda
Expand All @@ -35,8 +37,6 @@ require (
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.49.0 // indirect
go.opentelemetry.io/otel v1.24.0 // indirect
go.opentelemetry.io/otel/metric v1.24.0 // indirect
go.opentelemetry.io/otel/sdk v1.24.0 // indirect
go.opentelemetry.io/otel/trace v1.24.0 // indirect
golang.org/x/crypto v0.23.0 // indirect
golang.org/x/net v0.25.0 // indirect
golang.org/x/sync v0.7.0 // indirect
Expand Down
9 changes: 9 additions & 0 deletions logging/logging.go
Expand Up @@ -41,6 +41,8 @@ import (
"time"
"unicode/utf8"

"go.opentelemetry.io/otel/trace"

vkit "cloud.google.com/go/logging/apiv2"
logpb "cloud.google.com/go/logging/apiv2/loggingpb"
"cloud.google.com/go/logging/internal"
Expand Down Expand Up @@ -813,6 +815,13 @@ func populateTraceInfo(e *Entry, req *http.Request) bool {
return false
}
}
otelSpanContext := trace.SpanContextFromContext(req.Context())
if otelSpanContext.IsValid() {
e.Trace = otelSpanContext.TraceID().String()
e.SpanID = otelSpanContext.SpanID().String()
e.TraceSampled = otelSpanContext.IsSampled()
return true
}
header := req.Header.Get("Traceparent")
if header != "" {
// do not use traceSampled flag defined by traceparent because
Expand Down
129 changes: 129 additions & 0 deletions logging/logging_test.go
Expand Up @@ -48,6 +48,9 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
gax "github.com/googleapis/gax-go/v2"
sdktrace "go.opentelemetry.io/otel/sdk/trace"
"go.opentelemetry.io/otel/trace"
"go.opentelemetry.io/otel/trace/noop"
"golang.org/x/oauth2"
"google.golang.org/api/iterator"
"google.golang.org/api/option"
Expand Down Expand Up @@ -609,6 +612,132 @@ func TestToLogEntry(t *testing.T) {
}
}

func TestToLogEntryOTelIntegration(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to add system tests to start otel span and compare auto-generated otel spanId with the injected logEntry spanId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cindy-peng I just added an integration/system test for the exact same test case, just with different span setups.

// Some slight modifications need to be done for testing ToLogEntry
// for the OpenTelemetry integration, so they are in a separate function.
u := &url.URL{Scheme: "http"}
tests := []struct {
name string
in logging.Entry
want *logpb.LogEntry // if want is nil, pull wants from spanContext
}{
{
name: "Using OpenTelemetry with a valid span",
in: logging.Entry{
HTTPRequest: &logging.HTTPRequest{
Request: &http.Request{
URL: u,
},
},
},
},
{
name: "Using OpenTelemetry only with a valid span + valid traceparent headers (precedence test)",
in: logging.Entry{
HTTPRequest: &logging.HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{
"Traceparent": {"00-105445aa7843bc8bf206b12000100012-000000000000004a-01"},
},
},
},
},
},
{
name: "Using OpenTelemetry only with a valid span + valid XCTC headers (precedence test)",
in: logging.Entry{
HTTPRequest: &logging.HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{
"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120000000/0000000000000bbb;o=1"},
},
},
},
},
},
{
name: "Using OpenTelemetry with a valid span + trace info set in Entry object",
in: logging.Entry{
HTTPRequest: &logging.HTTPRequest{
Request: &http.Request{
URL: u,
},
},
Trace: "abc",
SpanID: "def",
TraceSampled: false,
},
want: &logpb.LogEntry{
Trace: "abc",
SpanId: "def",
TraceSampled: false,
},
},
{
name: "Using OpenTelemetry without a request",
in: logging.Entry{},
want: &logpb.LogEntry{},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
var span trace.Span
ctx := context.Background()

// Set up an OTel SDK tracer if integration test, mock noop tracer if not.
if integrationTest {
Copy link
Contributor

@cindy-peng cindy-peng May 7, 2024

Choose a reason for hiding this comment

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

Do we have a dedicated place for integration tests in Go Logging library?
For example, for Java-logging and Node-logging, there is a separate folder for integration tests:
https://github.com/googleapis/java-logging/tree/main/google-cloud-logging/src/test/java/com/google/cloud/logging/it
https://github.com/googleapis/nodejs-logging/tree/main/system-test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we have any right now, but if enough system tests happen that are kinda different from integration tests controlled by the integrationTest flag I could make a separate file for them.

tracerProvider := sdktrace.NewTracerProvider()
defer tracerProvider.Shutdown(ctx)

ctx, span = tracerProvider.Tracer("integration-test-tracer").Start(ctx, "test span")
defer span.End()
} else {
otelTraceID, _ := trace.TraceIDFromHex(strings.Repeat("a", 32))
otelSpanID, _ := trace.SpanIDFromHex(strings.Repeat("f", 16))
otelTraceFlags := trace.FlagsSampled // tracesampled = true
mockSpanContext := trace.NewSpanContext(trace.SpanContextConfig{
TraceID: otelTraceID,
SpanID: otelSpanID,
TraceFlags: otelTraceFlags,
})
ctx = trace.ContextWithSpanContext(ctx, mockSpanContext)
ctx, span = noop.NewTracerProvider().Tracer("test tracer").Start(ctx, "test span")
defer span.End()
}

if test.in.HTTPRequest != nil && test.in.HTTPRequest.Request != nil {
test.in.HTTPRequest.Request = test.in.HTTPRequest.Request.WithContext(ctx)
}
spanContext := trace.SpanContextFromContext(ctx)

// if want is nil, pull wants from spanContext
if test.want == nil {
test.want = &logpb.LogEntry{
Trace: "projects/P/traces/" + spanContext.TraceID().String(),
SpanId: spanContext.SpanID().String(),
TraceSampled: spanContext.TraceFlags().IsSampled(),
}
}

e, err := logging.ToLogEntry(test.in, "projects/P")
if err != nil {
t.Fatalf("Unexpected error: %+v: %v", test.in, err)
}
if got := e.Trace; got != test.want.Trace {
t.Errorf("TraceId: %+v: SpanContext: %+v: got %q, want %q", test.in, spanContext, got, test.want.Trace)
}
if got := e.SpanId; got != test.want.SpanId {
t.Errorf("SpanId: %+v: SpanContext: %+v: got %q, want %q", test.in, spanContext, got, test.want.SpanId)
}
if got := e.TraceSampled; got != test.want.TraceSampled {
t.Errorf("TraceSampled: %+v: SpanContext: %+v: got %t, want %t", test.in, spanContext, got, test.want.TraceSampled)
}
})
}
}

// compareEntries compares most fields list of Entries against expected. compareEntries does not compare:
// - HTTPRequest
// - Operation
Expand Down