Skip to content

Commit a50c133

Browse files
breedx-splktrask
andauthored
Don't report 400 level as error for SERVER spans (#4403)
* don't report 400 level as error for server spans * fix HttpServerTest base class * fix JspInstrumentationForward test * split HttpStatusConverter into client and server implementations, and create two HttpSpanStatusExtractor.create methods, one for server and one for client. * rebase * fix test * spotless * fix test * remove unused * use strongly typed attributes converters and rename to overloaded create() * fix tests * remove redundant assert Co-authored-by: Trask Stalnaker <[email protected]>
1 parent 5d7c0dd commit a50c133

File tree

21 files changed

+315
-63
lines changed

21 files changed

+315
-63
lines changed

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

+2-3
Original file line numberDiff line numberDiff line change
@@ -183,11 +183,10 @@ public void end(
183183
span.recordException(error);
184184
}
185185

186-
UnsafeAttributes attributesBuilder = new UnsafeAttributes();
186+
UnsafeAttributes attributes = new UnsafeAttributes();
187187
for (AttributesExtractor<? super REQUEST, ? super RESPONSE> extractor : attributesExtractors) {
188-
extractor.onEnd(attributesBuilder, request, response, error);
188+
extractor.onEnd(attributes, request, response, error);
189189
}
190-
Attributes attributes = attributesBuilder;
191190
span.setAllAttributes(attributes);
192191

193192
Instant endTime = null;

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractor.java

