From f88cc9b61109bf2b9ef027e6b3646f9514517232 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Tue, 29 Oct 2024 10:17:42 -0700 Subject: [PATCH 1/3] chore: introduce storage trace package --- storage/bucket.go | 36 ++++++++++++++++++------ storage/go.mod | 2 +- storage/trace.go | 70 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 9 deletions(-) create mode 100644 storage/trace.go diff --git a/storage/bucket.go b/storage/bucket.go index 3eded017831e..762faeab3492 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -82,8 +82,13 @@ func (c *Client) Bucket(name string) *BucketHandle { // Create creates the Bucket in the project. // If attrs is nil the API defaults will be used. func (b *BucketHandle) Create(ctx context.Context, projectID string, attrs *BucketAttrs) (err error) { - ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Create") - defer func() { trace.EndSpan(ctx, err) }() + if isOTelTracingDevEnabled() { + ctx, _ = startSpan(ctx, "cloud.google.com/go/storage.Bucket.Create") + defer func() { endSpan(ctx, err) }() + } else { + ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Create") + defer func() { trace.EndSpan(ctx, err) }() + } o := makeStorageOpts(true, b.retry, b.userProject) @@ -95,8 +100,13 @@ func (b *BucketHandle) Create(ctx context.Context, projectID string, attrs *Buck // Delete deletes the Bucket. func (b *BucketHandle) Delete(ctx context.Context) (err error) { - ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Delete") - defer func() { trace.EndSpan(ctx, err) }() + if isOTelTracingDevEnabled() { + ctx, _ = startSpan(ctx, "cloud.google.com/go/storage.Bucket.Delete") + defer func() { endSpan(ctx, err) }() + } else { + ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Delete") + defer func() { trace.EndSpan(ctx, err) }() + } o := makeStorageOpts(true, b.retry, b.userProject) return b.c.tc.DeleteBucket(ctx, b.name, b.conds, o...) @@ -150,8 +160,13 @@ func (b *BucketHandle) Object(name string) *ObjectHandle { // Attrs returns the metadata for the bucket. func (b *BucketHandle) Attrs(ctx context.Context) (attrs *BucketAttrs, err error) { - ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Attrs") - defer func() { trace.EndSpan(ctx, err) }() + if isOTelTracingDevEnabled() { + ctx, _ = startSpan(ctx, "cloud.google.com/go/storage.Bucket.Attrs") + defer func() { endSpan(ctx, err) }() + } else { + ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Attrs") + defer func() { trace.EndSpan(ctx, err) }() + } o := makeStorageOpts(true, b.retry, b.userProject) return b.c.tc.GetBucket(ctx, b.name, b.conds, o...) @@ -159,8 +174,13 @@ func (b *BucketHandle) Attrs(ctx context.Context) (attrs *BucketAttrs, err error // Update updates a bucket's attributes. func (b *BucketHandle) Update(ctx context.Context, uattrs BucketAttrsToUpdate) (attrs *BucketAttrs, err error) { - ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Update") - defer func() { trace.EndSpan(ctx, err) }() + if isOTelTracingDevEnabled() { + ctx, _ = startSpan(ctx, "cloud.google.com/go/storage.Bucket.Update") + defer func() { endSpan(ctx, err) }() + } else { + ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Update") + defer func() { trace.EndSpan(ctx, err) }() + } isIdempotent := b.conds != nil && b.conds.MetagenerationMatch != 0 o := makeStorageOpts(isIdempotent, b.retry, b.userProject) diff --git a/storage/go.mod b/storage/go.mod index d0cf85db3da9..66bdb9cd3bd5 100644 --- a/storage/go.mod +++ b/storage/go.mod @@ -17,6 +17,7 @@ require ( go.opentelemetry.io/otel v1.29.0 go.opentelemetry.io/otel/sdk v1.29.0 go.opentelemetry.io/otel/sdk/metric v1.29.0 + go.opentelemetry.io/otel/trace v1.29.0 golang.org/x/oauth2 v0.23.0 golang.org/x/sync v0.8.0 google.golang.org/api v0.203.0 @@ -51,7 +52,6 @@ require ( go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.54.0 // indirect go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.54.0 // indirect go.opentelemetry.io/otel/metric v1.29.0 // indirect - go.opentelemetry.io/otel/trace v1.29.0 // indirect golang.org/x/crypto v0.28.0 // indirect golang.org/x/net v0.30.0 // indirect golang.org/x/sys v0.26.0 // indirect diff --git a/storage/trace.go b/storage/trace.go new file mode 100644 index 000000000000..84e610b8ace2 --- /dev/null +++ b/storage/trace.go @@ -0,0 +1,70 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package storage + +import ( + "context" + "os" + + "cloud.google.com/go/storage/internal" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" + otelcodes "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/trace" +) + +const ( + OpenTelemetryTracingExpVar = "GO_STORAGE_EXPERIMENTAL_OTEL_TRACING" + defaultTracerName = "cloud.google.com/go/storage" + gcpClientRepo = "googleapis/google-cloud-go" + gcpClientArtifact = "storage" +) + +// isOTelTracingDevEnabled checks the development flag until experimental feature is launched. +func isOTelTracingDevEnabled() bool { + return os.Getenv(OpenTelemetryTracingExpVar) == "true" +} + +func tracer() trace.Tracer { + return otel.Tracer(defaultTracerName, trace.WithInstrumentationVersion(internal.Version)) +} + +// startSpan accepts SpanStartOption and is used to replace internal/trace/StartSpan. +func startSpan(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) { + opts = append(opts, getCommonTraceOptions()...) + return tracer().Start(ctx, name, opts...) +} + +// endSpan is used to replace internal/trace/EndSpan. +func endSpan(ctx context.Context, err error) { + span := trace.SpanFromContext(ctx) + if err != nil { + span.SetStatus(otelcodes.Error, err.Error()) + span.RecordError(err) + } + span.End() +} + +// getCommonTraceOptions includes the common attributes used for Cloud Trace adoption tracking. +func getCommonTraceOptions() []trace.SpanStartOption { + opts := []trace.SpanStartOption{ + trace.WithAttributes( + attribute.String("gcp.client.version", internal.Version), + attribute.String("gcp.client.repo", gcpClientRepo), + attribute.String("gcp.client.artifact", gcpClientArtifact), + ), + } + return opts +} From 0b2055c237f2f4643310ae1080aed3bc706a92ed Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Thu, 31 Oct 2024 14:18:00 -0700 Subject: [PATCH 2/3] add tests and review comments --- storage/bucket.go | 8 +-- storage/trace.go | 10 +-- storage/trace_test.go | 147 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+), 9 deletions(-) create mode 100644 storage/trace_test.go diff --git a/storage/bucket.go b/storage/bucket.go index 762faeab3492..cf027c599258 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -83,7 +83,7 @@ func (c *Client) Bucket(name string) *BucketHandle { // If attrs is nil the API defaults will be used. func (b *BucketHandle) Create(ctx context.Context, projectID string, attrs *BucketAttrs) (err error) { if isOTelTracingDevEnabled() { - ctx, _ = startSpan(ctx, "cloud.google.com/go/storage.Bucket.Create") + ctx, _ = startSpan(ctx, "storage.Bucket.Create") defer func() { endSpan(ctx, err) }() } else { ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Create") @@ -101,7 +101,7 @@ func (b *BucketHandle) Create(ctx context.Context, projectID string, attrs *Buck // Delete deletes the Bucket. func (b *BucketHandle) Delete(ctx context.Context) (err error) { if isOTelTracingDevEnabled() { - ctx, _ = startSpan(ctx, "cloud.google.com/go/storage.Bucket.Delete") + ctx, _ = startSpan(ctx, "storage.Bucket.Delete") defer func() { endSpan(ctx, err) }() } else { ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Delete") @@ -161,7 +161,7 @@ func (b *BucketHandle) Object(name string) *ObjectHandle { // Attrs returns the metadata for the bucket. func (b *BucketHandle) Attrs(ctx context.Context) (attrs *BucketAttrs, err error) { if isOTelTracingDevEnabled() { - ctx, _ = startSpan(ctx, "cloud.google.com/go/storage.Bucket.Attrs") + ctx, _ = startSpan(ctx, "storage.Bucket.Attrs") defer func() { endSpan(ctx, err) }() } else { ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Attrs") @@ -175,7 +175,7 @@ func (b *BucketHandle) Attrs(ctx context.Context) (attrs *BucketAttrs, err error // Update updates a bucket's attributes. func (b *BucketHandle) Update(ctx context.Context, uattrs BucketAttrsToUpdate) (attrs *BucketAttrs, err error) { if isOTelTracingDevEnabled() { - ctx, _ = startSpan(ctx, "cloud.google.com/go/storage.Bucket.Update") + ctx, _ = startSpan(ctx, "storage.Bucket.Update") defer func() { endSpan(ctx, err) }() } else { ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Update") diff --git a/storage/trace.go b/storage/trace.go index 84e610b8ace2..a7ded91bb5c0 100644 --- a/storage/trace.go +++ b/storage/trace.go @@ -26,15 +26,15 @@ import ( ) const ( - OpenTelemetryTracingExpVar = "GO_STORAGE_EXPERIMENTAL_OTEL_TRACING" - defaultTracerName = "cloud.google.com/go/storage" - gcpClientRepo = "googleapis/google-cloud-go" - gcpClientArtifact = "storage" + storageOtelTracingDevVar = "GO_STORAGE_DEV_OTEL_TRACING" + defaultTracerName = "cloud.google.com/go/storage" + gcpClientRepo = "googleapis/google-cloud-go" + gcpClientArtifact = "storage" ) // isOTelTracingDevEnabled checks the development flag until experimental feature is launched. func isOTelTracingDevEnabled() bool { - return os.Getenv(OpenTelemetryTracingExpVar) == "true" + return os.Getenv(storageOtelTracingDevVar) == "true" } func tracer() trace.Tracer { diff --git a/storage/trace_test.go b/storage/trace_test.go new file mode 100644 index 000000000000..11a56e7c5e8d --- /dev/null +++ b/storage/trace_test.go @@ -0,0 +1,147 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package storage + +import ( + "context" + "fmt" + "io" + "io/ioutil" + "net/http" + "os" + "testing" + + "cloud.google.com/go/internal/testutil" + "cloud.google.com/go/storage/internal" + "github.com/google/go-cmp/cmp" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" + "go.opentelemetry.io/otel/trace" + "google.golang.org/api/option" +) + +func TestTraceStorageTraceStartEndSpan(t *testing.T) { + originalOtelTracingBool := os.Getenv("GO_STORAGE_DEV_OTEL_TRACING") + defer os.Setenv("GO_STORAGE_DEV_OTEL_TRACING", originalOtelTracingBool) + + os.Setenv("GO_STORAGE_DEV_OTEL_TRACING", "true") + ctx := context.Background() + e := tracetest.NewInMemoryExporter() + tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(e)) + defer tp.Shutdown(ctx) + otel.SetTracerProvider(tp) + + spanName := "storage.TestTrace.TestStorageTraceStartEndSpan" + spanStartopts := []trace.SpanStartOption{ + trace.WithAttributes( + attribute.String("foo", "bar"), + ), + } + addAttrs := attribute.String("fakeKey", "fakeVal") + + ctx, span := startSpan(ctx, spanName, spanStartopts...) + span.SetAttributes(addAttrs) + endSpan(ctx, nil) + + spans := e.GetSpans() + if len(spans) != 1 { + t.Errorf("expected one span, got %d", len(spans)) + } + + // Test StartSpanOption and Cloud Trace Adoption common attributes are appended. + wantAttributes := tracetest.SpanStub{ + Name: spanName, + Attributes: []attribute.KeyValue{ + attribute.String("foo", "bar"), + attribute.String("gcp.client.version", internal.Version), + attribute.String("gcp.client.repo", gcpClientRepo), + attribute.String("gcp.client.artifact", gcpClientArtifact), + }, + } + // Test startSpan returns the span and additional attributes can be set. + wantAttributes.Attributes = append(wantAttributes.Attributes, addAttrs) + opts := []cmp.Option{ + cmp.Comparer(spanAttributesComparer), + } + for _, span := range spans { + if diff := testutil.Diff(span, wantAttributes, opts...); diff != "" { + t.Errorf("diff: -got, +want:\n%s\n", diff) + } + } +} + +func TestTraceOtelTraceDevFlagEnabled(t *testing.T) { + originalOtelTracingBool := os.Getenv("GO_STORAGE_DEV_OTEL_TRACING") + defer os.Setenv("GO_STORAGE_DEV_OTEL_TRACING", originalOtelTracingBool) + + os.Setenv("GO_STORAGE_DEV_OTEL_TRACING", "true") + ctx := context.Background() + e := tracetest.NewInMemoryExporter() + tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(e)) + defer tp.Shutdown(ctx) + otel.SetTracerProvider(tp) + + // This utilizes newTestServer to make RPC calls and export traces to + // the tracetest.InMemoryExporter. + // TBD: What RPC calls do we want to test? + hClient, close := newTestServer(func(w http.ResponseWriter, r *http.Request) { + io.Copy(ioutil.Discard, r.Body) + fmt.Fprintf(w, "{}") + }) + defer close() + + client, err := NewClient(ctx, option.WithHTTPClient(hClient)) + if err != nil { + t.Fatal(err) + } + _, err = client.Bucket("b").Attrs(ctx) + if err != nil { + t.Errorf("got %v", err) + } + + // Test Cloud Trace Adoption common attributes are appended. + wantAttributes := tracetest.SpanStub{ + Name: "storage.Bucket.Attrs", + Attributes: []attribute.KeyValue{ + attribute.String("gcp.client.version", internal.Version), + attribute.String("gcp.client.repo", gcpClientRepo), + attribute.String("gcp.client.artifact", gcpClientArtifact), + }, + } + + spans := e.GetSpans() + opts := []cmp.Option{ + cmp.Comparer(spanAttributesComparer), + } + for _, span := range spans { + if diff := testutil.Diff(span, wantAttributes, opts...); diff != "" { + t.Errorf("diff: -got, +want:\n%s\n", diff) + } + } +} + +func spanAttributesComparer(a, b tracetest.SpanStub) bool { + if a.Name != b.Name { + fmt.Printf("name mismatch: a.Name: %v, b.Name: %v\n", a.Name, b.Name) + return false + } + if len(a.Attributes) != len(b.Attributes) { + fmt.Printf("len mismatch: a.Attributes: %d, b.Attributes: %d\n", len(a.Attributes), len(b.Attributes)) + return false + } + return true +} From f74dae6aa86bcde3a63e672927aace928cc52462 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Tue, 5 Nov 2024 14:29:59 -0800 Subject: [PATCH 3/3] move tests and bucket instrumentation to separate PR --- storage/bucket.go | 36 +++++++---------------------- storage/trace_test.go | 54 ------------------------------------------- 2 files changed, 8 insertions(+), 82 deletions(-) diff --git a/storage/bucket.go b/storage/bucket.go index cf027c599258..3eded017831e 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -82,13 +82,8 @@ func (c *Client) Bucket(name string) *BucketHandle { // Create creates the Bucket in the project. // If attrs is nil the API defaults will be used. func (b *BucketHandle) Create(ctx context.Context, projectID string, attrs *BucketAttrs) (err error) { - if isOTelTracingDevEnabled() { - ctx, _ = startSpan(ctx, "storage.Bucket.Create") - defer func() { endSpan(ctx, err) }() - } else { - ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Create") - defer func() { trace.EndSpan(ctx, err) }() - } + ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Create") + defer func() { trace.EndSpan(ctx, err) }() o := makeStorageOpts(true, b.retry, b.userProject) @@ -100,13 +95,8 @@ func (b *BucketHandle) Create(ctx context.Context, projectID string, attrs *Buck // Delete deletes the Bucket. func (b *BucketHandle) Delete(ctx context.Context) (err error) { - if isOTelTracingDevEnabled() { - ctx, _ = startSpan(ctx, "storage.Bucket.Delete") - defer func() { endSpan(ctx, err) }() - } else { - ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Delete") - defer func() { trace.EndSpan(ctx, err) }() - } + ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Delete") + defer func() { trace.EndSpan(ctx, err) }() o := makeStorageOpts(true, b.retry, b.userProject) return b.c.tc.DeleteBucket(ctx, b.name, b.conds, o...) @@ -160,13 +150,8 @@ func (b *BucketHandle) Object(name string) *ObjectHandle { // Attrs returns the metadata for the bucket. func (b *BucketHandle) Attrs(ctx context.Context) (attrs *BucketAttrs, err error) { - if isOTelTracingDevEnabled() { - ctx, _ = startSpan(ctx, "storage.Bucket.Attrs") - defer func() { endSpan(ctx, err) }() - } else { - ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Attrs") - defer func() { trace.EndSpan(ctx, err) }() - } + ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Attrs") + defer func() { trace.EndSpan(ctx, err) }() o := makeStorageOpts(true, b.retry, b.userProject) return b.c.tc.GetBucket(ctx, b.name, b.conds, o...) @@ -174,13 +159,8 @@ func (b *BucketHandle) Attrs(ctx context.Context) (attrs *BucketAttrs, err error // Update updates a bucket's attributes. func (b *BucketHandle) Update(ctx context.Context, uattrs BucketAttrsToUpdate) (attrs *BucketAttrs, err error) { - if isOTelTracingDevEnabled() { - ctx, _ = startSpan(ctx, "storage.Bucket.Update") - defer func() { endSpan(ctx, err) }() - } else { - ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Update") - defer func() { trace.EndSpan(ctx, err) }() - } + ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Update") + defer func() { trace.EndSpan(ctx, err) }() isIdempotent := b.conds != nil && b.conds.MetagenerationMatch != 0 o := makeStorageOpts(isIdempotent, b.retry, b.userProject) diff --git a/storage/trace_test.go b/storage/trace_test.go index 11a56e7c5e8d..552fc526c290 100644 --- a/storage/trace_test.go +++ b/storage/trace_test.go @@ -17,9 +17,6 @@ package storage import ( "context" "fmt" - "io" - "io/ioutil" - "net/http" "os" "testing" @@ -31,7 +28,6 @@ import ( sdktrace "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" "go.opentelemetry.io/otel/trace" - "google.golang.org/api/option" ) func TestTraceStorageTraceStartEndSpan(t *testing.T) { @@ -84,56 +80,6 @@ func TestTraceStorageTraceStartEndSpan(t *testing.T) { } } -func TestTraceOtelTraceDevFlagEnabled(t *testing.T) { - originalOtelTracingBool := os.Getenv("GO_STORAGE_DEV_OTEL_TRACING") - defer os.Setenv("GO_STORAGE_DEV_OTEL_TRACING", originalOtelTracingBool) - - os.Setenv("GO_STORAGE_DEV_OTEL_TRACING", "true") - ctx := context.Background() - e := tracetest.NewInMemoryExporter() - tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(e)) - defer tp.Shutdown(ctx) - otel.SetTracerProvider(tp) - - // This utilizes newTestServer to make RPC calls and export traces to - // the tracetest.InMemoryExporter. - // TBD: What RPC calls do we want to test? - hClient, close := newTestServer(func(w http.ResponseWriter, r *http.Request) { - io.Copy(ioutil.Discard, r.Body) - fmt.Fprintf(w, "{}") - }) - defer close() - - client, err := NewClient(ctx, option.WithHTTPClient(hClient)) - if err != nil { - t.Fatal(err) - } - _, err = client.Bucket("b").Attrs(ctx) - if err != nil { - t.Errorf("got %v", err) - } - - // Test Cloud Trace Adoption common attributes are appended. - wantAttributes := tracetest.SpanStub{ - Name: "storage.Bucket.Attrs", - Attributes: []attribute.KeyValue{ - attribute.String("gcp.client.version", internal.Version), - attribute.String("gcp.client.repo", gcpClientRepo), - attribute.String("gcp.client.artifact", gcpClientArtifact), - }, - } - - spans := e.GetSpans() - opts := []cmp.Option{ - cmp.Comparer(spanAttributesComparer), - } - for _, span := range spans { - if diff := testutil.Diff(span, wantAttributes, opts...); diff != "" { - t.Errorf("diff: -got, +want:\n%s\n", diff) - } - } -} - func spanAttributesComparer(a, b tracetest.SpanStub) bool { if a.Name != b.Name { fmt.Printf("name mismatch: a.Name: %v, b.Name: %v\n", a.Name, b.Name)