Skip to content
This repository was archived by the owner on Mar 26, 2026. It is now read-only.

Fix: multiple calls to end of span #75

Merged
skuruppu merged 9 commits into from
Feb 25, 2020
Merged

Fix: multiple calls to end of span #75

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
feb6157
fix: multiple span.end calling
mayurkale22 Feb 14, 2020
a96f099
fix: multiple span.end calling
mayurkale22 Feb 14, 2020
1a6376d
fix: multiple span.end calling
mayurkale22 Feb 14, 2020
3d5561b
integrate test framework for span
mayurkale22 Feb 15, 2020
5042cae
add stream=null
mayurkale22 Feb 17, 2020
6082a6d
fix: execute tracer tests in separate execution
olavloite Feb 20, 2020
dbe356e
format: format pom
olavloite Feb 20, 2020
1bcaee8
attempt to fix tests
mayurkale22 Feb 20, 2020
a5c5d01
add com.google.errorprone:error_prone_annotations to ignoredDependencies
mayurkale22 Feb 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions google-cloud-spanner/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,37 @@
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<reportNameSuffix>sponge_log</reportNameSuffix>
</configuration>
<executions>
<execution>
<id>default-test</id>
<configuration>
<excludedGroups>com.google.cloud.spanner.TracerTest,com.google.cloud.spanner.IntegrationTest</excludedGroups>
</configuration>
</execution>
<execution>
<id>tracer</id>
<goals>
<goal>test</goal>
</goals>
<configuration>
<groups>com.google.cloud.spanner.TracerTest</groups>
</configuration>
</execution>
</executions>
</plugin>
</plugins>
<pluginManagement>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.0.0-M4</version>
<configuration>
<excludedGroups>com.google.cloud.spanner.IntegrationTest</excludedGroups>
<reportNameSuffix>sponge_log</reportNameSuffix>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
Expand All @@ -60,15 +80,15 @@
<spanner.testenv.instance>projects/gcloud-devel/instances/spanner-testing</spanner.testenv.instance>
</systemPropertyVariables>
<groups>com.google.cloud.spanner.IntegrationTest</groups>
<excludedGroups>com.google.cloud.spanner.FlakyTest</excludedGroups>
<excludedGroups>com.google.cloud.spanner.FlakyTest,com.google.cloud.spanner.TracerTest</excludedGroups>
<forkedProcessTimeoutInSeconds>2400</forkedProcessTimeoutInSeconds>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
<configuration>
<ignoredDependencies>io.grpc:grpc-protobuf-lite,org.hamcrest:hamcrest,org.hamcrest:hamcrest-core</ignoredDependencies>
<ignoredDependencies>io.grpc:grpc-protobuf-lite,org.hamcrest:hamcrest,org.hamcrest:hamcrest-core,com.google.errorprone:error_prone_annotations</ignoredDependencies>
</configuration>
</plugin>
</plugins>
Expand Down
5 changes: 3 additions & 2 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractResultSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,6 +922,7 @@ public void close(@Nullable String message) {
if (stream != null) {
stream.close(message);
span.end(TraceUtil.END_SPAN_OPTIONS);
stream = null;
}
}

