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.

Sign up for GitHub

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

Jump to bottom

Fix: multiple calls to end of span #75

Merged
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 26 additions & 6 deletions google-cloud-spanner/pom.xml
View file
Open in desktop
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
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
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
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
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
Expand Up @@ -895,7 +895,7 @@ private PooledSession take() throws SpannerException {
return s.session;
}
} catch (Exception e) {
TraceUtil.endSpanWithFailure(tracer.getCurrentSpan(), e);
Copy link
Member Author

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
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

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
Member Author

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
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