Skip to content

Commit

Permalink
Change SpanStatusExtractor to use a builder that can set status descr…
Browse files Browse the repository at this point in the history 
…iption (open-telemetry#6035)

* Change SpanStatusExtractor to use a builder that can set status descripion

* Refactor SpanStatusExtractor to only support builder approach to setting status

* Revert setting the exception as the status description

* PR feedback

* Re-fix graghql instrumentation span extractor

* Spotless
  • Loading branch information
@HaloFour @RashmiRam
HaloFour authored and RashmiRam committed May 23, 2022
1 parent 1f56285 commit f65f7fd
Show file tree
Hide file tree
Showing 12 changed files with 261 additions and 97 deletions.
12 changes: 9 additions & 3 deletions .../java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package io.opentelemetry.instrumentation.api.instrumenter.http;

import io.opentelemetry.api.trace.StatusCode;
import io.opentelemetry.instrumentation.api.instrumenter.SpanStatusBuilder;
import io.opentelemetry.instrumentation.api.instrumenter.SpanStatusExtractor;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -49,16 +50,21 @@ private HttpSpanStatusExtractor(
}

@Override
public StatusCode extract(REQUEST request, @Nullable RESPONSE response, Throwable error) {
public void extract(
SpanStatusBuilder spanStatusBuilder,
REQUEST request,
@Nullable RESPONSE response,
Throwable error) {
if (response != null) {
Integer statusCode = getter.statusCode(request, response);
if (statusCode != null) {
StatusCode statusCodeObj = statusConverter.statusFromHttpStatus(statusCode);
if (statusCodeObj == StatusCode.ERROR) {
return statusCodeObj;
spanStatusBuilder.setStatus(statusCodeObj);
return;
}
}
}
return SpanStatusExtractor.getDefault().extract(request, response, error);
SpanStatusExtractor.getDefault().extract(spanStatusBuilder, request, response, error);
}
}
60 changes: 41 additions & 19 deletions ...pentelemetry/instrumentation/api/instrumenter/http/HttpClientSpanStatusExtractorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@

package io.opentelemetry.instrumentation.api.instrumenter.http;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.anyMap;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.when;

import io.opentelemetry.api.trace.StatusCode;
import io.opentelemetry.instrumentation.api.instrumenter.SpanStatusBuilder;
import java.util.Collections;
import java.util.Map;
import org.junit.jupiter.api.Test;
Expand All @@ -21,49 +23,69 @@

@ExtendWith(MockitoExtension.class)
class HttpClientSpanStatusExtractorTest {

@Mock private HttpClientAttributesGetter<Map<String, String>, Map<String, String>> getter;

@Mock private SpanStatusBuilder spanStatusBuilder;

@ParameterizedTest
@ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
void hasStatus(int statusCode) {
StatusCode expectedStatusCode = HttpStatusConverter.CLIENT.statusFromHttpStatus(statusCode);
when(getter.statusCode(anyMap(), anyMap())).thenReturn(statusCode);
assertThat(
HttpSpanStatusExtractor.create(getter)
.extract(Collections.emptyMap(), Collections.emptyMap(), null))
.isEqualTo(HttpStatusConverter.CLIENT.statusFromHttpStatus(statusCode));

HttpSpanStatusExtractor.create(getter)
.extract(spanStatusBuilder, Collections.emptyMap(), Collections.emptyMap(), null);

if (expectedStatusCode != StatusCode.UNSET) {
verify(spanStatusBuilder).setStatus(expectedStatusCode);
} else {
verifyNoInteractions(spanStatusBuilder);
}
}

@ParameterizedTest
@ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
void hasStatusAndException(int statusCode) {
StatusCode expectedStatusCode = HttpStatusConverter.CLIENT.statusFromHttpStatus(statusCode);
when(getter.statusCode(anyMap(), anyMap())).thenReturn(statusCode);

// Presence of exception has no effect.
assertThat(
HttpSpanStatusExtractor.create(getter)
.extract(
Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException()))
.isEqualTo(StatusCode.ERROR);
HttpSpanStatusExtractor.create(getter)
.extract(
spanStatusBuilder,
Collections.emptyMap(),
Collections.emptyMap(),
new IllegalStateException("test"));

if (expectedStatusCode != StatusCode.UNSET) {
verify(spanStatusBuilder).setStatus(expectedStatusCode);
} else {
verify(spanStatusBuilder).setStatus(StatusCode.ERROR);
}
}

@Test
void hasNoStatus_fallsBackToDefault_unset() {
when(getter.statusCode(anyMap(), anyMap())).thenReturn(null);

assertThat(
HttpSpanStatusExtractor.create(getter)
.extract(Collections.emptyMap(), Collections.emptyMap(), null))
.isEqualTo(StatusCode.UNSET);
HttpSpanStatusExtractor.create(getter)
.extract(spanStatusBuilder, Collections.emptyMap(), Collections.emptyMap(), null);

verifyNoInteractions(spanStatusBuilder);
}

@Test
void hasNoStatus_fallsBackToDefault_error() {
when(getter.statusCode(anyMap(), anyMap())).thenReturn(null);

assertThat(
HttpSpanStatusExtractor.create(getter)
.extract(
Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException()))
.isEqualTo(StatusCode.ERROR);
HttpSpanStatusExtractor.create(getter)
.extract(
spanStatusBuilder,
Collections.emptyMap(),
Collections.emptyMap(),
new IllegalStateException("test"));

verify(spanStatusBuilder).setStatus(StatusCode.ERROR);
}
}
123 changes: 85 additions & 38 deletions ...a/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@

package io.opentelemetry.instrumentation.api.instrumenter.http;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.anyMap;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.when;

import io.opentelemetry.api.trace.StatusCode;
import io.opentelemetry.instrumentation.api.instrumenter.SpanStatusBuilder;
import java.util.Collections;
import java.util.Map;
import org.junit.jupiter.api.Test;
Expand All @@ -25,80 +27,125 @@ class HttpSpanStatusExtractorTest {

@Mock private HttpClientAttributesGetter<Map<String, String>, Map<String, String>> clientGetter;

@Mock private SpanStatusBuilder spanStatusBuilder;

@ParameterizedTest
@ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 500, 501, 600, 601})
void hasServerStatus(int statusCode) {
StatusCode expectedStatusCode = HttpStatusConverter.SERVER.statusFromHttpStatus(statusCode);
when(serverGetter.statusCode(anyMap(), anyMap())).thenReturn(statusCode);
assertThat(
HttpSpanStatusExtractor.create(serverGetter)
.extract(Collections.emptyMap(), Collections.emptyMap(), null))
.isEqualTo(HttpStatusConverter.SERVER.statusFromHttpStatus(statusCode));

HttpSpanStatusExtractor.create(serverGetter)
.extract(spanStatusBuilder, Collections.emptyMap(), Collections.emptyMap(), null);

if (expectedStatusCode != StatusCode.UNSET) {
verify(spanStatusBuilder).setStatus(expectedStatusCode);
} else {
verifyNoInteractions(spanStatusBuilder);
}
}