+18-4
Original file line numberDiff line numberDiff line change
@@ -19,30 +19,44 @@
1919
public final class HttpSpanStatusExtractor<REQUEST, RESPONSE>
2020
implements SpanStatusExtractor<REQUEST, RESPONSE> {
2121

22+
private final HttpStatusConverter statusConverter;
23+
24+
/**
25+
* Returns the {@link SpanStatusExtractor} for HTTP requests, which will use the HTTP status code
26+
* to determine the {@link StatusCode} if available or fallback to {@linkplain #getDefault() the
27+
* default status} otherwise.
28+
*/
29+
public static <REQUEST, RESPONSE> SpanStatusExtractor<REQUEST, RESPONSE> create(
30+
HttpClientAttributesExtractor<? super REQUEST, ? super RESPONSE> attributesExtractor) {
31+
return new HttpSpanStatusExtractor<>(attributesExtractor, HttpStatusConverter.CLIENT);
32+
}
33+
2234
/**
2335
* Returns the {@link SpanStatusExtractor} for HTTP requests, which will use the HTTP status code
2436
* to determine the {@link StatusCode} if available or fallback to {@linkplain #getDefault() the
2537
* default status} otherwise.
2638
*/
2739
public static <REQUEST, RESPONSE> SpanStatusExtractor<REQUEST, RESPONSE> create(
28-
HttpCommonAttributesExtractor<? super REQUEST, ? super RESPONSE> attributesExtractor) {
29-
return new HttpSpanStatusExtractor<>(attributesExtractor);
40+
HttpServerAttributesExtractor<? super REQUEST, ? super RESPONSE> attributesExtractor) {
41+
return new HttpSpanStatusExtractor<>(attributesExtractor, HttpStatusConverter.SERVER);
3042
}
3143

3244
private final HttpCommonAttributesExtractor<? super REQUEST, ? super RESPONSE>
3345
attributesExtractor;
3446

3547
private HttpSpanStatusExtractor(
36-
HttpCommonAttributesExtractor<? super REQUEST, ? super RESPONSE> attributesExtractor) {
48+
HttpCommonAttributesExtractor<? super REQUEST, ? super RESPONSE> attributesExtractor,
49+
HttpStatusConverter statusConverter) {
3750
this.attributesExtractor = attributesExtractor;
51+
this.statusConverter = statusConverter;
3852
}
3953

4054
@Override
4155
public StatusCode extract(REQUEST request, @Nullable RESPONSE response, Throwable error) {
4256
if (response != null) {
4357
Integer statusCode = attributesExtractor.statusCode(request, response);
4458
if (statusCode != null) {
45-
StatusCode statusCodeObj = HttpStatusConverter.statusFromHttpStatus(statusCode);
59+
StatusCode statusCodeObj = statusConverter.statusFromHttpStatus(statusCode);
4660
if (statusCodeObj == StatusCode.ERROR) {
4761
return statusCodeObj;
4862
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.instrumentation.api.tracer;
7+
8+
import io.opentelemetry.api.trace.StatusCode;
9+
10+
final class HttpClientStatusConverter implements HttpStatusConverter {
11+
12+
static final HttpStatusConverter INSTANCE = new HttpClientStatusConverter();
13+
14+
// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#status
15+
@Override
16+
public StatusCode statusFromHttpStatus(int httpStatus) {
17+
if (httpStatus >= 100 && httpStatus < 400) {
18+
return StatusCode.UNSET;
19+
}
20+
21+
return StatusCode.ERROR;
22+
}
23+
24+
private HttpClientStatusConverter() {}
25+
}

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ protected void onResponse(Span span, RESPONSE response) {
211211
Integer status = status(response);
212212
if (status != null) {
213213
span.setAttribute(SemanticAttributes.HTTP_STATUS_CODE, (long) status);
214-
StatusCode statusCode = HttpStatusConverter.statusFromHttpStatus(status);
214+
StatusCode statusCode = HttpStatusConverter.CLIENT.statusFromHttpStatus(status);
215215
if (statusCode != StatusCode.UNSET) {
216216
span.setStatus(statusCode);
217217
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.instrumentation.api.tracer;
7+
8+
import io.opentelemetry.api.trace.StatusCode;
9+
10+
final class HttpServerStatusConverter implements HttpStatusConverter {
11+
12+
static final HttpStatusConverter INSTANCE = new HttpServerStatusConverter();
13+
14+
// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#status
15+
@Override
16+
public StatusCode statusFromHttpStatus(int httpStatus) {
17+
if (httpStatus >= 100 && httpStatus < 500) {
18+
return StatusCode.UNSET;
19+
}
20+
21+
return StatusCode.ERROR;
22+
}
23+
24+
private HttpServerStatusConverter() {}
25+
}

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ private static String extractIpAddress(String forwarded, int start) {
276276

277277
private static void setStatus(Span span, int status) {
278278
span.setAttribute(SemanticAttributes.HTTP_STATUS_CODE, (long) status);
279-
StatusCode statusCode = HttpStatusConverter.statusFromHttpStatus(status);
279+
StatusCode statusCode = HttpStatusConverter.SERVER.statusFromHttpStatus(status);
280280
if (statusCode != StatusCode.UNSET) {
281281
span.setStatus(statusCode);
282282
}

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpStatusConverter.java

+4-10
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,10 @@
77

88
import io.opentelemetry.api.trace.StatusCode;
99

10-
public final class HttpStatusConverter {
10+
public interface HttpStatusConverter {
1111

12-
// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#status
13-
public static StatusCode statusFromHttpStatus(int httpStatus) {
14-
if (httpStatus >= 100 && httpStatus < 400) {
15-
return StatusCode.UNSET;
16-
}
12+
HttpStatusConverter SERVER = HttpServerStatusConverter.INSTANCE;
13+
HttpStatusConverter CLIENT = HttpClientStatusConverter.INSTANCE;
1714

18-
return StatusCode.ERROR;
19-
}
20-
21-
private HttpStatusConverter() {}
15+
StatusCode statusFromHttpStatus(int httpStatus);
2216
}

instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpStatusConverterTest.groovy instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientStatusConverterTest.groovy

+2-2
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,11 @@ package io.opentelemetry.instrumentation.api.tracer
88
import io.opentelemetry.api.trace.StatusCode
99
import spock.lang.Specification
1010

11-
class HttpStatusConverterTest extends Specification {
11+
class HttpClientStatusConverterTest extends Specification {
1212

1313
def "test HTTP #httpStatus to OTel #expectedStatus"() {
1414
when:
15-
def status = HttpStatusConverter.statusFromHttpStatus(httpStatus)
15+
def status = HttpStatusConverter.CLIENT.statusFromHttpStatus(httpStatus)
1616

1717
then:
1818
status == expectedStatus

instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy

+1-1
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ class HttpClientTracerTest extends BaseTracerTest {
111111
def "test onResponse"() {
112112
setup:
113113
def tracer = newTracer()
114-
def statusCode = status != null ? HttpStatusConverter.statusFromHttpStatus(status) : null
114+
def statusCode = status != null ? HttpStatusConverter.CLIENT.statusFromHttpStatus(status) : null
115115

116116
when:
117117
tracer.onResponse(span, resp)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.instrumentation.api.tracer
7+
8+
import io.opentelemetry.api.trace.StatusCode
9+
import spock.lang.Specification
10+
11+
class HttpServerStatusConverterTest extends Specification {
12+
13+
def "test HTTP #httpStatus to OTel #expectedStatus"() {
14+
when:
15+
def status = HttpStatusConverter.SERVER.statusFromHttpStatus(httpStatus)
16+
17+
then:
18+
status == expectedStatus
19+
20+
// https://en.wikipedia.org/wiki/List_of_HTTP_status_codes
21+
where:
22+
httpStatus | expectedStatus
23+
100 | StatusCode.UNSET
24+
101 | StatusCode.UNSET
25+
102 | StatusCode.UNSET
26+
103 | StatusCode.UNSET
27+
28+
200 | StatusCode.UNSET
29+
201 | StatusCode.UNSET
30+
202 | StatusCode.UNSET
31+
203 | StatusCode.UNSET
32+
204 | StatusCode.UNSET
33+
205 | StatusCode.UNSET
34+
206 | StatusCode.UNSET
35+
207 | StatusCode.UNSET
36+
208 | StatusCode.UNSET
37+
226 | StatusCode.UNSET
38+
39+
300 | StatusCode.UNSET
40+
301 | StatusCode.UNSET
41+
302 | StatusCode.UNSET
42+
303 | StatusCode.UNSET
43+
304 | StatusCode.UNSET
44+
305 | StatusCode.UNSET
45+
306 | StatusCode.UNSET
46+
307 | StatusCode.UNSET
47+
308 | StatusCode.UNSET
48+
49+
400 | StatusCode.UNSET
50+
401 | StatusCode.UNSET
51+
403 | StatusCode.UNSET
52+
404 | StatusCode.UNSET
53+
405 | StatusCode.UNSET
54+
406 | StatusCode.UNSET
55+
407 | StatusCode.UNSET
56+
408 | StatusCode.UNSET
57+
409 | StatusCode.UNSET
58+
410 | StatusCode.UNSET
59+
411 | StatusCode.UNSET
60+
412 | StatusCode.UNSET
61+
413 | StatusCode.UNSET
62+
414 | StatusCode.UNSET
63+
415 | StatusCode.UNSET
64+
416 | StatusCode.UNSET
65+
417 | StatusCode.UNSET
66+
418 | StatusCode.UNSET
67+
421 | StatusCode.UNSET
68+
422 | StatusCode.UNSET
69+
423 | StatusCode.UNSET
70+
424 | StatusCode.UNSET
71+
425 | StatusCode.UNSET
72+
426 | StatusCode.UNSET
73+
428 | StatusCode.UNSET
74+
429 | StatusCode.UNSET
75+
431 | StatusCode.UNSET
76+
451 | StatusCode.UNSET
77+
78+
500 | StatusCode.ERROR
79+
501 | StatusCode.ERROR
80+
502 | StatusCode.ERROR
81+
503 | StatusCode.ERROR
82+
504 | StatusCode.ERROR
83+
505 | StatusCode.ERROR
84+
506 | StatusCode.ERROR
85+
507 | StatusCode.ERROR
86+
508 | StatusCode.ERROR
87+
510 | StatusCode.ERROR
88+
511 | StatusCode.ERROR
89+
90+
// Don't exist
91+
99 | StatusCode.ERROR
92+
600 | StatusCode.ERROR
93+
}
94+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.instrumentation.api.instrumenter.http;
7+
8+
import static org.assertj.core.api.Assertions.assertThat;
9+
import static org.mockito.ArgumentMatchers.anyMap;
10+
import static org.mockito.Mockito.when;
11+
12+
import io.opentelemetry.api.trace.StatusCode;
13+
import io.opentelemetry.instrumentation.api.tracer.HttpStatusConverter;
14+
import java.util.Collections;
15+
import java.util.Map;
16+
import org.junit.jupiter.api.Test;
17+
import org.junit.jupiter.api.extension.ExtendWith;
18+
import org.junit.jupiter.params.ParameterizedTest;
19+
import org.junit.jupiter.params.provider.ValueSource;
20+
import org.mockito.Mock;
21+
import org.mockito.junit.jupiter.MockitoExtension;
22+
23+
@ExtendWith(MockitoExtension.class)
24+
class HttpClientSpanStatusExtractorTest {
25+
@Mock private HttpClientAttributesExtractor<Map<String, String>, Map<String, String>> extractor;
26+
27+
@ParameterizedTest
28+
@ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
29+
void hasStatus(int statusCode) {
30+
when(extractor.statusCode(anyMap(), anyMap())).thenReturn(statusCode);
31+
assertThat(
32+
HttpSpanStatusExtractor.create(extractor)
33+
.extract(Collections.emptyMap(), Collections.emptyMap(), null))
34+
.isEqualTo(HttpStatusConverter.CLIENT.statusFromHttpStatus(statusCode));
35+
}
36+
37+
@ParameterizedTest
38+
@ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
39+
void hasStatusAndException(int statusCode) {
40+
when(extractor.statusCode(anyMap(), anyMap())).thenReturn(statusCode);
41+
42+
// Presence of exception has no effect.
43+
assertThat(
44+
HttpSpanStatusExtractor.create(extractor)
45+
.extract(
46+
Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException()))
47+
.isEqualTo(StatusCode.ERROR);
48+
}
49+
50+
@Test
51+
void hasNoStatus_fallsBackToDefault_unset() {
52+
when(extractor.statusCode(anyMap(), anyMap())).thenReturn(null);
53+
54+
assertThat(
55+
HttpSpanStatusExtractor.create(extractor)
56+
.extract(Collections.emptyMap(), Collections.emptyMap(), null))
57+
.isEqualTo(StatusCode.UNSET);
58+
}
59+
60+
@Test
61+
void hasNoStatus_fallsBackToDefault_error() {
62+
when(extractor.statusCode(anyMap(), anyMap())).thenReturn(null);
63+
64+
assertThat(
65+
HttpSpanStatusExtractor.create(extractor)
66+
.extract(
67+
Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException()))
68+
.isEqualTo(StatusCode.ERROR);
69+
}
70+
}

0 commit comments

Comments
 (0)