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

chore(storage): Instrument existing metadata ops with storage trace package #11107

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
13 changes: 6 additions & 7 deletions storage/acl.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package storage
import (
"context"

"cloud.google.com/go/internal/trace"
"cloud.google.com/go/storage/internal/apiv2/storagepb"
raw "google.golang.org/api/storage/v1"
)
Expand Down Expand Up @@ -77,8 +76,8 @@ type ACLHandle struct {

// Delete permanently deletes the ACL entry for the given entity.
func (a *ACLHandle) Delete(ctx context.Context, entity ACLEntity) (err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.ACL.Delete")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.ACL.Delete")
defer func() { endSpan(ctx, err) }()

if a.object != "" {
return a.objectDelete(ctx, entity)
Expand All @@ -91,8 +90,8 @@ func (a *ACLHandle) Delete(ctx context.Context, entity ACLEntity) (err error) {

// Set sets the role for the given entity.
func (a *ACLHandle) Set(ctx context.Context, entity ACLEntity, role ACLRole) (err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.ACL.Set")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.ACL.Set")
Copy link
Contributor

Choose a reason for hiding this comment

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

So it looks like we're dropping the actual returned Span. I assume we will use this later if we want to start adding more context pieces?

Also, does it make sense to have a separate endSpan method vs. calling Span.End directly on the span that was returned from startSpan? I recognize that you are basically just replicating the internal package pattern currently...

defer func() { endSpan(ctx, err) }()

if a.object != "" {
return a.objectSet(ctx, entity, role, false)
Expand All @@ -105,8 +104,8 @@ func (a *ACLHandle) Set(ctx context.Context, entity ACLEntity, role ACLRole) (er

// List retrieves ACL entries.
func (a *ACLHandle) List(ctx context.Context) (rules []ACLRule, err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.ACL.List")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.ACL.List")
defer func() { endSpan(ctx, err) }()

if a.object != "" {
return a.objectList(ctx)
Expand Down
17 changes: 8 additions & 9 deletions storage/bucket.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (

"cloud.google.com/go/compute/metadata"
"cloud.google.com/go/internal/optional"
"cloud.google.com/go/internal/trace"
"cloud.google.com/go/storage/internal/apiv2/storagepb"
"google.golang.org/api/googleapi"
"google.golang.org/api/iamcredentials/v1"
Expand Down Expand Up @@ -82,8 +81,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) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Create")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.Bucket.Create")
defer func() { endSpan(ctx, err) }()

o := makeStorageOpts(true, b.retry, b.userProject)

Expand All @@ -95,8 +94,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) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Delete")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.Bucket.Delete")
defer func() { endSpan(ctx, err) }()

o := makeStorageOpts(true, b.retry, b.userProject)
return b.c.tc.DeleteBucket(ctx, b.name, b.conds, o...)
Expand Down Expand Up @@ -150,17 +149,17 @@ 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) }()
ctx, _ = startSpan(ctx, "storage.Bucket.Attrs")
defer func() { endSpan(ctx, err) }()

o := makeStorageOpts(true, b.retry, b.userProject)
return b.c.tc.GetBucket(ctx, b.name, b.conds, o...)
}

// 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) }()
ctx, _ = startSpan(ctx, "storage.Bucket.Update")
defer func() { endSpan(ctx, err) }()