@ParameterizedTest
@ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
void hasClientStatus(int statusCode) {
StatusCode expectedStatusCode = HttpStatusConverter.CLIENT.statusFromHttpStatus(statusCode);
when(clientGetter.statusCode(anyMap(), anyMap())).thenReturn(statusCode);
assertThat(
HttpSpanStatusExtractor.create(clientGetter)
.extract(Collections.emptyMap(), Collections.emptyMap(), null))
.isEqualTo(HttpStatusConverter.CLIENT.statusFromHttpStatus(statusCode));

HttpSpanStatusExtractor.create(clientGetter)
.extract(spanStatusBuilder, Collections.emptyMap(), Collections.emptyMap(), null);

if (expectedStatusCode != StatusCode.UNSET) {
verify(spanStatusBuilder).setStatus(expectedStatusCode);
} else {
verifyNoInteractions(spanStatusBuilder);
}
}

@ParameterizedTest
@ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
void hasServerStatusAndException(int statusCode) {
StatusCode expectedStatusCode = HttpStatusConverter.SERVER.statusFromHttpStatus(statusCode);
when(serverGetter.statusCode(anyMap(), anyMap())).thenReturn(statusCode);

// Presence of exception has no effect.
assertThat(
HttpSpanStatusExtractor.create(serverGetter)
.extract(
Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException()))
.isEqualTo(StatusCode.ERROR);
HttpSpanStatusExtractor.create(serverGetter)
.extract(
spanStatusBuilder,
Collections.emptyMap(),
Collections.emptyMap(),
new IllegalStateException("test"));

if (expectedStatusCode == StatusCode.ERROR) {
verify(spanStatusBuilder).setStatus(expectedStatusCode);
} else {
verify(spanStatusBuilder).setStatus(StatusCode.ERROR);
}
}

