Skip to content

Commit 919bf01

Browse files
amarzialiamarziali
authored
Disable grpc client message span by default (#7708)
* Disable grpc client message span by default * Propagate client scope in any case
1 parent 81a7f5b commit 919bf01

File tree

10 files changed

+163
-148
lines changed

10 files changed

+163
-148
lines changed

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import datadog.trace.agent.tooling.Instrumenter;
2020
import datadog.trace.agent.tooling.InstrumenterModule;
2121
import datadog.trace.agent.tooling.muzzle.Reference;
22+
import datadog.trace.api.InstrumenterConfig;
2223
import datadog.trace.bootstrap.InstrumentationContext;
2324
import datadog.trace.bootstrap.instrumentation.api.AgentScope;
2425
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
@@ -27,6 +28,7 @@
2728
import io.grpc.MethodDescriptor;
2829
import io.grpc.Status;
2930
import io.grpc.StatusException;
31+
import java.util.Arrays;
3032
import java.util.Collections;
3133
import java.util.Map;
3234
import net.bytebuddy.asm.Advice;
@@ -90,8 +92,11 @@ public void methodAdvice(MethodTransformer transformer) {
9092
transformer.applyAdvice(
9193
named("close").and(isMethod().and(takesArguments(2))),
9294
getClass().getName() + "$CloseObserver");
93-
transformer.applyAdvice(
94-
named("onNext").or(named("messageRead")), getClass().getName() + "$ReceiveMessages");
95+
if (InstrumenterConfig.get()
96+
.isIntegrationEnabled(Arrays.asList("armeria-grpc-message", "grpc-message"), false)) {
97+
transformer.applyAdvice(
98+
named("onNext").or(named("messageRead")), getClass().getName() + "$ReceiveMessages");
99+
}
95100
}
96101

97102
public static final class CaptureCall {

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

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ abstract class ArmeriaGrpcStreamingTest extends VersionedNamingTestBase {
3636

3737
protected abstract String serverOperation()
3838

39+
protected boolean hasClientMessageSpans() {
40+
false
41+
}
42+
3943
@Override
4044
boolean useStrictTraceWrites() {
4145
false
@@ -46,6 +50,9 @@ abstract class ArmeriaGrpcStreamingTest extends VersionedNamingTestBase {
4650
super.configurePreAgent()
4751
injectSysConfig("dd.trace.grpc.ignored.inbound.methods", "example.Greeter/IgnoreInbound")
4852
injectSysConfig("dd.trace.grpc.ignored.outbound.methods", "example.Greeter/Ignore")
53+
if (hasClientMessageSpans()) {
54+
injectSysConfig("integration.grpc-message.enabled", "true")
55+
}
4956
// here to trigger wrapping to record scheduling time - the logic is trivial so it's enough to verify
5057
// that ClassCastExceptions do not arise from the wrapping
5158
injectSysConfig("dd.profiling.enabled", "true")
@@ -157,7 +164,7 @@ abstract class ArmeriaGrpcStreamingTest extends VersionedNamingTestBase {
157164
}.flatten().sort()
158165

159166
assertTraces(2) {
160-
trace((clientMessageCount * serverMessageCount) + 1) {
167+
trace((hasClientMessageSpans() ? clientMessageCount * serverMessageCount : 0) + 1) {
161168
sortSpansByStart()
162169
span {
163170
operationName clientOperation()
@@ -176,18 +183,20 @@ abstract class ArmeriaGrpcStreamingTest extends VersionedNamingTestBase {
176183
defaultTags()
177184
}
178185
}
179-
(1..(clientMessageCount * serverMessageCount)).each {
180-
span {
181-
operationName "grpc.message"
182-
resourceName "grpc.message"
183-
spanType DDSpanTypes.RPC
184-
childOf span(0)
185-
errored false
186-
tags {
187-
"$Tags.COMPONENT" "armeria-grpc-client"
188-
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
189-
"message.type" "example.Helloworld\$Response"
190-
defaultTagsNoPeerService()
186+
if (hasClientMessageSpans()) {
187+
(1..(clientMessageCount * serverMessageCount)).each {
188+
span {
189+
operationName "grpc.message"
190+
resourceName "grpc.message"
191+
spanType DDSpanTypes.RPC
192+
childOf span(0)
193+
errored false
194+
tags {
195+
"$Tags.COMPONENT" "armeria-grpc-client"
196+
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
197+
"message.type" "example.Helloworld\$Response"
198+
defaultTagsNoPeerService()
199+
}
191200
}
192201
}
193202
}
@@ -246,7 +255,7 @@ abstract class ArmeriaGrpcStreamingTest extends VersionedNamingTestBase {
246255
}
247256
}
248257

249-
class ArmeriaGrpcStreamingV0ForkedTest extends ArmeriaGrpcStreamingTest {
258+
class ArmeriaGrpcStreamingV0Test extends ArmeriaGrpcStreamingTest {
250259

251260
@Override
252261
int version() {
@@ -264,6 +273,13 @@ class ArmeriaGrpcStreamingV0ForkedTest extends ArmeriaGrpcStreamingTest {
264273
}
265274
}
266275

276+
class ArmeriaGrpcStreamingClientMessagesEnabledTest extends ArmeriaGrpcStreamingV0Test {
277+
@Override
278+
protected boolean hasClientMessageSpans() {
279+
true
280+
}
281+
}
282+
267283
class ArmeriaGrpcStreamingV1ForkedTest extends ArmeriaGrpcStreamingTest {
268284

269285
@Override

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

Lines changed: 45 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,18 @@ abstract class ArmeriaGrpcTest extends VersionedNamingTestBase {
6868

6969
protected abstract String serverOperation()
7070

71+
protected boolean hasClientMessageSpans() {
72+
false
73+
}
74+
7175
@Override
7276
protected void configurePreAgent() {
7377
super.configurePreAgent()
7478
injectSysConfig("dd.trace.grpc.ignored.inbound.methods", "example.Greeter/IgnoreInbound")
7579
injectSysConfig("dd.trace.grpc.ignored.outbound.methods", "example.Greeter/Ignore")
80+
if (hasClientMessageSpans()) {
81+
injectSysConfig("integration.armeria-grpc-message.enabled", "true")
82+
}
7683
// here to trigger wrapping to record scheduling time - the logic is trivial so it's enough to verify
7784
// that ClassCastExceptions do not arise from the wrapping
7885
injectSysConfig("dd.profiling.enabled", "true")
@@ -160,7 +167,7 @@ abstract class ArmeriaGrpcTest extends VersionedNamingTestBase {
160167
response.message == "Hello $name"
161168
assertTraces(2) {
162169
sortSpansByStart()
163-
trace(3) {
170+
trace(hasClientMessageSpans() ? 3 : 2) {
164171
basicSpan(it, "parent")
165172
span {
166173
operationName clientOperation()
@@ -183,18 +190,20 @@ abstract class ArmeriaGrpcTest extends VersionedNamingTestBase {
183190
defaultTags()
184191
}
185192
}
186-
span {
187-
operationName "grpc.message"
188-
resourceName "grpc.message"
189-
spanType DDSpanTypes.RPC
190-
childOf span(1)
191-
errored false
192-
measured true
193-
tags {
194-
"$Tags.COMPONENT" "armeria-grpc-client"
195-
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
196-
"message.type" "example.Helloworld\$Response"
197-
defaultTagsNoPeerService()
193+
if (hasClientMessageSpans()) {
194+
span {
195+
operationName "grpc.message"
196+
resourceName "grpc.message"
197+
spanType DDSpanTypes.RPC
198+
childOf span(1)
199+
errored false
200+
measured true
201+
tags {
202+
"$Tags.COMPONENT" "armeria-grpc-client"
203+
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
204+
"message.type" "example.Helloworld\$Response"
205+
defaultTagsNoPeerService()
206+
}
198207
}
199208
}
200209
}
@@ -622,7 +631,7 @@ abstract class ArmeriaGrpcTest extends VersionedNamingTestBase {
622631
response.message == "Hello whatever"
623632
assertTraces(1) {
624633
sortSpansByStart()
625-
trace(2) {
634+
trace(hasClientMessageSpans() ? 2 : 1) {
626635
span {
627636
operationName clientOperation()
628637
resourceName "example.Greeter/IgnoreInbound"
@@ -644,18 +653,20 @@ abstract class ArmeriaGrpcTest extends VersionedNamingTestBase {
644653
defaultTags()
645654
}
646655
}
647-
span {
648-
operationName "grpc.message"
649-
resourceName "grpc.message"
650-
spanType DDSpanTypes.RPC
651-
childOf span(0)
652-
errored false
653-
measured true
654-
tags {
655-
"$Tags.COMPONENT" "armeria-grpc-client"
656-
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
657-
"message.type" "example.Helloworld\$Response"
658-
defaultTagsNoPeerService()
656+
if (hasClientMessageSpans()) {
657+
span {
658+
operationName "grpc.message"
659+
resourceName "grpc.message"
660+
spanType DDSpanTypes.RPC
661+
childOf span(0)
662+
errored false
663+
measured true
664+
tags {
665+
"$Tags.COMPONENT" "armeria-grpc-client"
666+
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
667+
"message.type" "example.Helloworld\$Response"
668+
defaultTagsNoPeerService()
669+
}
659670
}
660671
}
661672
}
@@ -697,6 +708,14 @@ class ArmeriaGrpcDataStreamsEnabledV0Test extends ArmeriaGrpcDataStreamsEnabledF
697708
}
698709
}
699710

711+
712+
class ArmeriaGrpcClientMessagesEnabledTest extends ArmeriaGrpcDataStreamsEnabledV0Test {
713+
@Override
714+
protected boolean hasClientMessageSpans() {
715+
true
716+
}
717+
}
718+
700719
class ArmeriaGrpcDataStreamsEnabledV1ForkedTest extends ArmeriaGrpcDataStreamsEnabledForkedTest {
701720

702721
@Override

‎dd-java-agent/instrumentation/google-pubsub/src/test/groovy/PubSubTest.groovy‎

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,10 @@ abstract class PubSubTest extends VersionedNamingTestBase {
138138
injectSysConfig(GeneralConfig.SERVICE_NAME, "A-service")
139139
injectSysConfig(GeneralConfig.DATA_STREAMS_ENABLED, isDataStreamsEnabled().toString())
140140
if (!shadowGrpcSpans()) {
141-
injectSysConfig(TraceInstrumentationConfig.GOOGLE_PUBSUB_IGNORED_GRPC_METHODS, "")
141+
// only keep Publish and Acknowledge to make this test deterministic
142+
// (things might be called depending on the transport and the test will be flaky otherwise)
143+
injectSysConfig(TraceInstrumentationConfig.GOOGLE_PUBSUB_IGNORED_GRPC_METHODS,
144+
"google.pubsub.v1.Subscriber/ModifyAckDeadline,google.pubsub.v1.Subscriber/Pull,google.pubsub.v1.Subscriber/StreamingPull")
142145
}
143146
}
144147

@@ -169,13 +172,13 @@ abstract class PubSubTest extends VersionedNamingTestBase {
169172

170173
then:
171174
def sendSpan
172-
assertTraces(shadowGrpcSpans() ? 2 : 4, [
175+
assertTraces(shadowGrpcSpans() ? 2 : 3, [
173176
compare : { List<DDSpan> o1, List<DDSpan> o2 ->
174177
// trace will never be empty
175178
o1[0].localRootSpan.getTag(Tags.SPAN_KIND) <=> o2[0].localRootSpan.getTag(Tags.SPAN_KIND)
176179
},
177180
] as Comparator) {
178-
trace(shadowGrpcSpans() ? 2 : 4) {
181+
trace(shadowGrpcSpans() ? 2 : 3) {
179182
sortSpansByStart()
180183
basicSpan(it, "parent")
181184
span {
@@ -203,11 +206,7 @@ abstract class PubSubTest extends VersionedNamingTestBase {
203206
}
204207
if (!shadowGrpcSpans()) {
205208
// Acknowledge
206-
trace(2) {
207-
grpcSpans(it, "A-service", true)
208-
}
209-
// ModifyAckDeadline
210-
trace(2) {
209+
trace(1) {
211210
grpcSpans(it, "A-service", true)
212211
}
213212
}
@@ -287,21 +286,6 @@ abstract class PubSubTest extends VersionedNamingTestBase {
287286
defaultTags()
288287
}
289288
}
290-
traceAssert.span {
291-
serviceName service
292-
operationName "grpc.message"
293-
resourceName "grpc.message"
294-
spanType DDSpanTypes.RPC
295-
errored false
296-
measured true
297-
childOfPrevious()
298-
tags {
299-
"$Tags.COMPONENT" "grpc-client"
300-
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
301-
"message.type" { String }
302-
defaultTagsNoPeerService()
303-
}
304-
}
305289
}
306290
}
307291

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import com.google.auto.service.AutoService;
1313
import datadog.trace.agent.tooling.Instrumenter;
1414
import datadog.trace.agent.tooling.InstrumenterModule;
15+
import datadog.trace.api.InstrumenterConfig;
1516
import datadog.trace.bootstrap.InstrumentationContext;
1617
import datadog.trace.bootstrap.instrumentation.api.AgentScope;
1718
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
@@ -54,7 +55,10 @@ public Map<String, String> contextStore() {
5455
@Override
5556
public void methodAdvice(MethodTransformer transformer) {
5657
transformer.applyAdvice(isConstructor(), getClass().getName() + "$Capture");
57-
transformer.applyAdvice(named("runInContext"), getClass().getName() + "$ReceiveMessages");
58+
if (InstrumenterConfig.get()
59+
.isIntegrationEnabled(Collections.singleton("grpc-message"), false)) {
60+
transformer.applyAdvice(named("runInContext"), getClass().getName() + "$ReceiveMessages");
61+
}
5862
}
5963

6064
public static final class Capture {

0 commit comments

Comments
 (0)