Skip to content

Commit 445bbe4

Browse files
amarzialiamarziali
authored
Fix grpc server error mark (#7505)
1 parent 40f624d commit 445bbe4

File tree

6 files changed

+68
-32
lines changed

6 files changed

+68
-32
lines changed

‎dd-java-agent/instrumentation/armeria-grpc/src/main/java/datadog/trace/instrumentation/armeria/grpc/client/GrpcClientDecorator.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,7 @@ public AgentSpan onClose(final AgentSpan span, final Status status) {
115115

116116
// TODO why is there a mismatch between client / server for calling the onError method?
117117
onError(span, status.getCause());
118-
if (CLIENT_ERROR_STATUSES.get(status.getCode().value())) {
119-
span.setError(true);
120-
}
121-
118+
span.setError(CLIENT_ERROR_STATUSES.get(status.getCode().value()));
122119
return span;
123120
}
124121
}

‎dd-java-agent/instrumentation/armeria-grpc/src/main/java/datadog/trace/instrumentation/armeria/grpc/server/GrpcServerDecorator.java‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
import datadog.trace.bootstrap.instrumentation.decorator.ServerDecorator;
1515
import io.grpc.ServerCall;
1616
import io.grpc.Status;
17+
import io.grpc.StatusException;
18+
import io.grpc.StatusRuntimeException;
1719
import java.util.BitSet;
1820
import java.util.LinkedHashMap;
1921
import java.util.function.Function;
@@ -97,15 +99,27 @@ public <RespT, ReqT> AgentSpan onCall(final AgentSpan span, ServerCall<ReqT, Res
9799
return span;
98100
}
99101

