Skip to content

Commit

Permalink
Only set SpanStatus in error scenarios (#58)
Browse files Browse the repository at this point in the history
- Only set SpanStatus when errors are non nil
- Leave SpanStatus unset for all non error scenarios
- Add test for Streaming 
- Update old tests to check SpanStatus

Fixes: https://github.com/bufbuild/connect-opentelemetry-go/issues/51
  • Loading branch information
joshcarp authored Jan 18, 2023
1 parent 4b36709 commit e512fc0
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 1 deletion.
2 changes: 1 addition & 1 deletion interceptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ func protocolToSemConv(protocol string) string {

func spanStatus(err error) (codes.Code, string) {
if err == nil {
return codes.Ok, ""
return codes.Unset, ""
}
if connectErr := new(connect.Error); errors.As(err, &connectErr) {
return codes.Error, connectErr.Message()
Expand Down
44 changes: 44 additions & 0 deletions interceptor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/stretchr/testify/require"
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/codes"
"go.opentelemetry.io/otel/metric/unit"
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/otel/sdk/instrumentation"
Expand Down Expand Up @@ -952,6 +953,8 @@ func TestClientSimple(t *testing.T) {
if _, err := pingClient.Ping(context.Background(), requestOfSize(1, 0)); err != nil {
t.Errorf(err.Error())
}
require.Len(t, clientSpanRecorder.Ended(), 1)
require.Equal(t, clientSpanRecorder.Ended()[0].Status().Code, codes.Unset)
assertSpans(t, []wantSpans{
{
spanName: pingv1connect.PingServiceName + "/" + PingMethod,
Expand Down Expand Up @@ -999,6 +1002,8 @@ func TestHandlerFailCall(t *testing.T) {
if err == nil {
t.Fatal("expecting error, got nil")
}
require.Len(t, clientSpanRecorder.Ended(), 1)
require.Equal(t, clientSpanRecorder.Ended()[0].Status().Code, codes.Error)
assertSpans(t, []wantSpans{
{
spanName: pingv1connect.PingServiceName + "/" + FailMethod,
Expand Down Expand Up @@ -1049,6 +1054,8 @@ func TestClientHandlerOpts(t *testing.T) {
t.Errorf(err.Error())
}
assertSpans(t, []wantSpans{}, serverSpanRecorder.Ended())
require.Len(t, clientSpanRecorder.Ended(), 1)
require.Equal(t, clientSpanRecorder.Ended()[0].Status().Code, codes.Unset)
assertSpans(t, []wantSpans{
{
spanName: pingv1connect.PingServiceName + "/" + PingMethod,
Expand Down Expand Up @@ -1469,6 +1476,8 @@ func TestStreamingHandlerTracing(t *testing.T) {
assert.NoError(t, err)
assert.NoError(t, stream.CloseRequest())
assert.NoError(t, stream.CloseResponse())
require.Len(t, spanRecorder.Ended(), 1)
require.Equal(t, spanRecorder.Ended()[0].Status().Code, codes.Unset)
assertSpans(t, []wantSpans{
{
spanName: pingv1connect.PingServiceName + "/" + CumSumMethod,
Expand Down Expand Up @@ -1524,6 +1533,8 @@ func TestStreamingClientTracing(t *testing.T) {
assert.NoError(t, err)
assert.NoError(t, stream.CloseRequest())
assert.NoError(t, stream.CloseResponse())
require.Len(t, spanRecorder.Ended(), 1)
require.Equal(t, spanRecorder.Ended()[0].Status().Code, codes.Unset)
assertSpans(t, []wantSpans{
{
spanName: pingv1connect.PingServiceName + "/" + CumSumMethod,
Expand Down Expand Up @@ -1668,6 +1679,39 @@ func TestWithoutServerPeerAttributes(t *testing.T) {
}, spanRecorder.Ended())
}

func TestStreamingSpanStatus(t *testing.T) {
t.Parallel()
var propagator propagation.TraceContext
handlerSpanRecorder := tracetest.NewSpanRecorder()
handlerTraceProvider := trace.NewTracerProvider(trace.WithSpanProcessor(handlerSpanRecorder))
clientSpanRecorder := tracetest.NewSpanRecorder()
clientTraceProvider := trace.NewTracerProvider(trace.WithSpanProcessor(clientSpanRecorder))
client, _, _ := startServer(
[]connect.HandlerOption{WithTelemetry(
WithPropagator(propagator),
WithTracerProvider(handlerTraceProvider),
)}, []connect.ClientOption{
WithTelemetry(
WithPropagator(propagator),
WithTracerProvider(clientTraceProvider),
),
}, failPingServer())
stream := client.CumSum(context.Background())
assert.NoError(t, stream.Send(&pingv1.CumSumRequest{Number: 1}))
_, err := stream.Receive()
assert.NoError(t, err)
_, err = stream.Receive()
assert.NoError(t, err)
_, err = stream.Receive()
assert.Error(t, err)
assert.NoError(t, stream.CloseRequest())
assert.NoError(t, stream.CloseResponse())
assert.Equal(t, len(handlerSpanRecorder.Ended()), 1)
assert.Equal(t, len(clientSpanRecorder.Ended()), 1)
assert.Equal(t, handlerSpanRecorder.Ended()[0].Status().Code, codes.Error)
assert.Equal(t, clientSpanRecorder.Ended()[0].Status().Code, codes.Error)
}

// streamingHandlerInterceptorFunc is a simple Interceptor implementation that only
// wraps streaming handler RPCs. It has no effect on unary or streaming client RPCs.
type streamingHandlerInterceptorFunc func(connect.StreamingHandlerFunc) connect.StreamingHandlerFunc
Expand Down

0 comments on commit e512fc0

Please sign in to comment.