From d9262a3e661d3649aa6278c59a5d967748fdb344 Mon Sep 17 00:00:00 2001 From: Rodrigo Arguello Date: Mon, 12 Aug 2024 10:31:26 +0200 Subject: [PATCH] address comments --- contrib/gocql/gocql/example_test.go | 54 ++++++++-------------------- contrib/gocql/gocql/gocql.go | 20 +++++------ contrib/gocql/gocql/observer.go | 21 +++-------- contrib/gocql/gocql/observer_test.go | 28 ++++++++------- contrib/gocql/gocql/option.go | 6 ++-- 5 files changed, 47 insertions(+), 82 deletions(-) diff --git a/contrib/gocql/gocql/example_test.go b/contrib/gocql/gocql/example_test.go index 6a0793e231..34982b5ab3 100644 --- a/contrib/gocql/gocql/example_test.go +++ b/contrib/gocql/gocql/example_test.go @@ -38,12 +38,12 @@ func ExampleNewCluster() { query.Exec() } -func ExampleNewTracedSession() { +func ExampleCreateTracedSession() { cluster := gocql.NewCluster("127.0.0.1:9042") cluster.Keyspace = "my-keyspace" // Create a new traced session using any number of options - session, err := gocqltrace.NewTracedSession(cluster, gocqltrace.WithServiceName("ServiceName")) + session, err := gocqltrace.CreateTracedSession(cluster, gocqltrace.WithServiceName("ServiceName")) if err != nil { log.Fatal(err) } @@ -57,13 +57,15 @@ func ExampleNewTracedSession() { ) query.WithContext(ctx) + // If you don't want a concrete query to be traced, you can do query.Observer(nil) + // Finally, execute the query if err := query.Exec(); err != nil { log.Fatal(err) } } -func ExampleTraceQuery() { +func ExampleNewObserver() { cluster := gocql.NewCluster("127.0.0.1:9042") cluster.Keyspace = "my-keyspace" @@ -72,7 +74,12 @@ func ExampleTraceQuery() { if err != nil { log.Fatal(err) } - query := session.Query("CREATE KEYSPACE if not exists trace WITH REPLICATION = { 'class' : 'SimpleStrategy', 'replication_factor': 1}") + // Create a new observer using same set of options as gocqltrace.CreateTracedSession. + obs := gocqltrace.NewObserver(cluster, gocqltrace.WithServiceName("ServiceName")) + + // Attach the observer to queries / batches individually. + tracedQuery := session.Query("SELECT something FROM somewhere").Observer(obs) + untracedQuery := session.Query("SELECT something FROM somewhere") // Use context to pass information down the call chain _, ctx := tracer.StartSpanFromContext(context.Background(), "parent.request", @@ -80,46 +87,13 @@ func ExampleTraceQuery() { tracer.ServiceName("web"), tracer.ResourceName("/home"), ) - query.WithContext(ctx) - - // Enable tracing this query only. - query = gocqltrace.TraceQuery(query, cluster, gocqltrace.WithServiceName("ServiceName")) + tracedQuery.WithContext(ctx) // Finally, execute the query - if err := query.Exec(); err != nil { - log.Fatal(err) - } -} - -func ExampleTraceBatch() { - cluster := gocql.NewCluster("127.0.0.1:9042") - cluster.Keyspace = "my-keyspace" - - // Create a new regular gocql session - session, err := cluster.CreateSession() - if err != nil { + if err := tracedQuery.Exec(); err != nil { log.Fatal(err) } - - // Create a new regular gocql batch and add some queries to it - stmt := "INSERT INTO trace.person (name, age, description) VALUES (?, ?, ?)" - batch := session.NewBatch(gocql.UnloggedBatch) - batch.Query(stmt, "Kate", 80, "Cassandra's sister running in kubernetes") - batch.Query(stmt, "Lucas", 60, "Another person") - - // Use context to pass information down the call chain - _, ctx := tracer.StartSpanFromContext(context.Background(), "parent.request", - tracer.SpanType(ext.SpanTypeCassandra), - tracer.ServiceName("web"), - tracer.ResourceName("/home"), - ) - batch.WithContext(ctx) - - // Enable tracing this batch only - batch = gocqltrace.TraceBatch(batch, cluster, gocqltrace.WithServiceName("ServiceName")) - - // Finally, execute the batch - if err := session.ExecuteBatch(batch); err != nil { + if err := untracedQuery.Exec(); err != nil { log.Fatal(err) } } diff --git a/contrib/gocql/gocql/gocql.go b/contrib/gocql/gocql/gocql.go index 1730b99de0..058c557109 100644 --- a/contrib/gocql/gocql/gocql.go +++ b/contrib/gocql/gocql/gocql.go @@ -32,7 +32,7 @@ func init() { // ClusterConfig embeds gocql.ClusterConfig and keeps information relevant to tracing. // -// Deprecated: use the Observer based methods NewTracedSession, TraceQuery or TraceBatch instead, which returns +// Deprecated: use the Observer based method CreateTracedSession instead, which allows to use // native gocql types instead of wrapped types. type ClusterConfig struct { *gocql.ClusterConfig @@ -42,7 +42,7 @@ type ClusterConfig struct { // NewCluster calls gocql.NewCluster and returns a wrapped instrumented version of it. // -// Deprecated: use the Observer based methods NewTracedSession, TraceQuery or TraceBatch instead, which returns +// Deprecated: use the Observer based method CreateTracedSession instead, which allows to use // native gocql types instead of wrapped types. func NewCluster(hosts []string, opts ...WrapOption) *ClusterConfig { return &ClusterConfig{ @@ -54,7 +54,7 @@ func NewCluster(hosts []string, opts ...WrapOption) *ClusterConfig { // Session embeds gocql.Session and keeps information relevant to tracing. // -// Deprecated: use the Observer based methods NewTracedSession, TraceQuery or TraceBatch instead, which returns +// Deprecated: use the Observer based method CreateTracedSession instead, which allows to use // native gocql types instead of wrapped types. type Session struct { *gocql.Session @@ -77,7 +77,7 @@ func (c *ClusterConfig) CreateSession() (*Session, error) { // Query inherits from gocql.Query, it keeps the tracer and the context. // -// Deprecated: use the Observer based methods NewTracedSession, TraceQuery or TraceBatch instead, which returns +// Deprecated: use the Observer based method CreateTracedSession instead, which allows to use // native gocql types instead of wrapped types. type Query struct { *gocql.Query @@ -93,7 +93,7 @@ func (s *Session) Query(stmt string, values ...interface{}) *Query { // Batch inherits from gocql.Batch, it keeps the tracer and the context. // -// Deprecated: use the Observer based methods NewTracedSession, TraceQuery or TraceBatch instead, which returns +// Deprecated: use the Observer based method CreateTracedSession instead, which allows to use // native gocql types instead of wrapped types. type Batch struct { *gocql.Batch @@ -103,7 +103,7 @@ type Batch struct { // NewBatch calls the underlying gocql.Session's NewBatch method and returns a new Batch augmented with tracing. // -// Deprecated: use the Observer based methods NewTracedSession, TraceQuery or TraceBatch instead, which returns +// Deprecated: use the Observer based method CreateTracedSession instead, which allows to use // native gocql types instead of wrapped types. func (s *Session) NewBatch(typ gocql.BatchType) *Batch { b := s.Session.NewBatch(typ) @@ -133,7 +133,7 @@ type params struct { // of `WithContext` and `PageState` but not that of `Consistency`, `Trace`, // `Observer`, etc. // -// Deprecated: use the Observer based methods NewTracedSession, TraceQuery or TraceBatch instead, which returns +// Deprecated: use the Observer based method CreateTracedSession instead, which allows to use // native gocql types instead of wrapped types. func WrapQuery(q *gocql.Query, opts ...WrapOption) *Query { return wrapQuery(q, nil, opts...) @@ -219,7 +219,7 @@ func (tq *Query) ScanCAS(dest ...interface{}) (applied bool, err error) { // Iter inherits from gocql.Iter and contains a span. // -// Deprecated: use the Observer based methods NewTracedSession, TraceQuery or TraceBatch instead, which returns +// Deprecated: use the Observer based method CreateTracedSession instead, which allows to use // native gocql types instead of wrapped types. type Iter struct { *gocql.Iter @@ -266,7 +266,7 @@ func (tIter *Iter) Close() error { // Scanner inherits from a gocql.Scanner derived from an Iter. // -// Deprecated: use the Observer based methods NewTracedSession, TraceQuery or TraceBatch instead, which returns +// Deprecated: use the Observer based method CreateTracedSession instead, which allows to use // native gocql types instead of wrapped types. type Scanner struct { gocql.Scanner @@ -303,7 +303,7 @@ func (s *Scanner) Err() error { // of `WithContext` and `WithTimestamp` but not that of `SerialConsistency`, `Trace`, // `Observer`, etc. // -// Deprecated: use the Observer based methods NewTracedSession, TraceQuery or TraceBatch instead, which returns +// Deprecated: use the Observer based method CreateTracedSession instead, which allows to use // native gocql types instead of wrapped types. func WrapBatch(b *gocql.Batch, opts ...WrapOption) *Batch { return wrapBatch(b, nil, opts...) diff --git a/contrib/gocql/gocql/observer.go b/contrib/gocql/gocql/observer.go index f563db71ac..a75d4c8b16 100644 --- a/contrib/gocql/gocql/observer.go +++ b/contrib/gocql/gocql/observer.go @@ -7,7 +7,6 @@ package gocql import ( "context" - "strconv" "strings" "github.com/gocql/gocql" @@ -16,8 +15,8 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" ) -// NewTracedSession returns a new session augmented with tracing. -func NewTracedSession(cluster *gocql.ClusterConfig, opts ...WrapOption) (*gocql.Session, error) { +// CreateTracedSession returns a new session augmented with tracing. +func CreateTracedSession(cluster *gocql.ClusterConfig, opts ...WrapOption) (*gocql.Session, error) { obs := NewObserver(cluster, opts...) cfg := obs.cfg @@ -33,19 +32,9 @@ func NewTracedSession(cluster *gocql.ClusterConfig, opts ...WrapOption) (*gocql. return cluster.CreateSession() } -// TraceQuery can be used to enable tracing an individual *gocql.Query. -func TraceQuery(q *gocql.Query, cluster *gocql.ClusterConfig, opts ...WrapOption) *gocql.Query { - obs := NewObserver(cluster, opts...) - return q.Observer(obs) -} - -// TraceBatch can be used to enable tracing for an individual *gocql.Batch. -func TraceBatch(b *gocql.Batch, cluster *gocql.ClusterConfig, opts ...WrapOption) *gocql.Batch { - obs := NewObserver(cluster, opts...) - return b.Observer(obs) -} - // NewObserver creates a new Observer to trace gocql. +// This method is useful in case you want to attach the observer to individual traces / batches instead of instrumenting +// the whole client. func NewObserver(cluster *gocql.ClusterConfig, opts ...WrapOption) *Observer { cfg := defaultConfig() for _, fn := range opts { @@ -86,7 +75,7 @@ func (o *Observer) ObserveQuery(ctx context.Context, query gocql.ObservedQuery) resource = query.Statement } span.SetTag(ext.ResourceName, resource) - span.SetTag(ext.CassandraRowCount, strconv.Itoa(query.Rows)) + span.SetTag(ext.CassandraRowCount, query.Rows) finishSpan(span, query.Err, p) } diff --git a/contrib/gocql/gocql/observer_test.go b/contrib/gocql/gocql/observer_test.go index ac71c2092e..148dd3d1bb 100644 --- a/contrib/gocql/gocql/observer_test.go +++ b/contrib/gocql/gocql/observer_test.go @@ -25,13 +25,14 @@ func TestObserver_Query(t *testing.T) { updateQuery func(cluster *gocql.ClusterConfig, sess *gocql.Session, q *gocql.Query) *gocql.Query wantServiceName string wantResourceName string - wantRowCount string + wantRowCount int wantErr bool wantErrTag bool }{ { - name: "default", - opts: nil, + name: "default", + opts: nil, + wantRowCount: 1, }, { name: "service_and_resource_name", @@ -39,6 +40,7 @@ func TestObserver_Query(t *testing.T) { WithServiceName("test-service"), WithResourceName("test-resource"), }, + wantRowCount: 1, wantServiceName: "test-service", wantResourceName: "test-resource", }, @@ -51,7 +53,7 @@ func TestObserver_Query(t *testing.T) { }, wantServiceName: "", wantResourceName: "SELECT name, age FRM trace.person WHERE name = 'This does not exist'", - wantRowCount: "0", + wantRowCount: 0, wantErr: true, wantErrTag: true, }, @@ -68,7 +70,7 @@ func TestObserver_Query(t *testing.T) { }, wantServiceName: "", wantResourceName: "SELECT name, age FRM trace.person WHERE name = 'This does not exist'", - wantRowCount: "0", + wantRowCount: 0, wantErr: true, wantErrTag: false, }, @@ -78,10 +80,12 @@ func TestObserver_Query(t *testing.T) { WithTraceQuery(false), }, updateQuery: func(cluster *gocql.ClusterConfig, _ *gocql.Session, q *gocql.Query) *gocql.Query { - return TraceQuery(q, cluster, WithResourceName("test resource"), WithServiceName("test service")) + obs := NewObserver(cluster, WithResourceName("test resource"), WithServiceName("test service")) + return q.Observer(obs) }, wantServiceName: "test service", wantResourceName: "test resource", + wantRowCount: 1, wantErr: false, wantErrTag: false, }, @@ -102,7 +106,7 @@ func TestObserver_Query(t *testing.T) { WithTraceConnect(false), } opts = append(opts, tc.opts...) - sess, err := NewTracedSession(cluster, opts...) + sess, err := CreateTracedSession(cluster, opts...) require.NoError(t, err) p, ctx := tracer.StartSpanFromContext(context.Background(), "parentSpan") @@ -135,9 +139,6 @@ func TestObserver_Query(t *testing.T) { wantResource = stmt } wantRowCount := tc.wantRowCount - if wantRowCount == "" { - wantRowCount = "1" - } parentSpan := spans[1] querySpan := spans[0] @@ -222,7 +223,8 @@ func TestObserver_Batch(t *testing.T) { WithTraceBatch(false), }, updateBatch: func(cluster *gocql.ClusterConfig, _ *gocql.Session, b *gocql.Batch) *gocql.Batch { - return TraceBatch(b, cluster, WithResourceName("test resource"), WithServiceName("test service")) + obs := NewObserver(cluster, WithResourceName("test resource"), WithServiceName("test service")) + return b.Observer(obs) }, wantServiceName: "test service", wantResourceName: "test resource", @@ -246,7 +248,7 @@ func TestObserver_Batch(t *testing.T) { WithTraceConnect(false), } opts = append(opts, tc.opts...) - sess, err := NewTracedSession(cluster, opts...) + sess, err := CreateTracedSession(cluster, opts...) require.NoError(t, err) p, ctx := tracer.StartSpanFromContext(context.Background(), "parentSpan") @@ -343,7 +345,7 @@ func TestObserver_Connect(t *testing.T) { WithTraceConnect(true), } opts = append(opts, tc.opts...) - sess, err := NewTracedSession(cluster, opts...) + sess, err := CreateTracedSession(cluster, opts...) require.NoError(t, err) err = sess.Query("SELECT * FROM trace.person WHERE name = 'Cassandra'").Exec() diff --git a/contrib/gocql/gocql/option.go b/contrib/gocql/gocql/option.go index 4096af7166..5e1c94a4d2 100644 --- a/contrib/gocql/gocql/option.go +++ b/contrib/gocql/gocql/option.go @@ -141,21 +141,21 @@ func WithCustomTag(key string, value interface{}) WrapOption { } } -// WithTraceQuery will enable tracing for queries. This option only takes effect in the NewTracedSession function. +// WithTraceQuery will enable tracing for queries. This option only takes effect in the CreateTracedSession function. func WithTraceQuery(enabled bool) WrapOption { return func(cfg *config) { cfg.traceQuery = enabled } } -// WithTraceBatch will enable tracing for batches. This option only takes effect in the NewTracedSession function. +// WithTraceBatch will enable tracing for batches. This option only takes effect in the CreateTracedSession function. func WithTraceBatch(enabled bool) WrapOption { return func(cfg *config) { cfg.traceBatch = enabled } } -// WithTraceConnect will enable tracing for connections. This option only takes effect in the NewTracedSession function. +// WithTraceConnect will enable tracing for connections. This option only takes effect in the CreateTracedSession function. func WithTraceConnect(enabled bool) WrapOption { return func(cfg *config) { cfg.traceConnect = enabled