100-
public AgentSpan onClose(final AgentSpan span, final Status status) {
102+
public AgentSpan onStatus(final AgentSpan span, final Status status) {
101103
span.setTag("status.code", status.getCode().name());
102104
span.setTag("status.description", status.getDescription());
105+
return span.setError(SERVER_ERROR_STATUSES.get(status.getCode().value()));
106+
}
103107

104-
if (SERVER_ERROR_STATUSES.get(status.getCode().value())) {
108+
public AgentSpan onClose(final AgentSpan span, final Status status) {
109+
if (status.getCause() != null) {
105110
onError(span, status.getCause());
106-
span.setError(true);
107111
}
112+
return onStatus(span, status);
113+
}
108114

115+
@Override
116+
public AgentSpan onError(AgentSpan span, Throwable throwable) {
117+
super.onError(span, throwable);
118+
if (throwable instanceof StatusRuntimeException) {
119+
onStatus(span, ((StatusRuntimeException) throwable).getStatus());
120+
} else if (throwable instanceof StatusException) {
121+
onStatus(span, ((StatusException) throwable).getStatus());
122+
}
109123
return span;
110124
}
111125
}

‎dd-java-agent/instrumentation/armeria-grpc/src/test/groovy/ArmeriaGrpcTest.groovy‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import static datadog.trace.api.config.TraceInstrumentationConfig.GRPC_SERVER_ERROR_STATUSES
2+
13
import com.google.common.util.concurrent.ListenableFuture
24
import com.linecorp.armeria.client.Clients
35
import com.linecorp.armeria.common.SessionProtocol
@@ -74,6 +76,7 @@ abstract class ArmeriaGrpcTest extends VersionedNamingTestBase {
7476
// here to trigger wrapping to record scheduling time - the logic is trivial so it's enough to verify
7577
// that ClassCastExceptions do not arise from the wrapping
7678
injectSysConfig("dd.profiling.enabled", "true")
79+
injectSysConfig(GRPC_SERVER_ERROR_STATUSES, "2-14", true)
7780
}
7881

7982
@Override
@@ -436,12 +439,14 @@ abstract class ArmeriaGrpcTest extends VersionedNamingTestBase {
436439
resourceName "example.Greeter/SayHello"
437440
spanType DDSpanTypes.RPC
438441
childOf trace(0).get(0)
439-
errored true
442+
errored errorFlag
440443
measured true
441444
tags {
442445
"$Tags.COMPONENT" "armeria-grpc-server"
443446
"$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER
444447
errorTags error.class, error.message
448+
"status.code" "${status.code.name()}"
449+
"status.description" { it == null || String}
445450
"canceled" { true } // 1.0.0 handles cancellation incorrectly so accesting any value
446451
if ({ isDataStreamsEnabled() }) {
447452
"$DDTags.PATHWAY_HASH" { String }
@@ -470,13 +475,15 @@ abstract class ArmeriaGrpcTest extends VersionedNamingTestBase {
470475
serverRule.stop().get()
471476

472477
where:
473-
name | status
474-
"Runtime - cause" | Status.UNKNOWN.withCause(new RuntimeException("some error"))
475-
"Status - cause" | Status.PERMISSION_DENIED.withCause(new RuntimeException("some error"))
476-
"StatusRuntime - cause" | Status.UNIMPLEMENTED.withCause(new RuntimeException("some error"))
477-
"Runtime - description" | Status.UNKNOWN.withDescription("some description")
478-
"Status - description" | Status.PERMISSION_DENIED.withDescription("some description")
479-
"StatusRuntime - description" | Status.UNIMPLEMENTED.withDescription("some description")
478+
name | status | errorFlag
479+
"Runtime - cause" | Status.UNKNOWN.withCause(new RuntimeException("some error")) | true
480+
"Status - cause" | Status.PERMISSION_DENIED.withCause(new RuntimeException("some error")) | true
481+
"StatusRuntime - cause" | Status.UNIMPLEMENTED.withCause(new RuntimeException("some error")) | true
482+
"Runtime - description" | Status.UNKNOWN.withDescription("some description") | true
483+
"Status - description" | Status.PERMISSION_DENIED.withDescription("some description") | true
484+
"StatusRuntime - description" | Status.UNIMPLEMENTED.withDescription("some description") | true
485+
"StatusRuntime - Not errored no cause" | Status.fromCodeValue(15).withDescription("some description") | false
486+
"StatusRuntime - Not errored with cause" | Status.fromCodeValue(15).withCause(new RuntimeException("some error")) | false
480487
}
481488

482489
def "skip binary headers"() {
@@ -672,7 +679,7 @@ abstract class ArmeriaGrpcDataStreamsEnabledForkedTest extends ArmeriaGrpcTest {
672679
}
673680
}
674681

675-
class ArmeriaGrpcDataStreamsEnabledV0ForkedTest extends ArmeriaGrpcDataStreamsEnabledForkedTest {
682+
class ArmeriaGrpcDataStreamsEnabledV0Test extends ArmeriaGrpcDataStreamsEnabledForkedTest {
676683

677684
@Override
678685
int version() {

‎dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/client/GrpcClientDecorator.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,7 @@ public AgentSpan onClose(final AgentSpan span, final Status status) {
115115

116116
// TODO why is there a mismatch between client / server for calling the onError method?
117117
onError(span, status.getCause());
118-
if (CLIENT_ERROR_STATUSES.get(status.getCode().value())) {
119-
span.setError(true);
120-
}
121-
118+
span.setError(CLIENT_ERROR_STATUSES.get(status.getCode().value()));
122119
return span;
123120
}
124121
}

‎dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/server/GrpcServerDecorator.java‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
import datadog.trace.bootstrap.instrumentation.decorator.ServerDecorator;
1515
import io.grpc.ServerCall;
1616
import io.grpc.Status;
17+
import io.grpc.StatusException;
18+
import io.grpc.StatusRuntimeException;
1719
import java.util.BitSet;
1820
import java.util.LinkedHashMap;
1921
import java.util.function.Function;
@@ -97,15 +99,27 @@ public <RespT, ReqT> AgentSpan onCall(final AgentSpan span, ServerCall<ReqT, Res
9799
return span;
98100
}
99101

100-
public AgentSpan onClose(final AgentSpan span, final Status status) {
102+
public AgentSpan onStatus(final AgentSpan span, final Status status) {
101103
span.setTag("status.code", status.getCode().name());
102104
span.setTag("status.description", status.getDescription());
105+
return span.setError(SERVER_ERROR_STATUSES.get(status.getCode().value()));
106+
}
103107

104-
if (SERVER_ERROR_STATUSES.get(status.getCode().value())) {
108+
public AgentSpan onClose(final AgentSpan span, final Status status) {
109+
if (status.getCause() != null) {
105110
onError(span, status.getCause());
106-
span.setError(true);
107111
}
112+
return onStatus(span, status);
113+
}
108114

115+
@Override
116+
public AgentSpan onError(AgentSpan span, Throwable throwable) {
117+
super.onError(span, throwable);
118+
if (throwable instanceof StatusRuntimeException) {
119+
onStatus(span, ((StatusRuntimeException) throwable).getStatus());
120+
} else if (throwable instanceof StatusException) {
121+
onStatus(span, ((StatusException) throwable).getStatus());
122+
}
109123
return span;
110124
}
111125
}

‎dd-java-agent/instrumentation/grpc-1.5/src/test/groovy/GrpcTest.groovy‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import static datadog.trace.api.config.TraceInstrumentationConfig.GRPC_SERVER_ERROR_STATUSES
2+
13
import com.google.common.util.concurrent.MoreExecutors
24
import datadog.trace.agent.test.naming.VersionedNamingTestBase
35
import datadog.trace.api.DDSpanId
@@ -71,6 +73,7 @@ abstract class GrpcTest extends VersionedNamingTestBase {
7173
// here to trigger wrapping to record scheduling time - the logic is trivial so it's enough to verify
7274
// that ClassCastExceptions do not arise from the wrapping
7375
injectSysConfig("dd.profiling.enabled", "true")
76+
injectSysConfig(GRPC_SERVER_ERROR_STATUSES, "2-14", true)
7477
}
7578

7679
def setupSpec() {
@@ -422,11 +425,13 @@ abstract class GrpcTest extends VersionedNamingTestBase {
422425
resourceName "example.Greeter/SayHello"
423426
spanType DDSpanTypes.RPC
424427
childOf trace(0).get(0)
425-
errored true
428+
errored errorFlag
426429
measured true
427430
tags {
428431
"$Tags.COMPONENT" "grpc-server"
429432
"$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER
433+
"status.code" "${status.code.name()}"
434+
"status.description" { it == null || String}
430435
errorTags error.class, error.message
431436
if ({ isDataStreamsEnabled() }) {
432437
"$DDTags.PATHWAY_HASH" { String }
@@ -456,13 +461,15 @@ abstract class GrpcTest extends VersionedNamingTestBase {
456461
server?.shutdownNow()?.awaitTermination()
457462

458463
where:
459-
name | status
460-
"Runtime - cause" | Status.UNKNOWN.withCause(new RuntimeException("some error"))
461-
"Status - cause" | Status.PERMISSION_DENIED.withCause(new RuntimeException("some error"))
462-
"StatusRuntime - cause" | Status.UNIMPLEMENTED.withCause(new RuntimeException("some error"))
463-
"Runtime - description" | Status.UNKNOWN.withDescription("some description")
464-
"Status - description" | Status.PERMISSION_DENIED.withDescription("some description")
465-
"StatusRuntime - description" | Status.UNIMPLEMENTED.withDescription("some description")
464+
name | status | errorFlag
465+
"Runtime - cause" | Status.UNKNOWN.withCause(new RuntimeException("some error")) | true
466+
"Status - cause" | Status.PERMISSION_DENIED.withCause(new RuntimeException("some error")) | true
467+
"StatusRuntime - cause" | Status.UNIMPLEMENTED.withCause(new RuntimeException("some error")) | true
468+
"Runtime - description" | Status.UNKNOWN.withDescription("some description") | true
469+
"Status - description" | Status.PERMISSION_DENIED.withDescription("some description") | true
470+
"StatusRuntime - description" | Status.UNIMPLEMENTED.withDescription("some description") | true
471+
"StatusRuntime - Not errored no cause" | Status.fromCodeValue(15).withDescription("some description") | false
472+
"StatusRuntime - Not errored with cause" | Status.fromCodeValue(15).withCause(new RuntimeException("some error")) | false
466473
}
467474

468475
def "skip binary headers"() {
@@ -642,7 +649,7 @@ abstract class GrpcDataStreamsEnabledForkedTest extends GrpcTest {
642649
}
643650
}
644651

645-
class GrpcDataStreamsEnabledV0ForkedTest extends GrpcDataStreamsEnabledForkedTest {
652+
class GrpcDataStreamsEnabledV0Test extends GrpcDataStreamsEnabledForkedTest {
646653

647654
@Override
648655
int version() {

0 commit comments

Comments
 (0)