Skip to content

Commit

Permalink
Fix: multiple calls to end of span (#75)
Browse files Browse the repository at this point in the history 
* fix: multiple span.end calling

* fix: multiple span.end calling

* fix: multiple span.end calling

* integrate test framework for span

* add stream=null

* fix: execute tracer tests in separate execution

* format: format pom

* attempt to fix tests

* add com.google.errorprone:error_prone_annotations to ignoredDependencies

Co-authored-by: Knut Olav Løite <koloite@gmail.com>
  • Loading branch information
@mayurkale22 @olavloite
mayurkale22 and olavloite committed Feb 25, 2020
1 parent 92785c1 commit 3f32f51
Show file tree
Hide file tree
Showing 11 changed files with 652 additions and 21 deletions.
32 changes: 26 additions & 6 deletions google-cloud-spanner/pom.xml
View file
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
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
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
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
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
Expand Up @@ -895,7 +895,7 @@ private PooledSession take() throws SpannerException {
return s.session;
}
} catch (Exception e) {
TraceUtil.endSpanWithFailure(tracer.getCurrentSpan(), e);
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
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) {
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
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

0 comments on commit 3f32f51

Please sign in to comment.