otelmemcache: Simplify config and span status setting (#477)

* otelmemcache: Simplify config - remove service name

- Service name no longer needed
- No need to keep config inside of client any longer

* otelmemcache: Do not set status and service name attribute

- Set status only in case of error

* otelmemcache: Adjust tests

* Update CHANGELOG

* PR feedback - revert to config func

Co-authored-by: Anthony Mirabella <[email protected]>

Author: matej-g

CHANGELOG.md

@@ -8,6 +8,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [Unreleased]
 
+### Fixed
+
+- `otelmemcache` no longer sets span status to OK instead of leaving it unset. (#477)
+
+### Removed
+
+- Remove service name from `otelmemcache` configuration and span attributes. (#477)
+
 ## [0.17.0] - 2021-02-15
 
 ### Added

instrumentation/github.com/bradfitz/gomemcache/memcache/otelmemcache/config.go

@@ -19,7 +19,6 @@ import (
 )
 
 type config struct {
-	serviceName    string
 	tracerProvider oteltrace.TracerProvider
 }
 
@@ -33,10 +32,3 @@ func WithTracerProvider(provider oteltrace.TracerProvider) Option {
 		cfg.tracerProvider = provider
 	}
 }
-
-// WithServiceName sets the service name.
-func WithServiceName(serviceName string) Option {
-	return func(cfg *config) {
-		cfg.serviceName = serviceName
-	}
-}

instrumentation/github.com/bradfitz/gomemcache/memcache/otelmemcache/gomemcache.go

@@ -21,20 +21,18 @@ import (
 
 	"go.opentelemetry.io/contrib"
 	"go.opentelemetry.io/otel"
+	"go.opentelemetry.io/otel/codes"
 	"go.opentelemetry.io/otel/label"
-	"go.opentelemetry.io/otel/semconv"
 	oteltrace "go.opentelemetry.io/otel/trace"
 )
 
 const (
-	defaultTracerName  = "go.opentelemetry.io/contrib/instrumentation/github.com/bradfitz/gomemcache/memcache/otelmemcache"
-	defaultServiceName = "memcached"
+	tracerName = "go.opentelemetry.io/contrib/instrumentation/github.com/bradfitz/gomemcache/memcache/otelmemcache"
 )
 
 // Client is a wrapper around *memcache.Client.
 type Client struct {
 	*memcache.Client
-	cfg    *config
 	tracer oteltrace.Tracer
 	ctx    context.Context
 }
@@ -54,19 +52,14 @@ func NewClientWithTracing(client *memcache.Client, opts ...Option) *Client {
 		o(cfg)
 	}
 
-	if cfg.serviceName == "" {
-		cfg.serviceName = defaultServiceName
-	}
-
 	if cfg.tracerProvider == nil {
 		cfg.tracerProvider = otel.GetTracerProvider()
 	}
 
 	return &Client{
 		client,
-		cfg,
 		cfg.tracerProvider.Tracer(
-			defaultTracerName,
+			tracerName,
 			oteltrace.WithInstrumentationVersion(contrib.SemVersion()),
 		),
 		context.Background(),
@@ -77,7 +70,6 @@ func NewClientWithTracing(client *memcache.Client, opts ...Option) *Client {
 // of the operation name and item key(s) (if available)
 func (c *Client) attrsByOperationAndItemKey(operation operation, key ...string) []label.KeyValue {
 	labels := []label.KeyValue{
-		semconv.ServiceNameKey.String(c.cfg.serviceName),
 		memcacheDBSystem(),
 		memcacheDBOperation(operation),
 	}
@@ -111,7 +103,7 @@ func (c *Client) startSpan(operationName operation, itemKey ...string) oteltrace
 // Ends span and, if applicable, sets error status
 func endSpan(s oteltrace.Span, err error) {
 	if err != nil {
-		s.SetStatus(memcacheErrToStatusCode(err), err.Error())
+		s.SetStatus(codes.Error, err.Error())
 	}
 	s.End()
 }
@@ -121,7 +113,6 @@ func (c *Client) WithContext(ctx context.Context) *Client {
 	cc := c.Client
 	return &Client{
 		Client: cc,
-		cfg:    c.cfg,
 		tracer: c.tracer,
 		ctx:    ctx,
 	}

instrumentation/github.com/bradfitz/gomemcache/memcache/otelmemcache/gomemcache_test.go

@@ -26,7 +26,6 @@ import (
 	"go.opentelemetry.io/otel/codes"
 	"go.opentelemetry.io/otel/label"
 	"go.opentelemetry.io/otel/oteltest"
-	"go.opentelemetry.io/otel/semconv"
 	oteltrace "go.opentelemetry.io/otel/trace"
 )
 
@@ -41,10 +40,7 @@ func TestNewClientWithTracing(t *testing.T) {
 	)
 
 	assert.NotNil(t, c.Client)
-	assert.NotNil(t, c.cfg)
-	assert.NotNil(t, c.cfg.tracerProvider)
 	assert.NotNil(t, c.tracer)
-	assert.Equal(t, defaultServiceName, c.cfg.serviceName)
 }
 
 func TestOperation(t *testing.T) {
@@ -61,10 +57,9 @@ func TestOperation(t *testing.T) {
 	assert.Len(t, spans, 1)
 	assert.Equal(t, oteltrace.SpanKindClient, spans[0].SpanKind())
 	assert.Equal(t, string(operationAdd), spans[0].Name())
-	assert.Len(t, spans[0].Attributes(), 4)
+	assert.Len(t, spans[0].Attributes(), 3)
 
 	expectedLabelMap := map[label.Key]label.Value{
-		semconv.ServiceNameKey:                              semconv.ServiceNameKey.String(defaultServiceName).Value,
 		memcacheDBSystem().Key:                              memcacheDBSystem().Value,
 		memcacheDBOperation(operationAdd).Key:               memcacheDBOperation(operationAdd).Value,
 		label.Key(memcacheDBItemKeyName).String(mi.Key).Key: label.Key(memcacheDBItemKeyName).String(mi.Key).Value,
@@ -83,10 +78,9 @@ func TestOperationWithCacheMissError(t *testing.T) {
 	assert.Len(t, spans, 1)
 	assert.Equal(t, oteltrace.SpanKindClient, spans[0].SpanKind())
 	assert.Equal(t, string(operationGet), spans[0].Name())
-	assert.Len(t, spans[0].Attributes(), 4)
+	assert.Len(t, spans[0].Attributes(), 3)
 
 	expectedLabelMap := map[label.Key]label.Value{
-		semconv.ServiceNameKey:                           semconv.ServiceNameKey.String(defaultServiceName).Value,
 		memcacheDBSystem().Key:                           memcacheDBSystem().Value,
 		memcacheDBOperation(operationGet).Key:            memcacheDBOperation(operationGet).Value,
 		label.Key(memcacheDBItemKeyName).String(key).Key: label.Key(memcacheDBItemKeyName).String(key).Value,