breaking(zipkin): removes servicName from zipkin exporter. (#1697)

* breaking(zipkin): removes servicName from zipkin exporter. Resource detector provides a serviceName in all cases, hence we can relay on span resource to obtain the serviceName. Also this is required by the spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/zipkin.md\#service-name (#1549). * docs(zipkin): adds changelog. * chore(examples/zipkin): updates example accordingly. Co-authored-by: Anthony Mirabella <[email protected]>
open-telemetry · Mar 16, 2021 · 345f264 · 345f264
1 parent 62cbf0f
commit 345f264
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 35 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -35,6 +35,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
    These are now returned as a SpanProcessor interface from their respective constructors. (#1638)
 - Removed setting status to `Error` while recording an error as a span event in `RecordError`. (#1663)
 - Removed `WithConfig` from tracer provider to avoid overriding configuration. (#1633)
+- Removed `serviceName` parameter from Zipkin exporter and uses resource instead. (#1549)
 - Removed `jaeger.WithProcess`. (#1673)
 
 ### Fixed

diff --git a/example/zipkin/main.go b/example/zipkin/main.go
@@ -25,7 +25,9 @@ import (
 
 	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/exporters/trace/zipkin"
+	"go.opentelemetry.io/otel/sdk/resource"
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
+	"go.opentelemetry.io/otel/semconv"
 	"go.opentelemetry.io/otel/trace"
 )
 
@@ -40,7 +42,6 @@ func initTracer(url string) func() {
 	// ratio.
 	exporter, err := zipkin.NewRawExporter(
 		url,
-		"zipkin-test",
 		zipkin.WithLogger(logger),
 		zipkin.WithSDK(&sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()}),
 	)
@@ -50,7 +51,12 @@ func initTracer(url string) func() {
 
 	batcher := sdktrace.NewBatchSpanProcessor(exporter)
 
-	tp := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(batcher))
+	tp := sdktrace.NewTracerProvider(
+		sdktrace.WithSpanProcessor(batcher),
+		sdktrace.WithResource(resource.NewWithAttributes(
+			semconv.ServiceNameKey.String("zipkin-test"),
+		)),
+	)
 	otel.SetTracerProvider(tp)
 
 	return func() {

diff --git a/exporters/trace/zipkin/model.go b/exporters/trace/zipkin/model.go
@@ -23,6 +23,7 @@ import (
 
 	"go.opentelemetry.io/otel/attribute"
 	export "go.opentelemetry.io/otel/sdk/export/trace"
+	"go.opentelemetry.io/otel/semconv"
 	"go.opentelemetry.io/otel/trace"
 )
 
@@ -31,15 +32,26 @@ const (
 	keyInstrumentationLibraryVersion = "otel.instrumentation_library.version"
 )
 
-func toZipkinSpanModels(batch []*export.SpanSnapshot, serviceName string) []zkmodel.SpanModel {
+func toZipkinSpanModels(batch []*export.SpanSnapshot) []zkmodel.SpanModel {
 	models := make([]zkmodel.SpanModel, 0, len(batch))
 	for _, data := range batch {
-		models = append(models, toZipkinSpanModel(data, serviceName))
+		models = append(models, toZipkinSpanModel(data))
 	}
 	return models
 }
 
-func toZipkinSpanModel(data *export.SpanSnapshot, serviceName string) zkmodel.SpanModel {
+func getServiceName(attrs []attribute.KeyValue) string {
+	for _, kv := range attrs {
+		if kv.Key == semconv.ServiceNameKey {
+			return kv.Value.AsString()
+		}
+	}
+
+	// Resource holds a default value so this might not be reach.
+	return ""
+}
+
+func toZipkinSpanModel(data *export.SpanSnapshot) zkmodel.SpanModel {
 	return zkmodel.SpanModel{
 		SpanContext: toZipkinSpanContext(data),
 		Name:        data.Name,
@@ -48,7 +60,7 @@ func toZipkinSpanModel(data *export.SpanSnapshot, serviceName string) zkmodel.Sp
 		Duration:    data.EndTime.Sub(data.StartTime),
 		Shared:      false,
 		LocalEndpoint: &zkmodel.Endpoint{
-			ServiceName: serviceName,
+			ServiceName: getServiceName(data.Resource.Attributes()),
 		},
 		RemoteEndpoint: nil, // *Endpoint
 		Annotations:    toZipkinAnnotations(data.MessageEvents),

diff --git a/exporters/trace/zipkin/model_test.go b/exporters/trace/zipkin/model_test.go
@@ -22,16 +22,23 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	zkmodel "github.com/openzipkin/zipkin-go/model"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/codes"
 	export "go.opentelemetry.io/otel/sdk/export/trace"
 	"go.opentelemetry.io/otel/sdk/instrumentation"
+	"go.opentelemetry.io/otel/sdk/resource"
+	"go.opentelemetry.io/otel/semconv"
 	"go.opentelemetry.io/otel/trace"
 )
 
 func TestModelConversion(t *testing.T) {
+	resource := resource.NewWithAttributes(
+		semconv.ServiceNameKey.String("model-test"),
+	)
+
 	inputBatch := []*export.SpanSnapshot{
 		// typical span data
 		{
@@ -64,6 +71,7 @@ func TestModelConversion(t *testing.T) {
 			},
 			StatusCode:    codes.Error,
 			StatusMessage: "404, file not found",
+			Resource:      resource,
 		},
 		// span data with no parent (same as typical, but has
 		// invalid parent)
@@ -97,6 +105,7 @@ func TestModelConversion(t *testing.T) {
 			},
 			StatusCode:    codes.Error,
 			StatusMessage: "404, file not found",
+			Resource:      resource,
 		},
 		// span data of unspecified kind
 		{
@@ -129,6 +138,7 @@ func TestModelConversion(t *testing.T) {
 			},
 			StatusCode:    codes.Error,
 			StatusMessage: "404, file not found",
+			Resource:      resource,
 		},
 		// span data of internal kind
 		{
@@ -161,6 +171,7 @@ func TestModelConversion(t *testing.T) {
 			},
 			StatusCode:    codes.Error,
 			StatusMessage: "404, file not found",
+			Resource:      resource,
 		},
 		// span data of client kind
 		{
@@ -193,6 +204,7 @@ func TestModelConversion(t *testing.T) {
 			},
 			StatusCode:    codes.Error,
 			StatusMessage: "404, file not found",
+			Resource:      resource,
 		},
 		// span data of producer kind
 		{
@@ -225,6 +237,7 @@ func TestModelConversion(t *testing.T) {
 			},
 			StatusCode:    codes.Error,
 			StatusMessage: "404, file not found",
+			Resource:      resource,
 		},
 		// span data of consumer kind
 		{
@@ -257,6 +270,7 @@ func TestModelConversion(t *testing.T) {
 			},
 			StatusCode:    codes.Error,
 			StatusMessage: "404, file not found",
+			Resource:      resource,
 		},
 		// span data with no events
 		{
@@ -276,6 +290,7 @@ func TestModelConversion(t *testing.T) {
 			MessageEvents: nil,
 			StatusCode:    codes.Error,
 			StatusMessage: "404, file not found",
+			Resource:      resource,
 		},
 		// span data with an "error" attribute set to "false"
 		{
@@ -307,6 +322,7 @@ func TestModelConversion(t *testing.T) {
 			},
 			StatusCode:    codes.Error,
 			StatusMessage: "404, file not found",
+			Resource:      resource,
 		},
 	}
 
@@ -652,7 +668,7 @@ func TestModelConversion(t *testing.T) {
 			},
 		},
 	}
-	gottenOutputBatch := toZipkinSpanModels(inputBatch, "model-test")
+	gottenOutputBatch := toZipkinSpanModels(inputBatch)
 	require.Equal(t, expectedOutputBatch, gottenOutputBatch)
 }
 
@@ -780,3 +796,14 @@ func Test_toZipkinTags(t *testing.T) {
 		})
 	}
 }
+
+func TestServiceName(t *testing.T) {
+	attrs := []attribute.KeyValue{}
+	assert.Empty(t, getServiceName(attrs))
+
+	attrs = append(attrs, attribute.String("test_key", "test_value"))
+	assert.Empty(t, getServiceName(attrs))
+
+	attrs = append(attrs, semconv.ServiceNameKey.String("my_service"))
+	assert.Equal(t, "my_service", getServiceName(attrs))
+}
diff --git a/exporters/trace/zipkin/zipkin.go b/exporters/trace/zipkin/zipkin.go
@@ -36,11 +36,10 @@ import (
 // the SpanBatcher interface, so it needs to be used together with the
 // WithBatcher option when setting up the exporter pipeline.
 type Exporter struct {
-	url         string
-	serviceName string
-	client      *http.Client
-	logger      *log.Logger
-	o           options
+	url    string
+	client *http.Client
+	logger *log.Logger
+	o      options
 
 	stoppedMu sync.RWMutex
 	stopped   bool
@@ -82,7 +81,7 @@ func WithSDK(config *sdktrace.Config) Option {
 }
 
 // NewRawExporter creates a new Zipkin exporter.
-func NewRawExporter(collectorURL, serviceName string, opts ...Option) (*Exporter, error) {
+func NewRawExporter(collectorURL string, opts ...Option) (*Exporter, error) {
 	if collectorURL == "" {
 		return nil, errors.New("collector URL cannot be empty")
 	}
@@ -102,18 +101,17 @@ func NewRawExporter(collectorURL, serviceName string, opts ...Option) (*Exporter
 		o.client = http.DefaultClient
 	}
 	return &Exporter{
-		url:         collectorURL,
-		client:      o.client,
-		logger:      o.logger,
-		serviceName: serviceName,
-		o:           o,
+		url:    collectorURL,
+		client: o.client,
+		logger: o.logger,
+		o:      o,
 	}, nil
 }
 
 // NewExportPipeline sets up a complete export pipeline
 // with the recommended setup for trace provider
-func NewExportPipeline(collectorURL, serviceName string, opts ...Option) (*sdktrace.TracerProvider, error) {
-	exporter, err := NewRawExporter(collectorURL, serviceName, opts...)
+func NewExportPipeline(collectorURL string, opts ...Option) (*sdktrace.TracerProvider, error) {
+	exporter, err := NewRawExporter(collectorURL, opts...)
 	if err != nil {
 		return nil, err
 	}
@@ -128,8 +126,8 @@ func NewExportPipeline(collectorURL, serviceName string, opts ...Option) (*sdktr
 
 // InstallNewPipeline instantiates a NewExportPipeline with the
 // recommended configuration and registers it globally.
-func InstallNewPipeline(collectorURL, serviceName string, opts ...Option) error {
-	tp, err := NewExportPipeline(collectorURL, serviceName, opts...)
+func InstallNewPipeline(collectorURL string, opts ...Option) error {
+	tp, err := NewExportPipeline(collectorURL, opts...)
 	if err != nil {
 		return err
 	}
@@ -152,7 +150,7 @@ func (e *Exporter) ExportSpans(ctx context.Context, ss []*export.SpanSnapshot) e
 		e.logf("no spans to export")
 		return nil
 	}
-	models := toZipkinSpanModels(ss, e.serviceName)
+	models := toZipkinSpanModels(ss)
 	body, err := json.Marshal(models)
 	if err != nil {
 		return e.errf("failed to serialize zipkin models to JSON: %v", err)