isIdempotent := b.conds != nil && b.conds.MetagenerationMatch != 0
o := makeStorageOpts(isIdempotent, b.retry, b.userProject)
Expand Down
2 changes: 1 addition & 1 deletion storage/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
9 changes: 0 additions & 9 deletions storage/http_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -1235,9 +1235,6 @@ func (c *httpStorageClient) DeleteHMACKey(ctx context.Context, project string, a
// Note: This API does not support pagination. However, entity limits cap the number of notifications on a single bucket,
// so all results will be returned in the first response. See https://cloud.google.com/storage/quotas#buckets.
func (c *httpStorageClient) ListNotifications(ctx context.Context, bucket string, opts ...storageOption) (n map[string]*Notification, err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.httpStorageClient.ListNotifications")
defer func() { trace.EndSpan(ctx, err) }()

s := callSettings(c.settings, opts...)
call := c.raw.Notifications.List(bucket)
if s.userProject != "" {
Expand All @@ -1255,9 +1252,6 @@ func (c *httpStorageClient) ListNotifications(ctx context.Context, bucket string
}

func (c *httpStorageClient) CreateNotification(ctx context.Context, bucket string, n *Notification, opts ...storageOption) (ret *Notification, err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.httpStorageClient.CreateNotification")
defer func() { trace.EndSpan(ctx, err) }()

s := callSettings(c.settings, opts...)
call := c.raw.Notifications.Insert(bucket, toRawNotification(n))
if s.userProject != "" {
Expand All @@ -1275,9 +1269,6 @@ func (c *httpStorageClient) CreateNotification(ctx context.Context, bucket strin
}

func (c *httpStorageClient) DeleteNotification(ctx context.Context, bucket string, id string, opts ...storageOption) (err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.httpStorageClient.DeleteNotification")
defer func() { trace.EndSpan(ctx, err) }()

s := callSettings(c.settings, opts...)
call := c.raw.Notifications.Delete(bucket, id)
if s.userProject != "" {
Expand Down
13 changes: 6 additions & 7 deletions storage/iam.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (

"cloud.google.com/go/iam"
"cloud.google.com/go/iam/apiv1/iampb"
"cloud.google.com/go/internal/trace"
raw "google.golang.org/api/storage/v1"
"google.golang.org/genproto/googleapis/type/expr"
)
Expand All @@ -45,25 +44,25 @@ func (c *iamClient) Get(ctx context.Context, resource string) (p *iampb.Policy,
}

func (c *iamClient) GetWithVersion(ctx context.Context, resource string, requestedPolicyVersion int32) (p *iampb.Policy, err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.IAM.Get")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.IAM.Get")
defer func() { endSpan(ctx, err) }()

o := makeStorageOpts(true, c.retry, c.userProject)
return c.client.tc.GetIamPolicy(ctx, resource, requestedPolicyVersion, o...)
}

func (c *iamClient) Set(ctx context.Context, resource string, p *iampb.Policy) (err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.IAM.Set")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.IAM.Set")
defer func() { endSpan(ctx, err) }()

isIdempotent := len(p.Etag) > 0
o := makeStorageOpts(isIdempotent, c.retry, c.userProject)
return c.client.tc.SetIamPolicy(ctx, resource, p, o...)
}

func (c *iamClient) Test(ctx context.Context, resource string, perms []string) (permissions []string, err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.IAM.Test")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.IAM.Test")
defer func() { endSpan(ctx, err) }()

o := makeStorageOpts(true, c.retry, c.userProject)
return c.client.tc.TestIamPermissions(ctx, resource, perms, o...)
Expand Down
13 changes: 6 additions & 7 deletions storage/notifications.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"fmt"
"regexp"

"cloud.google.com/go/internal/trace"
raw "google.golang.org/api/storage/v1"
)

Expand Down Expand Up @@ -121,8 +120,8 @@ func toRawNotification(n *Notification) *raw.Notification {
// returned Notification's ID can be used to refer to it.
// Note: gRPC is not supported.
func (b *BucketHandle) AddNotification(ctx context.Context, n *Notification) (ret *Notification, err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.AddNotification")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.Bucket.AddNotification")
defer func() { endSpan(ctx, err) }()

if n.ID != "" {
return nil, errors.New("storage: AddNotification: ID must not be set")
Expand All @@ -143,8 +142,8 @@ func (b *BucketHandle) AddNotification(ctx context.Context, n *Notification) (re
// indexed by notification ID.
// Note: gRPC is not supported.
func (b *BucketHandle) Notifications(ctx context.Context) (n map[string]*Notification, err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.Notifications")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.Bucket.Notifications")
defer func() { endSpan(ctx, err) }()

opts := makeStorageOpts(true, b.retry, b.userProject)
n, err = b.c.tc.ListNotifications(ctx, b.name, opts...)
Expand All @@ -162,8 +161,8 @@ func notificationsToMap(rns []*raw.Notification) map[string]*Notification {
// DeleteNotification deletes the notification with the given ID.
// Note: gRPC is not supported.
func (b *BucketHandle) DeleteNotification(ctx context.Context, id string) (err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Bucket.DeleteNotification")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.Bucket.DeleteNotification")
defer func() { endSpan(ctx, err) }()

opts := makeStorageOpts(true, b.retry, b.userProject)
return b.c.tc.DeleteNotification(ctx, b.name, id, opts...)
Expand Down
8 changes: 4 additions & 4 deletions storage/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -938,8 +938,8 @@ func (o *ObjectHandle) Key(encryptionKey []byte) *ObjectHandle {
// Attrs returns meta information about the object.
// ErrObjectNotExist will be returned if the object is not found.
func (o *ObjectHandle) Attrs(ctx context.Context) (attrs *ObjectAttrs, err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Object.Attrs")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.Object.Attrs")
defer func() { endSpan(ctx, err) }()

if err := o.validate(); err != nil {
return nil, err
Expand All @@ -952,8 +952,8 @@ func (o *ObjectHandle) Attrs(ctx context.Context) (attrs *ObjectAttrs, err error
// ObjectAttrsToUpdate docs for details on treatment of zero values.
// ErrObjectNotExist will be returned if the object is not found.
func (o *ObjectHandle) Update(ctx context.Context, uattrs ObjectAttrsToUpdate) (oa *ObjectAttrs, err error) {
ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.Object.Update")
defer func() { trace.EndSpan(ctx, err) }()
ctx, _ = startSpan(ctx, "storage.Object.Update")
defer func() { endSpan(ctx, err) }()

if err := o.validate(); err != nil {
return nil, err
Expand Down
87 changes: 87 additions & 0 deletions storage/trace.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// 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"

internalTrace "cloud.google.com/go/internal/trace"
"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 (
storageOtelTracingDevVar = "GO_STORAGE_DEV_OTEL_TRACING"
defaultTracerName = "cloud.google.com/go/storage"
gcpClientRepo = "googleapis/google-cloud-go"
gcpClientArtifact = "cloud.google.com/go/storage"
)

// isOTelTracingDevEnabled checks the development flag until experimental feature is launched.
// TODO: Remove development flag upon experimental launch.
func isOTelTracingDevEnabled() bool {
return os.Getenv(storageOtelTracingDevVar) == "true"
}

func tracer() trace.Tracer {
return otel.Tracer(defaultTracerName, trace.WithInstrumentationVersion(internal.Version))
}

// startSpan creates a span and a context.Context containing the newly-created span.
// If the context.Context provided in `ctx` contains a span then the newly-created
// span will be a child of that span, otherwise it will be a root span.
func startSpan(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) {
// TODO: Remove internalTrace upon experimental launch.
if !isOTelTracingDevEnabled() {
ctx = internalTrace.StartSpan(ctx, name)
return ctx, nil
}
opts = append(opts, getCommonTraceOptions()...)
ctx, span := tracer().Start(ctx, name, opts...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear, we are leaving out cloud.google.com/go/ from the trace name?

I'd suggest at least adding it back to the internalTrace implementation.

return ctx, span
}

// endSpan retrieves the current span from ctx and completes the span.
// If an error occurs, the error is recorded as an exception span event for this span,
// and the span status is set in the form of a code and a description.
func endSpan(ctx context.Context, err error) {
// TODO: Remove internalTrace upon experimental launch.
if !isOTelTracingDevEnabled() {
internalTrace.EndSpan(ctx, err)
} else {
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
}
Loading
Loading