spanner(fix): do not end span with iterator.Done (#3128)
Spans should not be ended with iterator.Done as an error. This is not an
actual error, but an indication that the iterator reached its end. Setting
this as an error on a span pollutes the traces and makes it harder to find
actual errors that might occur.
Fixes #3110
diff --git a/spanner/client_test.go b/spanner/client_test.go
index 8750541..7ea14f0 100644
--- a/spanner/client_test.go
+++ b/spanner/client_test.go
@@ -2183,3 +2183,59 @@
},
}
}
+
+func TestClient_DoForEachRow_ShouldNotEndSpanWithIteratorDoneError(t *testing.T) {
+ // This test cannot be parallel, as the TestExporter does not support that.
+ te := itestutil.NewTestExporter()
+ defer te.Unregister()
+ _, client, teardown := setupMockedTestServer(t)
+ defer teardown()
+
+ iter := client.Single().Query(context.Background(), NewStatement(SelectSingerIDAlbumIDAlbumTitleFromAlbums))
+ iter.Do(func(r *Row) error {
+ return nil
+ })
+ select {
+ case <-te.Stats:
+ case <-time.After(1 * time.Second):
+ t.Fatal("No stats were exported before timeout")
+ }
+ if len(te.Spans) == 0 {
+ t.Fatal("No spans were exported")
+ }
+ s := te.Spans[len(te.Spans)-1].Status
+ if s.Code != int32(codes.OK) {
+ t.Errorf("Span status mismatch\nGot: %v\nWant: %v", s.Code, codes.OK)
+ }
+}
+
+func TestClient_DoForEachRow_ShouldEndSpanWithQueryError(t *testing.T) {
+ // This test cannot be parallel, as the TestExporter does not support that.
+ te := itestutil.NewTestExporter()
+ defer te.Unregister()
+ server, client, teardown := setupMockedTestServer(t)
+ defer teardown()
+
+ sql := "SELECT * FROM"
+ server.TestSpanner.PutStatementResult(sql, &StatementResult{
+ Type: StatementResultError,
+ Err: status.Error(codes.InvalidArgument, "Invalid query"),
+ })
+
+ iter := client.Single().Query(context.Background(), NewStatement(sql))
+ iter.Do(func(r *Row) error {
+ return nil
+ })
+ select {
+ case <-te.Stats:
+ case <-time.After(1 * time.Second):
+ t.Fatal("No stats were exported before timeout")
+ }
+ if len(te.Spans) == 0 {
+ t.Fatal("No spans were exported")
+ }
+ s := te.Spans[len(te.Spans)-1].Status
+ if s.Code != int32(codes.InvalidArgument) {
+ t.Errorf("Span status mismatch\nGot: %v\nWant: %v", s.Code, codes.InvalidArgument)
+ }
+}
diff --git a/spanner/read.go b/spanner/read.go
index 5ede3a3..296e103 100644
--- a/spanner/read.go
+++ b/spanner/read.go
@@ -209,7 +209,11 @@
// iterator.
func (r *RowIterator) Stop() {
if r.streamd != nil {
- defer trace.EndSpan(r.streamd.ctx, r.err)
+ if r.err != nil && r.err != iterator.Done {
+ defer trace.EndSpan(r.streamd.ctx, r.err)
+ } else {
+ defer trace.EndSpan(r.streamd.ctx, nil)
+ }
}
if r.cancel != nil {
r.cancel()
@@ -220,7 +224,6 @@
r.err = spannerErrorf(codes.FailedPrecondition, "Next called after Stop")
}
r.release = nil
-
}
}