@ParameterizedTest
@ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
void hasClientStatusAndException(int statusCode) {
StatusCode expectedStatusCode = HttpStatusConverter.CLIENT.statusFromHttpStatus(statusCode);
when(clientGetter.statusCode(anyMap(), anyMap())).thenReturn(statusCode);

// Presence of exception has no effect.
assertThat(
HttpSpanStatusExtractor.create(clientGetter)
.extract(
Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException()))
.isEqualTo(StatusCode.ERROR);
HttpSpanStatusExtractor.create(clientGetter)
.extract(
spanStatusBuilder,
Collections.emptyMap(),
Collections.emptyMap(),
new IllegalStateException("test"));

if (expectedStatusCode == StatusCode.ERROR) {
verify(spanStatusBuilder).setStatus(expectedStatusCode);
} else {
verify(spanStatusBuilder).setStatus(StatusCode.ERROR);
}
}

@Test
void hasNoStatus_fallsBackToDefault_unset() {
when(clientGetter.statusCode(anyMap(), anyMap())).thenReturn(null);
void hasNoServerStatus_fallsBackToDefault_unset() {
when(serverGetter.statusCode(anyMap(), anyMap())).thenReturn(null);

assertThat(
HttpSpanStatusExtractor.create(serverGetter)
.extract(Collections.emptyMap(), Collections.emptyMap(), null))
.isEqualTo(StatusCode.UNSET);
assertThat(
HttpSpanStatusExtractor.create(clientGetter)
.extract(Collections.emptyMap(), Collections.emptyMap(), null))
.isEqualTo(StatusCode.UNSET);
HttpSpanStatusExtractor.create(serverGetter)
.extract(spanStatusBuilder, Collections.emptyMap(), Collections.emptyMap(), null);

verifyNoInteractions(spanStatusBuilder);
}

@Test
void hasNoStatus_fallsBackToDefault_error() {
void hasNoClientStatus_fallsBackToDefault_unset() {
when(clientGetter.statusCode(anyMap(), anyMap())).thenReturn(null);

HttpSpanStatusExtractor.create(clientGetter)
.extract(spanStatusBuilder, Collections.emptyMap(), Collections.emptyMap(), null);

verifyNoInteractions(spanStatusBuilder);
}

@Test
void hasNoServerStatus_fallsBackToDefault_error() {
when(serverGetter.statusCode(anyMap(), anyMap())).thenReturn(null);

StatusCode serverStatusCode =
HttpSpanStatusExtractor.create(serverGetter)
.extract(Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException());
assertThat(serverStatusCode).isEqualTo(StatusCode.ERROR);
HttpSpanStatusExtractor.create(serverGetter)
.extract(
spanStatusBuilder,
Collections.emptyMap(),
Collections.emptyMap(),
new IllegalStateException("test"));
verify(spanStatusBuilder).setStatus(StatusCode.ERROR);
}

@Test
void hasNoClientStatus_fallsBackToDefault_error() {
when(clientGetter.statusCode(anyMap(), anyMap())).thenReturn(null);

StatusCode clientStatusCode =
HttpSpanStatusExtractor.create(clientGetter)
.extract(Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException());
assertThat(clientStatusCode).isEqualTo(StatusCode.ERROR);
HttpSpanStatusExtractor.create(clientGetter)
.extract(
spanStatusBuilder,
Collections.emptyMap(),
Collections.emptyMap(),
new IllegalStateException("test"));
verify(spanStatusBuilder).setStatus(StatusCode.ERROR);
}
}
11 changes: 7 additions & 4 deletions ...in/java/io/opentelemetry/instrumentation/api/instrumenter/DefaultSpanStatusExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@ final class DefaultSpanStatusExtractor<REQUEST, RESPONSE>
static final SpanStatusExtractor<Object, Object> INSTANCE = new DefaultSpanStatusExtractor<>();

@Override
public StatusCode extract(
REQUEST request, @Nullable RESPONSE response, @Nullable Throwable error) {
public void extract(
SpanStatusBuilder spanStatusBuilder,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {

if (error != null) {
return StatusCode.ERROR;
spanStatusBuilder.setStatus(StatusCode.ERROR);
}
return StatusCode.UNSET;
}
}
7 changes: 2 additions & 5 deletions ...ion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanBuilder;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.api.trace.StatusCode;
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics;
Expand Down Expand Up @@ -232,10 +231,8 @@ private void doEnd(
}
}

StatusCode statusCode = spanStatusExtractor.extract(request, response, error);
if (statusCode != StatusCode.UNSET) {
span.setStatus(statusCode);
}
SpanStatusBuilder spanStatusBuilder = new SpanStatusBuilderImpl(span);
spanStatusExtractor.extract(spanStatusBuilder, request, response, error);

if (endTime != null) {
span.end(endTime);
Expand Down
Loading

0 comments on commit f65f7fd

Please sign in to comment.