Add support for TRACE_HTTP_CLIENT_TAG_QUERY_STRING and change default value of HTTP_CLIENT_TAG_QUERY_STRING
#7677
+111
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mhlidd
changed the title
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 49 metrics, 14 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.40.0-SNAPSHOT~a0a1527b4e, baseline=1.40.0-SNAPSHOT~1392830fdb
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.07 s) : 0, 1069630
Total [baseline] (10.478 s) : 0, 10478300
Agent [candidate] (1.07 s) : 0, 1070188
Total [candidate] (10.404 s) : 0, 10404357
section appsec
Agent [baseline] (1.205 s) : 0, 1204963
Total [baseline] (10.643 s) : 0, 10642749
Agent [candidate] (1.211 s) : 0, 1211142
Total [candidate] (10.668 s) : 0, 10667872
section iast
Agent [baseline] (1.202 s) : 0, 1202056
Total [baseline] (10.894 s) : 0, 10893677
Agent [candidate] (1.194 s) : 0, 1194305
Total [candidate] (10.9 s) : 0, 10900148
section profiling
Agent [baseline] (1.278 s) : 0, 1278489
Total [baseline] (10.591 s) : 0, 10591485
Agent [candidate] (1.269 s) : 0, 1268521
Total [candidate] (10.586 s) : 0, 10585850
gantt
title petclinic - break down per module: candidate=1.40.0-SNAPSHOT~a0a1527b4e, baseline=1.40.0-SNAPSHOT~1392830fdb
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (681.82 ms) : 0, 681820
BytebuddyAgent [candidate] (681.674 ms) : 0, 681674
GlobalTracer [baseline] (311.622 ms) : 0, 311622
GlobalTracer [candidate] (312.015 ms) : 0, 312015
AppSec [baseline] (54.122 ms) : 0, 54122
AppSec [candidate] (54.394 ms) : 0, 54394
Remote Config [baseline] (664.88 µs) : 0, 665
Remote Config [candidate] (678.045 µs) : 0, 678
Telemetry [baseline] (7.676 ms) : 0, 7676
Telemetry [candidate] (7.721 ms) : 0, 7721
section appsec
BytebuddyAgent [baseline] (705.066 ms) : 0, 705066
BytebuddyAgent [candidate] (708.283 ms) : 0, 708283
GlobalTracer [baseline] (304.259 ms) : 0, 304259
GlobalTracer [candidate] (306.062 ms) : 0, 306062
AppSec [baseline] (163.13 ms) : 0, 163130
AppSec [candidate] (163.338 ms) : 0, 163338
Remote Config [baseline] (649.822 µs) : 0, 650
Remote Config [candidate] (644.57 µs) : 0, 645
Telemetry [baseline] (7.793 ms) : 0, 7793
Telemetry [candidate] (8.883 ms) : 0, 8883
IAST [baseline] (20.671 ms) : 0, 20671
IAST [candidate] (20.288 ms) : 0, 20288
section iast
BytebuddyAgent [baseline] (799.655 ms) : 0, 799655
BytebuddyAgent [candidate] (793.624 ms) : 0, 793624
GlobalTracer [baseline] (301.732 ms) : 0, 301732
GlobalTracer [candidate] (300.455 ms) : 0, 300455
AppSec [baseline] (54.382 ms) : 0, 54382
AppSec [candidate] (52.634 ms) : 0, 52634
Remote Config [baseline] (625.996 µs) : 0, 626
Remote Config [candidate] (669.305 µs) : 0, 669
Telemetry [baseline] (7.118 ms) : 0, 7118
Telemetry [candidate] (7.035 ms) : 0, 7035
IAST [baseline] (24.726 ms) : 0, 24726
IAST [candidate] (26.167 ms) : 0, 26167
section profiling
ProfilingAgent [baseline] (97.544 ms) : 0, 97544
ProfilingAgent [candidate] (96.167 ms) : 0, 96167
BytebuddyAgent [baseline] (680.96 ms) : 0, 680960
BytebuddyAgent [candidate] (675.287 ms) : 0, 675287
GlobalTracer [baseline] (397.688 ms) : 0, 397688
GlobalTracer [candidate] (395.518 ms) : 0, 395518
AppSec [baseline] (54.986 ms) : 0, 54986
AppSec [candidate] (54.67 ms) : 0, 54670
Remote Config [baseline] (663.102 µs) : 0, 663
Remote Config [candidate] (660.674 µs) : 0, 661
Telemetry [baseline] (7.572 ms) : 0, 7572
Telemetry [candidate] (7.499 ms) : 0, 7499
Profiling [baseline] (97.567 ms) : 0, 97567
Profiling [candidate] (96.19 ms) : 0, 96190
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.40.0-SNAPSHOT~a0a1527b4e, baseline=1.40.0-SNAPSHOT~1392830fdb
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.076 s) : 0, 1075923
Total [baseline] (8.592 s) : 0, 8592238
Agent [candidate] (1.07 s) : 0, 1069547
Total [candidate] (8.538 s) : 0, 8538366
section iast
Agent [baseline] (1.193 s) : 0, 1192840
Total [baseline] (9.018 s) : 0, 9018454
Agent [candidate] (1.196 s) : 0, 1195580
Total [candidate] (9.035 s) : 0, 9035201
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.205 s) : 0, 1205059
Total [baseline] (9.042 s) : 0, 9042079
Agent [candidate] (1.203 s) : 0, 1203304
Total [candidate] (9.001 s) : 0, 9000589
section iast_TELEMETRY_OFF
Agent [baseline] (1.19 s) : 0, 1190258
Total [baseline] (9.014 s) : 0, 9013589
Agent [candidate] (1.191 s) : 0, 1190960
Total [candidate] (9.024 s) : 0, 9023740
gantt
title insecure-bank - break down per module: candidate=1.40.0-SNAPSHOT~a0a1527b4e, baseline=1.40.0-SNAPSHOT~1392830fdb
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (686.341 ms) : 0, 686341
BytebuddyAgent [candidate] (682.35 ms) : 0, 682350
GlobalTracer [baseline] (312.802 ms) : 0, 312802
GlobalTracer [candidate] (311.233 ms) : 0, 311233
AppSec [baseline] (54.484 ms) : 0, 54484
AppSec [candidate] (53.93 ms) : 0, 53930
Remote Config [baseline] (673.857 µs) : 0, 674
Remote Config [candidate] (666.458 µs) : 0, 666
Telemetry [baseline] (7.799 ms) : 0, 7799
Telemetry [candidate] (7.642 ms) : 0, 7642
section iast
BytebuddyAgent [baseline] (792.734 ms) : 0, 792734
BytebuddyAgent [candidate] (795.508 ms) : 0, 795508
GlobalTracer [baseline] (300.023 ms) : 0, 300023
GlobalTracer [candidate] (300.315 ms) : 0, 300315
AppSec [baseline] (53.919 ms) : 0, 53919
AppSec [candidate] (53.992 ms) : 0, 53992
IAST [baseline] (23.883 ms) : 0, 23883
IAST [candidate] (24.301 ms) : 0, 24301
Remote Config [baseline] (660.789 µs) : 0, 661
Remote Config [candidate] (647.891 µs) : 0, 648
Telemetry [baseline] (7.865 ms) : 0, 7865
Telemetry [candidate] (6.994 ms) : 0, 6994
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (803.622 ms) : 0, 803622
BytebuddyAgent [candidate] (800.151 ms) : 0, 800151
GlobalTracer [baseline] (300.627 ms) : 0, 300627
GlobalTracer [candidate] (302.42 ms) : 0, 302420
AppSec [baseline] (54.421 ms) : 0, 54421
AppSec [candidate] (56.073 ms) : 0, 56073
IAST [baseline] (24.673 ms) : 0, 24673
IAST [candidate] (23.026 ms) : 0, 23026
Remote Config [baseline] (621.51 µs) : 0, 622
Remote Config [candidate] (641.321 µs) : 0, 641
Telemetry [baseline] (7.151 ms) : 0, 7151
Telemetry [candidate] (7.148 ms) : 0, 7148
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (789.89 ms) : 0, 789890
BytebuddyAgent [candidate] (790.742 ms) : 0, 790742
GlobalTracer [baseline] (300.441 ms) : 0, 300441
GlobalTracer [candidate] (300.428 ms) : 0, 300428
AppSec [baseline] (52.863 ms) : 0, 52863
AppSec [candidate] (55.123 ms) : 0, 55123
IAST [baseline] (25.786 ms) : 0, 25786
IAST [candidate] (23.347 ms) : 0, 23347
Remote Config [baseline] (609.88 µs) : 0, 610
Remote Config [candidate] (614.095 µs) : 0, 614
Telemetry [baseline] (6.91 ms) : 0, 6910
Telemetry [candidate] (6.912 ms) : 0, 6912
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 17 unstable metrics. Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.40.0-SNAPSHOT~a0a1527b4e, baseline=1.40.0-SNAPSHOT~1392830fdb
dateFormat X
axisFormat %s
section baseline
no_agent (1.333 ms) : 1313, 1352
. : milestone, 1333,
appsec (1.71 ms) : 1685, 1734
. : milestone, 1710,
appsec_no_iast (1.747 ms) : 1723, 1772
. : milestone, 1747,
iast (1.465 ms) : 1443, 1488
. : milestone, 1465,
profiling (1.464 ms) : 1439, 1488
. : milestone, 1464,
tracing (1.456 ms) : 1432, 1481
. : milestone, 1456,
section candidate
no_agent (1.353 ms) : 1334, 1372
. : milestone, 1353,
appsec (1.711 ms) : 1687, 1735
. : milestone, 1711,
appsec_no_iast (1.718 ms) : 1694, 1743
. : milestone, 1718,
iast (1.494 ms) : 1471, 1517
. : milestone, 1494,
profiling (1.485 ms) : 1461, 1509
. : milestone, 1485,
tracing (1.474 ms) : 1450, 1498
. : milestone, 1474,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.40.0-SNAPSHOT~a0a1527b4e, baseline=1.40.0-SNAPSHOT~1392830fdb
dateFormat X
axisFormat %s
section baseline
no_agent (372.825 µs) : 353, 393
. : milestone, 373,
iast (486.289 µs) : 465, 507
. : milestone, 486,
iast_FULL (556.083 µs) : 535, 577
. : milestone, 556,
iast_GLOBAL (511.494 µs) : 489, 534
. : milestone, 511,
iast_HARDCODED_SECRET_DISABLED (483.028 µs) : 462, 504
. : milestone, 483,
iast_INACTIVE (450.744 µs) : 429, 472
. : milestone, 451,
iast_TELEMETRY_OFF (474.422 µs) : 452, 497
. : milestone, 474,
tracing (446.845 µs) : 427, 467
. : milestone, 447,
section candidate
no_agent (370.368 µs) : 350, 391
. : milestone, 370,
iast (485.315 µs) : 464, 507
. : milestone, 485,
iast_FULL (555.408 µs) : 534, 577
. : milestone, 555,
iast_GLOBAL (510.033 µs) : 488, 532
. : milestone, 510,
iast_HARDCODED_SECRET_DISABLED (489.427 µs) : 468, 511
. : milestone, 489,
iast_INACTIVE (450.492 µs) : 430, 471
. : milestone, 450,
iast_TELEMETRY_OFF (473.551 µs) : 451, 496
. : milestone, 474,
tracing (453.298 µs) : 433, 474
. : milestone, 453,
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 tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.40.0-SNAPSHOT~a0a1527b4e, baseline=1.40.0-SNAPSHOT~1392830fdb
dateFormat X
axisFormat %s
section baseline
no_agent (1.463 ms) : 1452, 1475
. : milestone, 1463,
appsec (2.314 ms) : 2273, 2355
. : milestone, 2314,
iast (2.057 ms) : 2006, 2107
. : milestone, 2057,
iast_GLOBAL (2.107 ms) : 2056, 2158
. : milestone, 2107,
profiling (1.93 ms) : 1890, 1971
. : milestone, 1930,
tracing (1.911 ms) : 1873, 1950
. : milestone, 1911,
section candidate
no_agent (1.47 ms) : 1458, 1481
. : milestone, 1470,
appsec (2.323 ms) : 2282, 2364
. : milestone, 2323,
iast (2.079 ms) : 2027, 2131
. : milestone, 2079,
iast_GLOBAL (2.082 ms) : 2033, 2132
. : milestone, 2082,
profiling (1.93 ms) : 1889, 1971
. : milestone, 1930,
tracing (1.916 ms) : 1877, 1955
. : milestone, 1916,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.40.0-SNAPSHOT~a0a1527b4e, baseline=1.40.0-SNAPSHOT~1392830fdb
dateFormat X
axisFormat %s
section baseline
no_agent (15.778 s) : 15778000, 15778000
. : milestone, 15778000,
appsec (15.131 s) : 15131000, 15131000
. : milestone, 15131000,
iast (19.219 s) : 19219000, 19219000
. : milestone, 19219000,
iast_GLOBAL (18.091 s) : 18091000, 18091000
. : milestone, 18091000,
profiling (15.349 s) : 15349000, 15349000
. : milestone, 15349000,
tracing (15.263 s) : 15263000, 15263000
. : milestone, 15263000,
section candidate
no_agent (15.178 s) : 15178000, 15178000
. : milestone, 15178000,
appsec (15.204 s) : 15204000, 15204000
. : milestone, 15204000,
iast (18.929 s) : 18929000, 18929000
. : milestone, 18929000,
iast_GLOBAL (17.792 s) : 17792000, 17792000
. : milestone, 17792000,
profiling (15.5 s) : 15500000, 15500000
. : milestone, 15500000,
tracing (15.098 s) : 15098000, 15098000
. : milestone, 15098000,
|
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.
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()).
| @@ -1295,7 +1295,9 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins | |||
|
|
|||
| httpClientTagQueryString = | |||
| configProvider.getBoolean( | |||
| HTTP_CLIENT_TAG_QUERY_STRING, DEFAULT_HTTP_CLIENT_TAG_QUERY_STRING); | |||
| "trace." + HTTP_CLIENT_TAG_QUERY_STRING, | |||
There was a problem hiding this comment.
Can you create a dedicated constant near to the legacy one, and deprecate the older one using@Deprecated and some Javadoc comment?
There was a problem hiding this comment.
This was a conversation I had with @mcculls, who proposed that we go through with this solution as it is already used in other config scenarios. Do you think that we should change this implementation to have a dedicated constant that does this?
public static final String TRACE_HTTP_SERVER_TAG_QUERY_STRING = "trace.http.server.tag.query-string";
There was a problem hiding this comment.
Do you think that we should change this implementation to have a dedicated constant that does this?
Exactly. The main rationale is that we usually do a text search for a configuration key to find out how it is used in the code base. When using "trace." + HTTP_CLIENT_TAG_QUERY_STRING, we won't be able to find it 😞
mtoffl01
reviewed
Sep 27, 2024
| @@ -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.
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.
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 😉
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What Does This Do
Changes the default value of the
HTTP_CLIENT_TAG_QUERY_STRINGto have the value oftrueinstead offalseto be consistent with other language defaults. Additionally, we add support to handlethe
TRACE_HTTP_CLIENT_TAG_QUERY_STRINGtag as well as the originalHTTP_CLIENT_TAG_QUERY_STRINGin 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 and state that the tag can be manually set to false
Contributor Checklist
type:and (comp:orinst:) labels in addition to any usefull labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]