From 53726f175fefb856c77916965ee7b684bbfae39d Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Thu, 14 Nov 2024 22:38:04 -0800 Subject: [PATCH] refactor tests and review comments --- storage/trace.go | 5 + storage/trace_test.go | 207 +++++++++++++++++------------------------- 2 files changed, 90 insertions(+), 122 deletions(-) diff --git a/storage/trace.go b/storage/trace.go index b3456d25cbbe..6e2faea0d782 100644 --- a/storage/trace.go +++ b/storage/trace.go @@ -49,6 +49,7 @@ func tracer() trace.Tracer { func startSpan(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) { // TODO: Remove internalTrace upon experimental launch. if !isOTelTracingDevEnabled() { + name = appendPackageName(name) ctx = internalTrace.StartSpan(ctx, name) return ctx, nil } @@ -85,3 +86,7 @@ func getCommonTraceOptions() []trace.SpanStartOption { } return opts } + +func appendPackageName(spanName string) string { + return "cloud.google.com/go/" + spanName +} diff --git a/storage/trace_test.go b/storage/trace_test.go index 205804f81861..b86d6967689c 100644 --- a/storage/trace_test.go +++ b/storage/trace_test.go @@ -16,15 +16,11 @@ package storage import ( "context" - "fmt" - "slices" "testing" - "time" "cloud.google.com/go/internal/testutil" "cloud.google.com/go/storage/internal" "github.com/google/go-cmp/cmp" - "github.com/googleapis/gax-go/v2" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/sdk/instrumentation" @@ -36,13 +32,9 @@ import ( type emulatedTraceTest struct { *testing.T - name string - resources resources - transportClient *Client + resources resources } -type traceFunc func(ctx context.Context, c *Client, fs *resources) error - func TestTraceStorageTraceStartEndSpan(t *testing.T) { ctx := context.Background() e := tracetest.NewInMemoryExporter() @@ -85,78 +77,104 @@ func TestTraceStorageTraceStartEndSpan(t *testing.T) { e.Reset() } -func TestTraceSpansEmulated(t *testing.T) { - checkEmulatorEnvironment(t) - - // Create non-wrapped client to use for setup steps. - ctx := context.Background() - client, err := NewClient(ctx) - if err != nil { - t.Fatalf("storage.NewClient: %v", err) +func TestTraceSpansMultiEmulated(t *testing.T) { + ctx := skipJSONReads(context.Background(), "no reads in test") + // To only test storage library layer instrumentation, + // we disable transport layer traces for testing purposes. + opts := []option.ClientOption{ + option.WithTelemetryDisabled(), } - - skippedTraceMethods := []string{"storage.Bucket.AddNotification", "storage.Bucket.Notifications", "storage.Bucket.DeleteNotification"} - for spanName, fn := range traceMethods { - transports := []string{"http", "grpc"} - for _, transport := range transports { - testName := fmt.Sprintf("TestTrace-%v-%v", transport, spanName) - t.Run(testName, func(t *testing.T) { - if transport == "grpc" && slices.Contains(skippedTraceMethods, spanName) { - t.Skip("not supported") - } - - // Setup: Create the trace subtest, transport client and test resources. - subtest := &emulatedTraceTest{T: t, name: testName} - subtest.initTransportClient(transport) - resources := []string{"bucket", "object", "notification"} - subtest.populateResources(ctx, client, resources) + multiTransportTest(ctx, t, func(t *testing.T, ctx context.Context, bucket string, prefix string, client *Client) { + for _, c := range []struct { + name string + resources []string + call func(ctx context.Context, c *Client, fs *resources) error + }{ + { + name: "storage.Bucket.Attrs", + resources: []string{"bucket"}, + call: func(ctx context.Context, c *Client, fs *resources) error { + _, err := c.Bucket(fs.bucket.Name).Attrs(ctx) + return err + }, + }, + { + name: "storage.Bucket.Create", + resources: []string{}, + call: func(ctx context.Context, c *Client, fs *resources) error { + b := bucketIDs.New() + return c.Bucket(b).Create(ctx, projectID, nil) + }, + }, + { + name: "storage.Bucket.Delete", + resources: []string{"bucket"}, + call: func(ctx context.Context, c *Client, fs *resources) error { + c.Bucket(fs.bucket.Name).Delete(ctx) + return nil + }, + }, + { + name: "storage.Bucket.Update", + resources: []string{"bucket"}, + call: func(ctx context.Context, c *Client, fs *resources) error { + uattrs := BucketAttrsToUpdate{StorageClass: "ARCHIVE"} + bkt := c.Bucket(fs.bucket.Name) + _, err := bkt.Update(ctx, uattrs) + return err + }, + }, + { + name: "storage.Object.Attrs", + resources: []string{"bucket", "object"}, + call: func(ctx context.Context, c *Client, fs *resources) error { + _, err := c.Bucket(fs.bucket.Name).Object(fs.object.Name).Attrs(ctx) + return err + }, + }, + { + name: "storage.ACL.List", + resources: []string{"bucket"}, + call: func(ctx context.Context, c *Client, fs *resources) error { + _, err := c.Bucket(fs.bucket.Name).ACL().List(ctx) + return err + }, + }, + { + name: "storage.ACL.Set", + resources: []string{"bucket"}, + call: func(ctx context.Context, c *Client, fs *resources) error { + return c.Bucket(fs.bucket.Name).ACL().Set(ctx, AllAuthenticatedUsers, RoleOwner) + }, + }, + } { + t.Run(c.name, func(t *testing.T) { + // Create the test resources. + subtest := &emulatedTraceTest{} + subtest.populateResources(ctx, veneerClient, c.resources) // TODO: Remove setting development env var upon launch. t.Setenv("GO_STORAGE_DEV_OTEL_TRACING", "true") + // Configure the tracer provider and in-memory exporter. - ctx := context.Background() e := tracetest.NewInMemoryExporter() tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(e)) defer tp.Shutdown(ctx) otel.SetTracerProvider(tp) - // Run the library method that has trace instrumentation. - err = fn(ctx, subtest.transportClient, &subtest.resources) + // Run the library method to test trace instrumentation. + err := c.call(ctx, client, &subtest.resources) if err != nil { - t.Errorf("%v error: %v", subtest.name, err) + t.Errorf("%v error: %v", c.name, err) } // Verify trace spans. - wantSpan := createWantSpanStub(spanName) - subtest.checkOTelTraceSpans(e, wantSpan) + wantSpan := createWantSpanStub(c.name) + checkOTelTraceSpans(t, e, wantSpan) e.Reset() }) } - } -} - -// Creates the transport client used in emulated trace tests. -func (et *emulatedTraceTest) initTransportClient(transport string) { - ctx := context.Background() - // Create transportClient for http or grpc. To test veneer library - // instrumentation, we disable transport layer traces for testing purposes. - opts := []option.ClientOption{ - option.WithTelemetryDisabled(), - } - transportClient, err := NewClient(ctx, opts...) - if err != nil { - et.Fatalf("HTTP transportClient: %v", err) - } - - if transport == "grpc" { - transportClient, err = NewGRPCClient(ctx, opts...) - if err != nil { - et.Fatalf("GRPC transportClient: %v", err) - } - } - // Reduce backoff to get faster test execution. - transportClient.SetRetry(WithBackoff(gax.Backoff{Initial: 10 * time.Millisecond})) - et.transportClient = transportClient + }, opts...) } func createWantSpanStub(spanName string) tracetest.SpanStub { @@ -174,17 +192,17 @@ func createWantSpanStub(spanName string) tracetest.SpanStub { } } -func (et *emulatedTraceTest) checkOTelTraceSpans(e *tracetest.InMemoryExporter, wantSpan tracetest.SpanStub) { +func checkOTelTraceSpans(t *testing.T, e *tracetest.InMemoryExporter, wantSpan tracetest.SpanStub) { spans := e.GetSpans() if len(spans) == 0 { - et.Errorf("Wanted trace spans, got none") + t.Errorf("Wanted trace spans, got none") } opts := []cmp.Option{ cmp.Comparer(spanAttributesComparer), } for _, span := range spans { if diff := testutil.Diff(span, wantSpan, opts...); diff != "" { - et.Errorf("diff: -got, +want:\n%s\n", diff) + t.Errorf("diff: -got, +want:\n%s\n", diff) } } } @@ -245,58 +263,3 @@ func (et *emulatedTraceTest) populateResources(ctx context.Context, c *Client, r } } } - -// traceMethods are library methods that have trace instrumentation. This is a map whose keys are -// a string describing the spanName (e.g. storage.Bucket.Attrs) and values are functions which -// wrap library methods that implement the API calls. -var traceMethods = map[string]traceFunc{ - // Bucket module - "storage.Bucket.Attrs": func(ctx context.Context, c *Client, fs *resources) error { - _, err := c.Bucket(fs.bucket.Name).Attrs(ctx) - return err - }, - "storage.Bucket.Delete": func(ctx context.Context, c *Client, fs *resources) error { - c.Bucket(fs.bucket.Name).Delete(ctx) - return nil - }, - "storage.Bucket.Create": func(ctx context.Context, c *Client, fs *resources) error { - b := bucketIDs.New() - return c.Bucket(b).Create(ctx, projectID, nil) - }, - "storage.Bucket.Update": func(ctx context.Context, c *Client, fs *resources) error { - uattrs := BucketAttrsToUpdate{StorageClass: "ARCHIVE"} - bkt := c.Bucket(fs.bucket.Name) - _, err := bkt.Update(ctx, uattrs) - return err - }, - // Notifications module - "storage.Bucket.DeleteNotification": func(ctx context.Context, c *Client, fs *resources) error { - return c.Bucket(fs.bucket.Name).DeleteNotification(ctx, fs.notification.ID) - }, - "storage.Bucket.AddNotification": func(ctx context.Context, c *Client, fs *resources) error { - notification := Notification{ - TopicID: "my-topic", - TopicProjectID: projectID, - PayloadFormat: "json", - } - _, err := c.Bucket(fs.bucket.Name).AddNotification(ctx, ¬ification) - return err - }, - "storage.Bucket.Notifications": func(ctx context.Context, c *Client, fs *resources) error { - _, err := c.Bucket(fs.bucket.Name).Notifications(ctx) - return err - }, - // Storage module - "storage.Object.Attrs": func(ctx context.Context, c *Client, fs *resources) error { - _, err := c.Bucket(fs.bucket.Name).Object(fs.object.Name).Attrs(ctx) - return err - }, - // ACL module - "storage.ACL.List": func(ctx context.Context, c *Client, fs *resources) error { - _, err := c.Bucket(fs.bucket.Name).ACL().List(ctx) - return err - }, - "storage.ACL.Set": func(ctx context.Context, c *Client, fs *resources) error { - return c.Bucket(fs.bucket.Name).ACL().Set(ctx, AllAuthenticatedUsers, RoleOwner) - }, -}