Skip to content

Commit 41f6a2f

Browse files
chingor13chingor13
authored
fix: OpenCensus spans should close on IOExceptions (#797)
* chore: add basic test for OpenCensus spans * chore: add failing test for closing spans on IOExceptions * fix: close SENT span if the IOException was not handled before rethrowing * relax curl logger tests for added opencensus header * fix: address PR comments * chore: remove unused helper * fix: dependency warning for opencensus-impl * chore: cleanup PR comments
1 parent c557a10 commit 41f6a2f

File tree

5 files changed

+201
-20
lines changed

5 files changed

+201
-20
lines changed

‎google-http-client/pom.xml

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,18 @@
1616
</description>
1717

1818
<build>
19+
<pluginManagement>
20+
<plugins>
21+
<plugin>
22+
<groupId>org.apache.maven.plugins</groupId>
23+
<artifactId>maven-dependency-plugin</artifactId>
24+
<version>3.1.1</version>
25+
<configuration>
26+
<ignoredUnusedDeclaredDependencies>io.opencensus:opencensus-impl</ignoredUnusedDeclaredDependencies>
27+
</configuration>
28+
</plugin>
29+
</plugins>
30+
</pluginManagement>
1931
<plugins>
2032
<plugin>
2133
<artifactId>maven-javadoc-plugin</artifactId>
@@ -84,6 +96,19 @@
8496
<groupId>com.google.guava</groupId>
8597
<artifactId>guava</artifactId>
8698
</dependency>
99+
<dependency>
100+
<groupId>com.google.j2objc</groupId>
101+
<artifactId>j2objc-annotations</artifactId>
102+
</dependency>
103+
<dependency>
104+
<groupId>io.opencensus</groupId>
105+
<artifactId>opencensus-api</artifactId>
106+
</dependency>
107+
<dependency>
108+
<groupId>io.opencensus</groupId>
109+
<artifactId>opencensus-contrib-http-util</artifactId>
110+
</dependency>
111+
87112
<dependency>
88113
<groupId>com.google.guava</groupId>
89114
<artifactId>guava-testlib</artifactId>
@@ -104,17 +129,15 @@
104129
<artifactId>mockito-all</artifactId>
105130
<scope>test</scope>
106131
</dependency>
107-
<dependency>
108-
<groupId>com.google.j2objc</groupId>
109-
<artifactId>j2objc-annotations</artifactId>
110-
</dependency>
111132
<dependency>
112133
<groupId>io.opencensus</groupId>
113-
<artifactId>opencensus-api</artifactId>
134+
<artifactId>opencensus-impl</artifactId>
135+
<scope>test</scope>
114136
</dependency>
115137
<dependency>
116138
<groupId>io.opencensus</groupId>
117-
<artifactId>opencensus-contrib-http-util</artifactId>
139+
<artifactId>opencensus-testing</artifactId>
140+
<scope>test</scope>
118141
</dependency>
119142
</dependencies>
120143
</project>

‎google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1013,6 +1013,8 @@ public HttpResponse execute() throws IOException {
10131013
if (!retryOnExecuteIOException
10141014
&& (ioExceptionHandler == null
10151015
|| !ioExceptionHandler.handleIOException(this, retryRequest))) {
1016+
// static analysis shows response is always null here
1017+
span.end(OpenCensusUtils.getEndSpanOptions(null));
10161018
throw e;
10171019
}
10181020
// Save the exception in case the retries do not work and we need to re-throw it later.

‎google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1222,13 +1222,9 @@ public void testExecute_curlLogger() throws Exception {
12221222
for (String message : recorder.messages()) {
12231223
if (message.startsWith("curl")) {
12241224
found = true;
1225-
assertEquals(
1226-
"curl -v --compressed -H 'Accept-Encoding: gzip' -H 'User-Agent: "
1227-
+ "Google-HTTP-Java-Client/"
1228-
+ HttpRequest.VERSION
1229-
+ " (gzip)"
1230-
+ "' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c'",
1231-
message);
1225+
assertTrue(message.contains("curl -v --compressed -H 'Accept-Encoding: gzip'"));
1226+
assertTrue(message.contains("-H 'User-Agent: Google-HTTP-Java-Client/" + HttpRequest.VERSION + " (gzip)'"));
1227+
assertTrue(message.contains("' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c'"));
12321228
}
12331229
}
12341230
assertTrue(found);
@@ -1253,16 +1249,13 @@ public void testExecute_curlLoggerWithContentEncoding() throws Exception {
12531249
.execute();
12541250

12551251
boolean found = false;
1256-
final String expectedCurlLog =
1257-
"curl -v --compressed -X POST -H 'Accept-Encoding: gzip' "
1258-
+ "-H 'User-Agent: "
1259-
+ HttpRequest.USER_AGENT_SUFFIX
1260-
+ "' -H 'Content-Type: text/plain; charset=UTF-8' -H 'Content-Encoding: gzip' "
1261-
+ "-d '@-' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c' << $$$";
12621252
for (String message : recorder.messages()) {
12631253
if (message.startsWith("curl")) {
12641254
found = true;
1265-
assertEquals(expectedCurlLog, message);
1255+
assertTrue(message.contains("curl -v --compressed -X POST -H 'Accept-Encoding: gzip'"));
1256+
assertTrue(message.contains("-H 'User-Agent: " + HttpRequest.USER_AGENT_SUFFIX + "'"));
1257+
assertTrue(message.contains("-H 'Content-Type: text/plain; charset=UTF-8' -H 'Content-Encoding: gzip'"));
1258+
assertTrue(message.contains("-d '@-' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c' << $$$"));
12661259
}
12671260
}
12681261
assertTrue(found);

‎google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
/*
2+
* Copyright 2019 Google LLC
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
5+
* in compliance with the License. You may obtain a copy of the License at
6+
*
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
*
9+
* Unless required by applicable law or agreed to in writing, software distributed under the License
10+
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
11+
* or implied. See the License for the specific language governing permissions and limitations under
12+
* the License.
13+
*/
14+
package com.google.api.client.http;
15+
16+
import com.google.api.client.testing.http.MockHttpTransport;
17+
import com.google.api.client.testing.http.MockLowLevelHttpRequest;
18+
import com.google.api.client.testing.http.MockLowLevelHttpResponse;
19+
import io.opencensus.common.Functions;
20+
import io.opencensus.testing.export.TestHandler;
21+
import io.opencensus.trace.AttributeValue;
22+
import io.opencensus.trace.MessageEvent;
23+
import io.opencensus.trace.Status;
24+
import io.opencensus.trace.Tracing;
25+
import io.opencensus.trace.config.TraceParams;
26+
import io.opencensus.trace.export.SpanData;
27+
import io.opencensus.trace.samplers.Samplers;
28+
import org.junit.After;
29+
import org.junit.Before;
30+
import org.junit.Test;
31+
32+
import java.io.IOException;
33+
import java.util.List;
34+
35+
import static com.google.api.client.http.OpenCensusUtils.SPAN_NAME_HTTP_REQUEST_EXECUTE;
36+
import static org.junit.Assert.assertEquals;
37+
import static org.junit.Assert.assertNotNull;
38+
import static org.junit.Assert.assertTrue;
39+
import static org.junit.Assert.fail;
40+
41+
public class HttpRequestTracingTest {
42+
private static final TestHandler testHandler = new TestHandler();
43+
44+
@Before
45+
public void setupTestTracer() {
46+
Tracing.getExportComponent().getSpanExporter().registerHandler("test", testHandler);
47+
TraceParams params =
48+
Tracing.getTraceConfig()
49+
.getActiveTraceParams()
50+
.toBuilder()
51+
.setSampler(Samplers.alwaysSample())
52+
.build();
53+
Tracing.getTraceConfig().updateActiveTraceParams(params);
54+
}
55+
56+
@After
57+
public void teardownTestTracer() {
58+
Tracing.getExportComponent().getSpanExporter().unregisterHandler("test");
59+
}
60+
61+
@Test(timeout = 20_000L)
62+
public void executeCreatesSpan() throws IOException {
63+
MockLowLevelHttpResponse mockResponse = new MockLowLevelHttpResponse()
64+
.setStatusCode(200);
65+
HttpTransport transport = new MockHttpTransport.Builder()
66+
.setLowLevelHttpResponse(mockResponse)
67+
.build();
68+
HttpRequest request = new HttpRequestFactory(transport, null)
69+
.buildGetRequest(new GenericUrl("https://google.com/"));
70+
request.execute();
71+
72+
// This call blocks - we set a timeout on this test to ensure we don't wait forever
73+
List<SpanData> spans = testHandler.waitForExport(1);
74+
assertEquals(1, spans.size());
75+
SpanData span = spans.get(0);
76+
77+
// Ensure the span name is set
78+
assertEquals(SPAN_NAME_HTTP_REQUEST_EXECUTE, span.getName());
79+
80+
// Ensure we have basic span attributes
81+
assertAttributeEquals(span, "http.path", "/");
82+
assertAttributeEquals(span, "http.host", "google.com");
83+
assertAttributeEquals(span, "http.url", "https://google.com/");
84+
assertAttributeEquals(span, "http.method", "GET");
85+
86+
// Ensure we have a single annotation for starting the first attempt
87+
assertEquals(1, span.getAnnotations().getEvents().size());
88+
89+
// Ensure we have 2 message events, SENT and RECEIVED
90+
assertEquals(2, span.getMessageEvents().getEvents().size());
91+
assertEquals(MessageEvent.Type.SENT, span.getMessageEvents().getEvents().get(0).getEvent().getType());
92+
assertEquals(MessageEvent.Type.RECEIVED, span.getMessageEvents().getEvents().get(1).getEvent().getType());
93+
94+
// Ensure we record the span status as OK
95+
assertEquals(Status.OK, span.getStatus());
96+
}
97+
98+
@Test(timeout = 20_000L)
99+
public void executeExceptionCreatesSpan() throws IOException {
100+
HttpTransport transport = new MockHttpTransport.Builder()
101+
.setLowLevelHttpRequest(new MockLowLevelHttpRequest() {
102+
@Override
103+
public LowLevelHttpResponse execute() throws IOException {
104+
throw new IOException("some IOException");
105+
}
106+
})
107+
.build();
108+
HttpRequest request = new HttpRequestFactory(transport, null)
109+
.buildGetRequest(new GenericUrl("https://google.com/"));
110+
111+
try {
112+
request.execute();
113+
fail("expected to throw an IOException");
114+
} catch (IOException expected) {
115+
}
116+
117+
// This call blocks - we set a timeout on this test to ensure we don't wait forever
118+
List<SpanData> spans = testHandler.waitForExport(1);
119+
assertEquals(1, spans.size());
120+
SpanData span = spans.get(0);
121+
122+
// Ensure the span name is set
123+
assertEquals(SPAN_NAME_HTTP_REQUEST_EXECUTE, span.getName());
124+
125+
// Ensure we have basic span attributes
126+
assertAttributeEquals(span, "http.path", "/");
127+
assertAttributeEquals(span, "http.host", "google.com");
128+
assertAttributeEquals(span, "http.url", "https://google.com/");
129+
assertAttributeEquals(span, "http.method", "GET");
130+
131+
// Ensure we have a single annotation for starting the first attempt
132+
assertEquals(1, span.getAnnotations().getEvents().size());
133+
134+
// Ensure we have 2 message events, SENT and RECEIVED
135+
assertEquals(1, span.getMessageEvents().getEvents().size());
136+
assertEquals(MessageEvent.Type.SENT, span.getMessageEvents().getEvents().get(0).getEvent().getType());
137+
138+
// Ensure we record the span status as UNKNOWN
139+
assertEquals(Status.UNKNOWN, span.getStatus()); }
140+
141+
void assertAttributeEquals(SpanData span, String attributeName, String expectedValue) {
142+
Object attributeValue = span.getAttributes().getAttributeMap().get(attributeName);
143+
assertNotNull("expected span to contain attribute: " + attributeName, attributeValue);
144+
assertTrue(attributeValue instanceof AttributeValue);
145+
String value = ((AttributeValue) attributeValue).match(
146+
Functions.returnToString(),
147+
Functions.returnToString(),
148+
Functions.returnToString(),
149+
Functions.returnToString(),
150+
Functions.</*@Nullable*/ String>returnNull());
151+
assertEquals(expectedValue, value);
152+
}
153+
}

‎pom.xml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,16 @@
242242
<artifactId>opencensus-contrib-http-util</artifactId>
243243
<version>${project.opencensus.version}</version>
244244
</dependency>
245+
<dependency>
246+
<groupId>io.opencensus</groupId>
247+
<artifactId>opencensus-impl</artifactId>
248+
<version>${project.opencensus.version}</version>
249+
</dependency>
250+
<dependency>
251+
<groupId>io.opencensus</groupId>
252+
<artifactId>opencensus-testing</artifactId>
253+
<version>${project.opencensus.version}</version>
254+
</dependency>
245255
</dependencies>
246256
</dependencyManagement>
247257

0 commit comments

Comments
 (0)