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

Allow custom display name format for spans (#45) #48

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions exporter/trace/cloudtrace.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ import (
// Option is function type that is passed to the exporter initialization function.
type Option func(*options)

// DisplayNameFormatter is is a function that produces the display name of a span
// given its SpanData
type DisplayNameFormatter func(*export.SpanData) string

// options contains options for configuring the exporter.
type options struct {
// ProjectID is the identifier of the Stackdriver
Expand Down Expand Up @@ -107,6 +111,11 @@ type options struct {
// to Stackdriver Monitoring. This is only used for Proto metrics export
// for now. The minimum number of workers is 1.
NumberOfWorkers int

// DisplayNameFormatter is a function that produces the display name of a span
// given its SpanData.
// Optional. Default format for SpanData s is "Span.{s.SpanKind}-{s.Name}"
DisplayNameFormatter
}

// WithProjectID sets Google Cloud Platform project as projectID.
Expand Down Expand Up @@ -151,6 +160,14 @@ func WithTimeout(t time.Duration) func(o *options) {
}
}

// WithDisplayNameFormatter sets the way span's display names will be
// generated from SpanData
func WithDisplayNameFormatter(f DisplayNameFormatter) func(o *options) {
return func(o *options) {
o.DisplayNameFormatter = f
}
}

func (o *options) handleError(err error) {
if o.OnError != nil {
o.OnError(err)
Expand Down
40 changes: 40 additions & 0 deletions exporter/trace/cloudtrace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (

"go.opentelemetry.io/otel/api/global"
sdktrace "go.opentelemetry.io/otel/sdk/trace"
export "go.opentelemetry.io/otel/sdk/export/trace"
)

type mockTraceServer struct {
Expand Down Expand Up @@ -122,6 +123,45 @@ func TestExporter_ExportSpans(t *testing.T) {
assert.EqualValues(t, 1, mockTrace.len())
}

func TestExporter_DisplayNameFormatter(t *testing.T) {
// Initial test precondition
mockTrace.spansUploaded = nil
mockTrace.delay = 0

spanName := "span1234"
format := func(s *export.SpanData) string {
return "TEST_FORMAT" + s.Name
}

// Create Google Cloud Trace Exporter
exp, err := texporter.NewExporter(
texporter.WithProjectID("PROJECT_ID_NOT_REAL"),
texporter.WithTraceClientOptions(clientOpt),
// uncomment when exporter is using bundler
// texporter.WithBundleCountThreshold(1),
texporter.WithDisplayNameFormatter(format),
)
assert.NoError(t, err)

tp, err := sdktrace.NewProvider(
sdktrace.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()}),
sdktrace.WithBatcher(exp, // add following two options to ensure flush
sdktrace.WithScheduleDelayMillis(1),
sdktrace.WithMaxExportBatchSize(1),
))
assert.NoError(t, err)

global.SetTraceProvider(tp)
_, span := global.TraceProvider().Tracer("test-tracer").Start(context.Background(), spanName)
span.End()
assert.True(t, span.SpanContext().IsValid())

// wait exporter to flush
time.Sleep(20 * time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sleeping for a block amount of time in tests in never ideal. To make this signficiantly more robust, and faster, a better alternative to this is to do something like

assert.Eventually(t, function() {
    ...
}, 500 * time.Millisecond, 2 * time.Millisecond)

I wouldn't bother changing this now though. After you add the NewPipeline method that returns a flush function (see the Jaeger exporter), you can avoid the sleep here entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, sounds good. I'll leave this for now, and when I create the NewPipeline method, I plan to update this test and others to use the pipeline and flush function as appropriate.

assert.EqualValues(t, 1, mockTrace.len())
assert.EqualValues(t, "TEST_FORMAT" + spanName, mockTrace.spansUploaded[0].DisplayName.Value)
}

func TestExporter_Timeout(t *testing.T) {
// Initial test precondition
mockTrace.spansUploaded = nil
Expand Down
4 changes: 2 additions & 2 deletions exporter/trace/trace.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ func newTraceExporter(o *options) (*traceExporter, error) {

// ExportSpan exports a SpanData to Stackdriver Trace.
func (e *traceExporter) ExportSpan(ctx context.Context, sd *export.SpanData) {
protoSpan := protoFromSpanData(sd, e.projectID)
protoSpan := protoFromSpanData(sd, e.projectID, e.o.DisplayNameFormatter)
e.uploadFn(ctx, []*tracepb.Span{protoSpan})
}

// ExportSpans exports a slice of SpanData to Stackdriver Trace in batch
func (e *traceExporter) ExportSpans(ctx context.Context, sds []*export.SpanData) {
pbSpans := make([]*tracepb.Span, len(sds))
for i, sd := range sds {
pbSpans[i] = protoFromSpanData(sd, e.projectID)
pbSpans[i] = protoFromSpanData(sd, e.projectID, e.o.DisplayNameFormatter)
}
e.uploadFn(ctx, pbSpans)
}
Expand Down
24 changes: 16 additions & 8 deletions exporter/trace/trace_proto.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,25 +60,33 @@ const (

var userAgent = fmt.Sprintf("opentelemetry-go %s; cloudtrace-exporter %s", opentelemetry.Version(), version)

func protoFromSpanData(s *export.SpanData, projectID string) *tracepb.Span {
func generateDisplayName(s *export.SpanData, format DisplayNameFormatter) (displayName string) {
if format == nil {
switch s.SpanKind {
// TODO(ymotongpoo): add cases for "Send" and "Recv".
default:
displayName = fmt.Sprintf("Span.%s-%s", s.SpanKind, s.Name)
}
} else {
displayName = format(s)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's always a little easier to read code if you minimize nesting imo:

Suggested change
if format == nil {
switch s.SpanKind {
// TODO(ymotongpoo): add cases for "Send" and "Recv".
default:
displayName = fmt.Sprintf("Span.%s-%s", s.SpanKind, s.Name)
}
} else {
displayName = format(s)
}
if format != nil {
return format(s)
}
switch s.SpanKind {
// TODO(ymotongpoo): add cases for "Send" and "Recv".
default:
return fmt.Sprintf("Span.%s-%s", s.SpanKind, s.Name)
}

return
}

func protoFromSpanData(s *export.SpanData, projectID string, format DisplayNameFormatter) *tracepb.Span {
if s == nil {
return nil
}

traceIDString := s.SpanContext.TraceID.String()
spanIDString := s.SpanContext.SpanID.String()

name := s.Name
switch s.SpanKind {
// TODO(ymotongpoo): add cases for "Send" and "Recv".
default:
name = fmt.Sprintf("Span.%s-%s", s.SpanKind, name)
}
displayName := generateDisplayName(s, format)

sp := &tracepb.Span{
Name: "projects/" + projectID + "/traces/" + traceIDString + "/spans/" + spanIDString,
SpanId: spanIDString,
DisplayName: trunc(name, 128),
DisplayName: trunc(displayName, 128),
StartTime: timestampProto(s.StartTime),
EndTime: timestampProto(s.EndTime),
SameProcessAsParentSpan: &wrapperspb.BoolValue{Value: !s.HasRemoteParent},
Expand Down