Expand Down Expand Up @@ -997,11 +998,11 @@ protected PartialResultSet computeNext() {
continue;
}
span.addAnnotation("Stream broken. Not safe to retry");
TraceUtil.endSpanWithFailure(span, e);
TraceUtil.setWithFailure(span, e);
throw e;
} catch (RuntimeException e) {
span.addAnnotation("Stream broken. Not safe to retry");
TraceUtil.endSpanWithFailure(span, e);
TraceUtil.setWithFailure(span, e);
throw e;
}
}
Expand Down
8 changes: 5 additions & 3 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public Timestamp apply(Session session) {
}
});
} catch (RuntimeException e) {
TraceUtil.endSpanWithFailure(span, e);
TraceUtil.setWithFailure(span, e);
throw e;
} finally {
span.end(TraceUtil.END_SPAN_OPTIONS);
Expand All @@ -86,7 +86,7 @@ public Timestamp apply(Session session) {
}
});
} catch (RuntimeException e) {
TraceUtil.endSpanWithFailure(span, e);
TraceUtil.setWithFailure(span, e);
throw e;
} finally {
span.end(TraceUtil.END_SPAN_OPTIONS);
Expand Down Expand Up @@ -165,8 +165,10 @@ public TransactionRunner readWriteTransaction() {
try (Scope s = tracer.withSpan(span)) {
return getReadWriteSession().readWriteTransaction();
} catch (RuntimeException e) {
TraceUtil.endSpanWithFailure(span, e);
TraceUtil.setWithFailure(span, e);
throw e;
} finally {
span.end(TraceUtil.END_SPAN_OPTIONS);
}
}

Expand Down
7 changes: 4 additions & 3 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ public void run() {
try {
sessions = internalBatchCreateSessions(remainingSessionsToCreate, channelHint);
} catch (Throwable t) {
TraceUtil.endSpanWithFailure(SpannerImpl.tracer.getCurrentSpan(), t);
TraceUtil.setWithFailure(SpannerImpl.tracer.getCurrentSpan(), t);
consumer.onSessionCreateFailure(t, remainingSessionsToCreate);
break;
}
Expand Down Expand Up @@ -207,11 +207,12 @@ SessionImpl createSession() {
spanner
.getRpc()
.createSession(db.getName(), spanner.getOptions().getSessionLabels(), options);
span.end(TraceUtil.END_SPAN_OPTIONS);
return new SessionImpl(spanner, session.getName(), options);
} catch (RuntimeException e) {
TraceUtil.endSpanWithFailure(span, e);
TraceUtil.setWithFailure(span, e);
throw e;
} finally {
span.end(TraceUtil.END_SPAN_OPTIONS);
}
}

Expand Down
12 changes: 7 additions & 5 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,15 @@ public Timestamp writeAtLeastOnce(Iterable<Mutation> mutations) throws SpannerEx
try (Scope s = tracer.withSpan(span)) {
CommitResponse response = spanner.getRpc().commit(request, options);
Timestamp t = Timestamp.fromProto(response.getCommitTimestamp());
span.end(TraceUtil.END_SPAN_OPTIONS);
return t;
} catch (IllegalArgumentException e) {
TraceUtil.endSpanWithFailure(span, e);
TraceUtil.setWithFailure(span, e);
throw newSpannerException(ErrorCode.INTERNAL, "Could not parse commit timestamp", e);
} catch (RuntimeException e) {
TraceUtil.endSpanWithFailure(span, e);
TraceUtil.setWithFailure(span, e);
throw e;
} finally {
span.end(TraceUtil.END_SPAN_OPTIONS);
}
}

Expand Down Expand Up @@ -208,10 +209,11 @@ public void close() {
Span span = tracer.spanBuilder(SpannerImpl.DELETE_SESSION).startSpan();
try (Scope s = tracer.withSpan(span)) {
spanner.getRpc().deleteSession(name, options);
span.end(TraceUtil.END_SPAN_OPTIONS);
} catch (RuntimeException e) {
TraceUtil.endSpanWithFailure(span, e);
TraceUtil.setWithFailure(span, e);
throw e;
} finally {
span.end(TraceUtil.END_SPAN_OPTIONS);
}
}

Expand Down
2 changes: 1 addition & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,7 @@ private PooledSession take() throws SpannerException {
return s.session;
}
} catch (Exception e) {
TraceUtil.endSpanWithFailure(tracer.getCurrentSpan(), e);
Copy link
Copy Markdown
Contributor Author

@mayurkale22 mayurkale22 Feb 14, 2020

Choose a reason for hiding this comment

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

tracer.getCurrentSpan() is called outside of scope, which means endSpanWithFailure will execute on either READ_WRITE_TRANSACTION or READ_ONLY_TRANSACTION span instead of WAIT_FOR_SESSION span.

TraceUtil.setWithFailure(span, e);
throw e;
} finally {
span.end(TraceUtil.END_SPAN_OPTIONS);
Expand Down
10 changes: 10 additions & 0 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/TraceUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,16 @@ static ImmutableMap<String, AttributeValue> getExceptionAnnotations(SpannerExcep
"Status", AttributeValue.stringAttributeValue(e.getErrorCode().toString()));
}

static void setWithFailure(Span span, Throwable e) {
if (e instanceof SpannerException) {
span.setStatus(
StatusConverter.fromGrpcStatus(((SpannerException) e).getErrorCode().getGrpcStatus())
.withDescription(e.getMessage()));
} else {
span.setStatus(Status.INTERNAL.withDescription(e.getMessage()));
}
}

static void endSpanWithFailure(Span span, Throwable e) {
Copy link
Copy Markdown

@rghetia rghetia Feb 14, 2020

Choose a reason for hiding this comment

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

is endSpanWithFailure used anywhere now? If not then please remove it.

Copy link
Copy Markdown
Contributor Author

@mayurkale22 mayurkale22 Feb 14, 2020

Choose a reason for hiding this comment

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

This has been used in a few places (ex: singleUse) where span does not end except when there is a failure.

See: https://github.com/googleapis/google-cloud-java/pull/2677/files#r157323309 for original reason.

if (e instanceof SpannerException) {
endSpanWithFailure(span, (SpannerException) e);
Expand Down
2 changes: 1 addition & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ public <T> T run(TransactionCallable<T> callable) {
}
return runInternal(callable);
} catch (RuntimeException e) {
TraceUtil.endSpanWithFailure(span, e);
TraceUtil.setWithFailure(span, e);
throw e;
} finally {
// Remove threadLocal rather than set to FALSE to avoid a possible memory leak.
Expand Down
Loading