-
Notifications
You must be signed in to change notification settings - Fork 292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for TRACE_HTTP_CLIENT_TAG_QUERY_STRING
and change default value of HTTP_CLIENT_TAG_QUERY_STRING
to true
#7677
Conversation
TRACE_HTTP_CLIENT_TAG_QUERY_STRING
and change default value of HTTP_CLIENT_TAG_QUERY_STRING
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 53 metrics, 10 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.42.0-SNAPSHOT~4f35efe453, baseline=1.42.0-SNAPSHOT~9d6a07b28b
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.083 s) : 0, 1083223
Total [baseline] (10.423 s) : 0, 10423177
Agent [candidate] (1.077 s) : 0, 1076609
Total [candidate] (10.314 s) : 0, 10313721
section appsec
Agent [baseline] (1.221 s) : 0, 1221383
Total [baseline] (10.645 s) : 0, 10645115
Agent [candidate] (1.213 s) : 0, 1212641
Total [candidate] (10.613 s) : 0, 10612552
section iast
Agent [baseline] (1.201 s) : 0, 1200978
Total [baseline] (10.881 s) : 0, 10880736
Agent [candidate] (1.202 s) : 0, 1202017
Total [candidate] (10.837 s) : 0, 10837278
section profiling
Agent [baseline] (1.281 s) : 0, 1280544
Total [baseline] (10.759 s) : 0, 10758677
Agent [candidate] (1.273 s) : 0, 1272944
Total [candidate] (10.732 s) : 0, 10731910
gantt
title petclinic - break down per module: candidate=1.42.0-SNAPSHOT~4f35efe453, baseline=1.42.0-SNAPSHOT~9d6a07b28b
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (689.451 ms) : 0, 689451
BytebuddyAgent [candidate] (684.327 ms) : 0, 684327
GlobalTracer [baseline] (315.297 ms) : 0, 315297
GlobalTracer [candidate] (314.216 ms) : 0, 314216
AppSec [baseline] (54.254 ms) : 0, 54254
AppSec [candidate] (53.975 ms) : 0, 53975
Remote Config [baseline] (666.182 µs) : 0, 666
Remote Config [candidate] (684.141 µs) : 0, 684
Telemetry [baseline] (9.861 ms) : 0, 9861
Telemetry [candidate] (9.784 ms) : 0, 9784
section appsec
BytebuddyAgent [baseline] (708.665 ms) : 0, 708665
BytebuddyAgent [candidate] (704.219 ms) : 0, 704219
GlobalTracer [baseline] (314.093 ms) : 0, 314093
GlobalTracer [candidate] (311.296 ms) : 0, 311296
AppSec [baseline] (165.475 ms) : 0, 165475
AppSec [candidate] (165.267 ms) : 0, 165267
IAST [baseline] (20.739 ms) : 0, 20739
IAST [candidate] (19.116 ms) : 0, 19116
Remote Config [baseline] (637.0 µs) : 0, 637
Remote Config [candidate] (633.833 µs) : 0, 634
Telemetry [baseline] (8.401 ms) : 0, 8401
Telemetry [candidate] (8.009 ms) : 0, 8009
section iast
BytebuddyAgent [baseline] (800.005 ms) : 0, 800005
BytebuddyAgent [candidate] (801.429 ms) : 0, 801429
GlobalTracer [baseline] (302.105 ms) : 0, 302105
GlobalTracer [candidate] (302.333 ms) : 0, 302333
AppSec [baseline] (57.317 ms) : 0, 57317
AppSec [candidate] (56.223 ms) : 0, 56223
IAST [baseline] (19.882 ms) : 0, 19882
IAST [candidate] (20.545 ms) : 0, 20545
Remote Config [baseline] (612.512 µs) : 0, 613
Remote Config [candidate] (585.634 µs) : 0, 586
Telemetry [baseline] (7.468 ms) : 0, 7468
Telemetry [candidate] (7.261 ms) : 0, 7261
section profiling
BytebuddyAgent [baseline] (683.727 ms) : 0, 683727
BytebuddyAgent [candidate] (679.029 ms) : 0, 679029
GlobalTracer [baseline] (398.743 ms) : 0, 398743
GlobalTracer [candidate] (396.882 ms) : 0, 396882
AppSec [baseline] (54.583 ms) : 0, 54583
AppSec [candidate] (54.123 ms) : 0, 54123
Remote Config [baseline] (668.435 µs) : 0, 668
Remote Config [candidate] (662.821 µs) : 0, 663
Telemetry [baseline] (13.369 ms) : 0, 13369
Telemetry [candidate] (11.814 ms) : 0, 11814
ProfilingAgent [baseline] (90.534 ms) : 0, 90534
ProfilingAgent [candidate] (91.736 ms) : 0, 91736
Profiling [baseline] (90.557 ms) : 0, 90557
Profiling [candidate] (91.76 ms) : 0, 91760
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.42.0-SNAPSHOT~4f35efe453, baseline=1.42.0-SNAPSHOT~9d6a07b28b
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.08 s) : 0, 1080144
Total [baseline] (8.578 s) : 0, 8577618
Agent [candidate] (1.076 s) : 0, 1075737
Total [candidate] (8.544 s) : 0, 8544495
section iast
Agent [baseline] (1.203 s) : 0, 1202543
Total [baseline] (9.076 s) : 0, 9075609
Agent [candidate] (1.201 s) : 0, 1201052
Total [candidate] (9.085 s) : 0, 9085327
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.199 s) : 0, 1199158
Total [baseline] (9.056 s) : 0, 9056170
Agent [candidate] (1.199 s) : 0, 1199434
Total [candidate] (9.086 s) : 0, 9085608
section iast_TELEMETRY_OFF
Agent [baseline] (1.198 s) : 0, 1198121
Total [baseline] (9.085 s) : 0, 9084517
Agent [candidate] (1.195 s) : 0, 1194990
Total [candidate] (9.098 s) : 0, 9097736
gantt
title insecure-bank - break down per module: candidate=1.42.0-SNAPSHOT~4f35efe453, baseline=1.42.0-SNAPSHOT~9d6a07b28b
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (686.773 ms) : 0, 686773
BytebuddyAgent [candidate] (684.974 ms) : 0, 684974
GlobalTracer [baseline] (314.96 ms) : 0, 314960
GlobalTracer [candidate] (313.567 ms) : 0, 313567
AppSec [baseline] (54.334 ms) : 0, 54334
AppSec [candidate] (53.873 ms) : 0, 53873
Remote Config [baseline] (662.021 µs) : 0, 662
Remote Config [candidate] (664.777 µs) : 0, 665
Telemetry [baseline] (9.779 ms) : 0, 9779
Telemetry [candidate] (9.078 ms) : 0, 9078
section iast
BytebuddyAgent [baseline] (801.203 ms) : 0, 801203
BytebuddyAgent [candidate] (800.352 ms) : 0, 800352
GlobalTracer [baseline] (302.461 ms) : 0, 302461
GlobalTracer [candidate] (302.194 ms) : 0, 302194
AppSec [baseline] (56.591 ms) : 0, 56591
AppSec [candidate] (57.248 ms) : 0, 57248
IAST [baseline] (20.687 ms) : 0, 20687
IAST [candidate] (19.675 ms) : 0, 19675
Remote Config [baseline] (609.137 µs) : 0, 609
Remote Config [candidate] (593.538 µs) : 0, 594
Telemetry [baseline] (7.357 ms) : 0, 7357
Telemetry [candidate] (7.344 ms) : 0, 7344
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (798.855 ms) : 0, 798855
BytebuddyAgent [candidate] (798.776 ms) : 0, 798776
GlobalTracer [baseline] (301.869 ms) : 0, 301869
GlobalTracer [candidate] (302.084 ms) : 0, 302084
AppSec [baseline] (56.519 ms) : 0, 56519
AppSec [candidate] (57.226 ms) : 0, 57226
IAST [baseline] (20.488 ms) : 0, 20488
IAST [candidate] (19.743 ms) : 0, 19743
Remote Config [baseline] (594.053 µs) : 0, 594
Remote Config [candidate] (606.879 µs) : 0, 607
Telemetry [baseline] (7.261 ms) : 0, 7261
Telemetry [candidate] (7.39 ms) : 0, 7390
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (797.149 ms) : 0, 797149
BytebuddyAgent [candidate] (795.616 ms) : 0, 795616
GlobalTracer [baseline] (302.571 ms) : 0, 302571
GlobalTracer [candidate] (301.473 ms) : 0, 301473
AppSec [baseline] (57.382 ms) : 0, 57382
AppSec [candidate] (56.314 ms) : 0, 56314
IAST [baseline] (19.497 ms) : 0, 19497
IAST [candidate] (20.186 ms) : 0, 20186
Remote Config [baseline] (593.199 µs) : 0, 593
Remote Config [candidate] (583.645 µs) : 0, 584
Telemetry [baseline] (7.324 ms) : 0, 7324
Telemetry [candidate] (7.24 ms) : 0, 7240
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 13 metrics, 15 unstable metrics. Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.42.0-SNAPSHOT~4f35efe453, baseline=1.42.0-SNAPSHOT~9d6a07b28b
dateFormat X
axisFormat %s
section baseline
no_agent (383.429 µs) : 364, 403
. : milestone, 383,
iast (503.188 µs) : 482, 525
. : milestone, 503,
iast_FULL (651.962 µs) : 631, 673
. : milestone, 652,
iast_GLOBAL (524.895 µs) : 504, 546
. : milestone, 525,
iast_HARDCODED_SECRET_DISABLED (486.672 µs) : 466, 508
. : milestone, 487,
iast_INACTIVE (462.421 µs) : 441, 484
. : milestone, 462,
iast_TELEMETRY_OFF (478.872 µs) : 458, 500
. : milestone, 479,
tracing (448.084 µs) : 427, 469
. : milestone, 448,
section candidate
no_agent (381.924 µs) : 362, 401
. : milestone, 382,
iast (495.208 µs) : 474, 517
. : milestone, 495,
iast_FULL (647.846 µs) : 626, 669
. : milestone, 648,
iast_GLOBAL (525.094 µs) : 503, 547
. : milestone, 525,
iast_HARDCODED_SECRET_DISABLED (496.095 µs) : 475, 518
. : milestone, 496,
iast_INACTIVE (451.757 µs) : 431, 473
. : milestone, 452,
iast_TELEMETRY_OFF (482.317 µs) : 461, 504
. : milestone, 482,
tracing (446.278 µs) : 426, 467
. : milestone, 446,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.42.0-SNAPSHOT~4f35efe453, baseline=1.42.0-SNAPSHOT~9d6a07b28b
dateFormat X
axisFormat %s
section baseline
no_agent (1.338 ms) : 1319, 1358
. : milestone, 1338,
appsec (1.753 ms) : 1727, 1778
. : milestone, 1753,
appsec_no_iast (1.73 ms) : 1705, 1754
. : milestone, 1730,
iast (1.492 ms) : 1469, 1515
. : milestone, 1492,
profiling (1.517 ms) : 1493, 1541
. : milestone, 1517,
tracing (1.493 ms) : 1466, 1520
. : milestone, 1493,
section candidate
no_agent (1.348 ms) : 1328, 1368
. : milestone, 1348,
appsec (1.715 ms) : 1692, 1737
. : milestone, 1715,
appsec_no_iast (1.713 ms) : 1688, 1738
. : milestone, 1713,
iast (1.491 ms) : 1469, 1513
. : milestone, 1491,
profiling (1.49 ms) : 1467, 1514
. : milestone, 1490,
tracing (1.486 ms) : 1463, 1510
. : milestone, 1486,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.42.0-SNAPSHOT~4f35efe453, baseline=1.42.0-SNAPSHOT~9d6a07b28b
dateFormat X
axisFormat %s
section baseline
no_agent (15.775 s) : 15775000, 15775000
. : milestone, 15775000,
appsec (15.253 s) : 15253000, 15253000
. : milestone, 15253000,
iast (18.883 s) : 18883000, 18883000
. : milestone, 18883000,
iast_GLOBAL (18.129 s) : 18129000, 18129000
. : milestone, 18129000,
profiling (15.71 s) : 15710000, 15710000
. : milestone, 15710000,
tracing (15.222 s) : 15222000, 15222000
. : milestone, 15222000,
section candidate
no_agent (15.091 s) : 15091000, 15091000
. : milestone, 15091000,
appsec (15.217 s) : 15217000, 15217000
. : milestone, 15217000,
iast (19.215 s) : 19215000, 19215000
. : milestone, 19215000,
iast_GLOBAL (18.072 s) : 18072000, 18072000
. : milestone, 18072000,
profiling (15.707 s) : 15707000, 15707000
. : milestone, 15707000,
tracing (15.308 s) : 15308000, 15308000
. : milestone, 15308000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.42.0-SNAPSHOT~4f35efe453, baseline=1.42.0-SNAPSHOT~9d6a07b28b
dateFormat X
axisFormat %s
section baseline
no_agent (1.475 ms) : 1463, 1486
. : milestone, 1475,
appsec (2.357 ms) : 2315, 2399
. : milestone, 2357,
iast (2.096 ms) : 2044, 2148
. : milestone, 2096,
iast_GLOBAL (2.131 ms) : 2079, 2184
. : milestone, 2131,
profiling (1.931 ms) : 1890, 1972
. : milestone, 1931,
tracing (1.931 ms) : 1891, 1971
. : milestone, 1931,
section candidate
no_agent (1.467 ms) : 1456, 1479
. : milestone, 1467,
appsec (2.341 ms) : 2299, 2382
. : milestone, 2341,
iast (2.075 ms) : 2024, 2127
. : milestone, 2075,
iast_GLOBAL (2.124 ms) : 2072, 2177
. : milestone, 2124,
profiling (1.945 ms) : 1903, 1987
. : milestone, 1945,
tracing (1.915 ms) : 1875, 1954
. : milestone, 1915,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the PR ready for review? I can see a bunch of leftovers 🤔
If not, thank for using draft as mentioned in contribution guidelines 🙏
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java
Outdated
Show resolved
Hide resolved
...opentracing/api-0.31/src/main/java/datadog/trace/instrumentation/opentracing31/OTTracer.java
Outdated
Show resolved
Hide resolved
Accidentally marked as ready earlier. Code has been cleaned up and is ready for review! 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About test changes, it feels this duplicate the test url handling for #url
test twice without adding new behavior test. If the goal is to test the setting is well applied, it is already covered through config name alias tests.
If you still want have a test for this behavior, I would recommend creating a parametric tests with all possible user config combinations (new config to null, true, false + old config to new, true, false) and only checking at the value the config returns (from Config.isHttpClientTagQueryString()
).
@@ -98,6 +100,110 @@ class HttpClientDecoratorTest extends ClientDecoratorTest { | |||
req = [url: url == null ? null : new URI(url)] | |||
} | |||
|
|||
def "test url handling for #url : trace env var"() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than differentiating these tests with "default env var" and "trace env var," I'd suggest actually using the env var names (Because "default" didn't mean anything to me until I read "trace"): DD_HTTP_CLIENT_TAG_QUERY_STRING and DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING.
Not sure if this conflicts with Java standards, but just my two cents from a generic code reader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spock test methods does not have to respect Java standards, so we're good here.
But if we really want to test the behavior with all the different config values (I started a discussion with @mhlidd already as it feels more like testing config rather than testing the HTTP client decorator), I would recommend creating test child classes, having the config setting / retrieval in the parent abstract method. -- and here, the class name will have to respect the standards 😉
91ec4e7
to
b8ccaba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the new default behavior is to have TRACE_HTTP_CLIENT_TAG_QUERY_STRING
enabled, you would have to update all the tests that break with this new behavior rather than disabling it with injectSysConfig(TRACE_HTTP_CLIENT_TAG_QUERY_STRING, "false")
-- I'm referring to the integration test updates.
dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java
Show resolved
Hide resolved
...entation/aws-common/src/main/java/datadog/trace/instrumentation/aws/ExpectedQueryParams.java
Outdated
Show resolved
Hide resolved
…ng tests rebasing
merge conflicts rebase
fixing merge conflicts through rebase
95a6728
to
1436014
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great improvements! 🙌
I left a bunch of comments as there are some changes needed but the hardest part is done 😉
dd-java-agent/instrumentation/aws-java-sns-1.0/src/test/groovy/SnsClientTest.groovy
Outdated
Show resolved
Hide resolved
import java.util.Arrays; | ||
import java.util.List; | ||
|
||
public class ExpectedQueryParams { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can set the class as final
and add a private constructor (private ExpectedQueryParams() {}
).
This usually denotes utility classes, letting developers know this class only holds utility methods, and should not be instantiated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, as it is a test class, it would be better to put it under src/test/java
rather than src/main/java
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got it wrong, it is well src/main/java
if you would like to be able to reuse it in some other modules 🙇
dd-java-agent/instrumentation/aws-java-sns-2.0/src/test/groovy/SnsClientTest.groovy
Outdated
Show resolved
Hide resolved
dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/SqsClientTest.groovy
Outdated
Show resolved
Hide resolved
dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/asserts/TagsAssert.groovy
Show resolved
Hide resolved
81a3a5b
to
9ee39fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks for keep improving it PR 👍
TRACE_HTTP_CLIENT_TAG_QUERY_STRING
and change default value of HTTP_CLIENT_TAG_QUERY_STRING
TRACE_HTTP_CLIENT_TAG_QUERY_STRING
and change default value of HTTP_CLIENT_TAG_QUERY_STRING
to true
What Does This Do
Changes the default value of the
HTTP_CLIENT_TAG_QUERY_STRING
to have the value oftrue
instead offalse
to be consistent with other language defaults. Additionally, we add support to handlethe
TRACE_HTTP_CLIENT_TAG_QUERY_STRING
tag as well as the originalHTTP_CLIENT_TAG_QUERY_STRING
in efforts to be more consistent with the tags from other languages as well.Motivation
Our goal is to make the implementation of configuration variables consistent for all languages as part of the config consistency effort listed in the following RFC.
Additional Notes
In Release Notes: Mention the change of the default value of the
HTTP_CLIENT_TAG_QUERY_STRING
totrue
and state that the tag can be manually set to falseContributor Checklist
type:
and (comp:
orinst:
) labels in addition to any usefull labelsclose
,fix
or any linking keywords when referencing an issue.Use
solves
instead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]