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

Conversation

stevencl1013
Copy link
Contributor

This allows users to, when creating the exporter, use a custom DisplayNameFormatter function to specify how to create the display name from span data. Default format remains the same. Addressing issue #45

@ymotongpoo
Copy link
Member

@stevencl1013 Can you run make precommit? It's failing as the following:

=== RUN   TestExporter_Timeout
    TestExporter_Timeout: cloudtrace_test.go:157: err.Error() = "rpc error: code = Unknown desc = context deadline exceeded"; want "rpc error: code = DeadlineExceeded desc = context deadline exceeded"
--- FAIL: TestExporter_Timeout (0.00s)
FAIL
FAIL	github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/trace	0.029s
FAIL
make: *** [Makefile:85: test-386] Error 1

@stevencl1013
Copy link
Contributor Author

@ymotongpoo When I ran make precommit on my machine, all the tests passed. I think there might be an issue with the timeout test, because it did get a context deadline exceeded error, but failed because it got rpc error: code = Unknown instead of rpc error: code = DeadlineExceeded. I've experienced this issue occasionally on my machine as well in the past.

Copy link

@qqqstuv qqqstuv left a comment

Choose a reason for hiding this comment

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

LGTM.

Does the other languages also support custom span name? We should coordinate with them to lepe the feature parity among languages

Copy link
Contributor

@nilebox nilebox left a comment

Choose a reason for hiding this comment

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

LGTM, please address the comments.

@@ -69,10 +69,14 @@ func protoFromSpanData(s *export.SpanData, projectID string) *tracepb.Span {
spanIDString := s.SpanContext.SpanID.String()

name := s.Name
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename this variable to displayName - Span has the Name field which will have completely different value (see below).
Also, the initialization with s.Name value seems redundant since it will always be overwritten below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually suggest to extract this entire code block into generateDisplayName(s, format) to make the code cleaner / easier to read.

Copy link
Contributor

@james-bebbington james-bebbington left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 64 to 72
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)
}

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.

@james-bebbington james-bebbington merged commit a1393af into GoogleCloudPlatform:master Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants