Skip to content

Commit

Permalink
refactor tests and review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@cojenco
cojenco committed Nov 15, 2024
1 parent cd7e94c commit 53726f1
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 122 deletions.
5 changes: 5 additions & 0 deletions storage/trace.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -85,3 +86,7 @@ func getCommonTraceOptions() []trace.SpanStartOption {
}
return opts
}

func appendPackageName(spanName string) string {
return "cloud.google.com/go/" + spanName
}
207 changes: 85 additions & 122 deletions storage/trace_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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()
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}
}
}
Expand Down Expand Up @@ -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, &notification)
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)
},
}

0 comments on commit 53726f1

Please sign in to comment.