Skip to content

Commit

Permalink
internal: create internal/trace
Browse files Browse the repository at this point in the history 
- Create internal/trace for starting and stopping spans
- Refactor storage and datastore to use internal/trace instead of their own impls
- Document source for http errors

Change-Id: Ida23b9bb37d0094a9c4e5d43699eb7e53c8e2d27
Reviewed-on: https://code-review.googlesource.com/24731
Reviewed-by: kokoro <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>
  • Loading branch information
@jeanbza
jeanbza committed Mar 7, 2018
1 parent dcdccb2 commit ff2b4e2
Show file tree
Hide file tree
Showing 14 changed files with 158 additions and 187 deletions.
21 changes: 11 additions & 10 deletions datastore/datastore.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"os"
"reflect"

"cloud.google.com/go/internal/trace"
"golang.org/x/net/context"
"google.golang.org/api/option"
gtransport "google.golang.org/api/transport/grpc"
Expand Down Expand Up @@ -303,8 +304,8 @@ func (c *Client) Close() error {
// unexported in the destination struct. ErrFieldMismatch is only returned if
// dst is a struct pointer.
func (c *Client) Get(ctx context.Context, key *Key, dst interface{}) (err error) {
traceStartSpan(ctx, "cloud.google.com/go/datastore.Get")
defer func() { traceEndSpan(ctx, err) }()
trace.StartSpan(ctx, "cloud.google.com/go/datastore.Get")
defer func() { trace.EndSpan(ctx, err) }()

if dst == nil { // get catches nil interfaces; we need to catch nil ptr here
return ErrInvalidEntityType
Expand All @@ -327,8 +328,8 @@ func (c *Client) Get(ctx context.Context, key *Key, dst interface{}) (err error)
// PropertyList is a slice of structs. It is treated as invalid to avoid being
// mistakenly passed when []PropertyList was intended.
func (c *Client) GetMulti(ctx context.Context, keys []*Key, dst interface{}) (err error) {
traceStartSpan(ctx, "cloud.google.com/go/datastore.GetMulti")
defer func() { traceEndSpan(ctx, err) }()
trace.StartSpan(ctx, "cloud.google.com/go/datastore.GetMulti")
defer func() { trace.EndSpan(ctx, err) }()

return c.get(ctx, keys, dst, nil)
}
Expand Down Expand Up @@ -460,8 +461,8 @@ func (c *Client) Put(ctx context.Context, key *Key, src interface{}) (*Key, erro
// src must satisfy the same conditions as the dst argument to GetMulti.
// TODO(jba): rewrite in terms of Mutate.
func (c *Client) PutMulti(ctx context.Context, keys []*Key, src interface{}) (_ []*Key, err error) {
traceStartSpan(ctx, "cloud.google.com/go/datastore.PutMulti")
defer func() { traceEndSpan(ctx, err) }()
trace.StartSpan(ctx, "cloud.google.com/go/datastore.PutMulti")
defer func() { trace.EndSpan(ctx, err) }()

mutations, err := putMutations(keys, src)
if err != nil {
Expand Down Expand Up @@ -552,8 +553,8 @@ func (c *Client) Delete(ctx context.Context, key *Key) error {
// DeleteMulti is a batch version of Delete.
// TODO(jba): rewrite in terms of Mutate.
func (c *Client) DeleteMulti(ctx context.Context, keys []*Key) (err error) {
traceStartSpan(ctx, "cloud.google.com/go/datastore.DeleteMulti")
defer func() { traceEndSpan(ctx, err) }()
trace.StartSpan(ctx, "cloud.google.com/go/datastore.DeleteMulti")
defer func() { trace.EndSpan(ctx, err) }()

mutations, err := deleteMutations(keys)
if err != nil {
Expand Down Expand Up @@ -593,8 +594,8 @@ func deleteMutations(keys []*Key) ([]*pb.Mutation, error) {
// If any of the mutations are invalid, Mutate returns a MultiError with the errors.
// Mutate returns a MultiError in this case even if there is only one Mutation.
func (c *Client) Mutate(ctx context.Context, muts ...*Mutation) (_ []*Key, err error) {
traceStartSpan(ctx, "cloud.google.com/go/datastore.Mutate")
defer func() { traceEndSpan(ctx, err) }()
trace.StartSpan(ctx, "cloud.google.com/go/datastore.Mutate")
defer func() { trace.EndSpan(ctx, err) }()

pmuts, err := mutationProtos(muts)
if err != nil {
Expand Down
72 changes: 0 additions & 72 deletions datastore/go18.go
View file

This file was deleted.

30 changes: 0 additions & 30 deletions datastore/not_go18.go
View file

This file was deleted.

17 changes: 9 additions & 8 deletions datastore/query.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"strconv"
"strings"

"cloud.google.com/go/internal/trace"
wrapperspb "github.com/golang/protobuf/ptypes/wrappers"
"golang.org/x/net/context"
"google.golang.org/api/iterator"
Expand Down Expand Up @@ -446,8 +447,8 @@ func (q *Query) toProto(req *pb.RunQueryRequest) error {
// expected to be small, it is best to specify a limit; otherwise Count will
// continue until it finishes counting or the provided context expires.
func (c *Client) Count(ctx context.Context, q *Query) (_ int, err error) {
traceStartSpan(ctx, "cloud.google.com/go/datastore.Query.Count")
defer func() { traceEndSpan(ctx, err) }()
trace.StartSpan(ctx, "cloud.google.com/go/datastore.Query.Count")
defer func() { trace.EndSpan(ctx, err) }()

// Check that the query is well-formed.
if q.err != nil {
Expand Down Expand Up @@ -496,8 +497,8 @@ func (c *Client) Count(ctx context.Context, q *Query) (_ int, err error) {
// continue until it finishes collecting results or the provided context
// expires.
func (c *Client) GetAll(ctx context.Context, q *Query, dst interface{}) (_ []*Key, err error) {
traceStartSpan(ctx, "cloud.google.com/go/datastore.Query.GetAll")
defer func() { traceEndSpan(ctx, err) }()
trace.StartSpan(ctx, "cloud.google.com/go/datastore.Query.GetAll")
defer func() { trace.EndSpan(ctx, err) }()

var (
dv reflect.Value
Expand Down Expand Up @@ -582,8 +583,8 @@ func (c *Client) Run(ctx context.Context, q *Query) *Iterator {
},
}

traceStartSpan(ctx, "cloud.google.com/go/datastore.Query.Run")
defer func() { traceEndSpan(ctx, t.err) }()
trace.StartSpan(ctx, "cloud.google.com/go/datastore.Query.Run")
defer func() { trace.EndSpan(ctx, t.err) }()
if q.namespace != "" {
t.req.PartitionId = &pb.PartitionId{
NamespaceId: q.namespace,
Expand Down Expand Up @@ -735,8 +736,8 @@ func (t *Iterator) nextBatch() error {

// Cursor returns a cursor for the iterator's current location.
func (t *Iterator) Cursor() (_ Cursor, err error) {
traceStartSpan(t.ctx, "cloud.google.com/go/datastore.Query.Cursor")
defer func() { traceEndSpan(t.ctx, err) }()
trace.StartSpan(t.ctx, "cloud.google.com/go/datastore.Query.Cursor")
defer func() { trace.EndSpan(t.ctx, err) }()

// If there is still an offset, we need to the skip those results first.
for t.err == nil && t.offset > 0 {
Expand Down
33 changes: 17 additions & 16 deletions datastore/transaction.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package datastore
import (
"errors"

"cloud.google.com/go/internal/trace"
"golang.org/x/net/context"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
Expand Down Expand Up @@ -96,8 +97,8 @@ type Transaction struct {

// NewTransaction starts a new transaction.
func (c *Client) NewTransaction(ctx context.Context, opts ...TransactionOption) (_ *Transaction, err error) {
traceStartSpan(ctx, "cloud.google.com/go/datastore.NewTransaction")
defer func() { traceEndSpan(ctx, err) }()
trace.StartSpan(ctx, "cloud.google.com/go/datastore.NewTransaction")
defer func() { trace.EndSpan(ctx, err) }()

for _, o := range opts {
if _, ok := o.(maxAttempts); ok {
Expand Down Expand Up @@ -156,8 +157,8 @@ func (c *Client) newTransaction(ctx context.Context, s *transactionSettings) (*T
// Transaction.Get will append when unmarshalling slice fields, so it is not
// necessarily idempotent.
func (c *Client) RunInTransaction(ctx context.Context, f func(tx *Transaction) error, opts ...TransactionOption) (_ *Commit, err error) {
traceStartSpan(ctx, "cloud.google.com/go/datastore.RunInTransaction")
defer func() { traceEndSpan(ctx, err) }()
trace.StartSpan(ctx, "cloud.google.com/go/datastore.RunInTransaction")
defer func() { trace.EndSpan(ctx, err) }()

settings := newTransactionSettings(opts)
for n := 0; n < settings.attempts; n++ {
Expand All @@ -183,8 +184,8 @@ func (c *Client) RunInTransaction(ctx context.Context, f func(tx *Transaction) e

// Commit applies the enqueued operations atomically.
func (t *Transaction) Commit() (_ *Commit, err error) {
traceStartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.Commit")
defer func() { traceEndSpan(t.ctx, err) }()
trace.StartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.Commit")
defer func() { trace.EndSpan(t.ctx, err) }()

if t.id == nil {
return nil, errExpiredTransaction
Expand Down Expand Up @@ -223,8 +224,8 @@ func (t *Transaction) Commit() (_ *Commit, err error) {

// Rollback abandons a pending transaction.
func (t *Transaction) Rollback() (err error) {
traceStartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.Rollback")
defer func() { traceEndSpan(t.ctx, err) }()
trace.StartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.Rollback")
defer func() { trace.EndSpan(t.ctx, err) }()

if t.id == nil {
return errExpiredTransaction
Expand All @@ -244,8 +245,8 @@ func (t *Transaction) Rollback() (err error) {
// level, another transaction cannot concurrently modify the data that is read
// or modified by this transaction.
func (t *Transaction) Get(key *Key, dst interface{}) (err error) {
traceStartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.Get")
defer func() { traceEndSpan(t.ctx, err) }()
trace.StartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.Get")
defer func() { trace.EndSpan(t.ctx, err) }()

opts := &pb.ReadOptions{
ConsistencyType: &pb.ReadOptions_Transaction{Transaction: t.id},
Expand All @@ -259,8 +260,8 @@ func (t *Transaction) Get(key *Key, dst interface{}) (err error) {

// GetMulti is a batch version of Get.
func (t *Transaction) GetMulti(keys []*Key, dst interface{}) (err error) {
traceStartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.GetMulti")
defer func() { traceEndSpan(t.ctx, err) }()
trace.StartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.GetMulti")
defer func() { trace.EndSpan(t.ctx, err) }()

if t.id == nil {
return errExpiredTransaction
Expand Down Expand Up @@ -292,8 +293,8 @@ func (t *Transaction) Put(key *Key, src interface{}) (*PendingKey, error) {
// element of src in the same order.
// TODO(jba): rewrite in terms of Mutate.
func (t *Transaction) PutMulti(keys []*Key, src interface{}) (_ []*PendingKey, err error) {
traceStartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.PutMulti")
defer func() { traceEndSpan(t.ctx, err) }()
trace.StartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.PutMulti")
defer func() { trace.EndSpan(t.ctx, err) }()

if t.id == nil {
return nil, errExpiredTransaction
Expand Down Expand Up @@ -335,8 +336,8 @@ func (t *Transaction) Delete(key *Key) error {
// DeleteMulti is a batch version of Delete.
// TODO(jba): rewrite in terms of Mutate.
func (t *Transaction) DeleteMulti(keys []*Key) (err error) {
traceStartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.DeleteMulti")
defer func() { traceEndSpan(t.ctx, err) }()
trace.StartSpan(t.ctx, "cloud.google.com/go/datastore.Transaction.DeleteMulti")
defer func() { trace.EndSpan(t.ctx, err) }()

if t.id == nil {
return errExpiredTransaction
Expand Down
29 changes: 19 additions & 10 deletions storage/go18.go → internal/trace/go18.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,34 +14,43 @@

// +build go1.8

package storage
package trace

import (
"go.opencensus.io/trace"
"golang.org/x/net/context"
"google.golang.org/api/googleapi"
"google.golang.org/genproto/googleapis/rpc/code"
"google.golang.org/grpc/status"
)

func traceStartSpan(ctx context.Context, name string) context.Context {
func StartSpan(ctx context.Context, name string) context.Context {
ctx, _ = trace.StartSpan(ctx, name)
return ctx
}

func traceEndSpan(ctx context.Context, err error) {
func EndSpan(ctx context.Context, err error) {
span := trace.FromContext(ctx)
if err != nil {
if err2, ok := err.(*googleapi.Error); ok {
span.SetStatus(trace.Status{Code: httpStatusCodeToOCCode(err2.Code), Message: err2.Message})
} else if err == ErrBucketNotExist || err == ErrObjectNotExist {
span.SetStatus(trace.Status{Code: int32(code.Code_NOT_FOUND), Message: err.Error()})
} else {
span.SetStatus(trace.Status{Code: int32(code.Code_UNKNOWN), Message: err.Error()})
}
span.SetStatus(toStatus(err))
}
span.End()
}

// ToStatus interrogates an error and converts it to an appropriate
// OpenCensus status.
func toStatus(err error) trace.Status {
if err2, ok := err.(*googleapi.Error); ok {
return trace.Status{Code: httpStatusCodeToOCCode(err2.Code), Message: err2.Message}
} else if s, ok := status.FromError(err); ok {
return trace.Status{Code: int32(s.Code()), Message: s.Message()}
} else {
return trace.Status{Code: int32(code.Code_UNKNOWN), Message: err.Error()}
}
}

// TODO (deklerk): switch to using OpenCensus function when it becomes available.
// Reference: https://github.com/googleapis/googleapis/blob/26b634d2724ac5dd30ae0b0cbfb01f07f2e4050e/google/rpc/code.proto
func httpStatusCodeToOCCode(httpStatusCode int) int32 {
switch httpStatusCode {
case 200:
Expand Down
Loading

3 comments on commit ff2b4e2

@sethvargo
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of a package under internal/* breaks under go 1.9+:

vendor/cloud.google.com/go/storage/acl.go:21:2: use of internal package not allowed

@jba
Copy link
Contributor

@jba jba commented on ff2b4e2 Mar 31, 2018

Choose a reason for hiding this comment

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

This is some interaction with vendoring that I don't understand.

@jadekler, @broady can you take a look?

@broady
Copy link
Contributor

@broady broady commented on ff2b4e2 Mar 31, 2018 via email

Choose a reason for hiding this comment

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

Please sign in to comment.