Skip to content

httpasyncclient4: fix url parsing and make host/port extraction happening#5543

Merged
amarziali merged 3 commits into
masterfrom
andrea.marziali/httpclient-url-fix
Aug 29, 2023
Merged

httpasyncclient4: fix url parsing and make host/port extraction happening#5543
amarziali merged 3 commits into
masterfrom
andrea.marziali/httpclient-url-fix

Conversation

@amarziali

@amarziali amarziali commented Jul 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Does This Do

Http async client 4 may not produce absolute urls in http.url tag if the client is configured with HttpHost taget and the request uri is hence relative.

This happens when the client executes the request with host and port explicitly provided with a HttpHost argument.

Impacted signatures:

  • execute(org.apache.http.HttpHost target, org.apache.http.HttpRequest request, org.apache.http.concurrent.FutureCallback<org.apache.http.HttpResponse> callback)
  • execute(org.apache.http.HttpHost target, org.apache.http.HttpRequest request, org.apache.http.protocol.HttpContext context, org.apache.http.concurrent.FutureCallback<org.apache.http.HttpResponse> callback)

The side effects are:

  • http.url tag will miss scheme, host, port
  • MIssing peer.hostname,peer.port and peer.service tags
  • Split by domain feature will not work (since the captured url is missing the host part)

This PR fixes this buggy behaviour and it's a breaking change. It also impact the following instrumentations which are relying on httpasyncclient4:

  • elasticsearch rest
  • opensearch rest

In case this change is not suitable , the old tracing flavour can be restored by adding the following feature flag:

  • Sysprop: -Ddd.httpasyncclient4.legacy.tracing.enabled=true
  • Env: DD_HTTPASYNCCLIENT4_LEGACY_TRACING_ENABLED=true

Motivation

Additional Notes

@amarziali
amarziali requested a review from a team as a code owner July 6, 2023 12:04
@amarziali amarziali added inst: others All other instrumentations tag: breaking change Breaking changes labels Jul 6, 2023
@amarziali
amarziali marked this pull request as draft July 6, 2023 12:31
@amarziali
amarziali force-pushed the andrea.marziali/httpclient-url-fix branch from bd106ba to 25b97db Compare July 6, 2023 14:26
@amarziali
amarziali marked this pull request as ready for review July 7, 2023 09:07
PerfectSlayer
PerfectSlayer reviewed Jul 17, 2023
View reviewed changes

@PerfectSlayer PerfectSlayer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it is breaking changes for httpasyncclient-4 integration but it should align its behavior with other client integrations, right?

Comment thread internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/URIUtils.java Outdated
@amarziali

amarziali commented Aug 7, 2023

Copy link
Copy Markdown
Contributor Author

I get it is breaking changes for httpasyncclient-4 integration but it should align its behavior with other client integrations, right?

yes it will made http async client 4 behave like other version of same library on the reported used case. So basically it's a bugfix

@amarziali
amarziali force-pushed the andrea.marziali/httpclient-url-fix branch from 25b97db to e8ea069 Compare August 7, 2023 13:53
@amarziali
amarziali requested a review from PerfectSlayer August 7, 2023 13:53
@amarziali
amarziali force-pushed the andrea.marziali/httpclient-url-fix branch 3 times, most recently from eac65a4 to 156b26a Compare August 9, 2023 14:15
@pr-commenter

pr-commenter Bot commented Aug 9, 2023 •
edited
Loading

Copy link
Copy Markdown

Benchmarks

Startup

Parameters

Baseline Candidate
commit 1.20.0-SNAPSHOT~db25ce9317 1.20.0-SNAPSHOT~48b7a5a8e3
config baseline candidate
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
module Agent Agent
parent None None
variant appsec appsec

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 62 cases.

Startup time reports for insecure-bank 
gantt
    title insecure-bank - global startup overhead: candidate=1.20.0-SNAPSHOT~48b7a5a8e3, baseline=1.20.0-SNAPSHOT~db25ce9317

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (986.485 ms) : 0, 986485
Total [baseline] (8.617 s) : 0, 8616886
Agent [candidate] (995.973 ms) : 0, 995973
Total [candidate] (8.712 s) : 0, 8711964
section appsec
Agent [baseline] (1.08 s) : 0, 1080296
Total [baseline] (8.734 s) : 0, 8733833
Agent [candidate] (1.072 s) : 0, 1072106
Total [candidate] (8.703 s) : 0, 8702767
section iast
Agent [baseline] (1.105 s) : 0, 1104904
Total [baseline] (9.188 s) : 0, 9188324
Agent [candidate] (1.1 s) : 0, 1100476
Total [candidate] (9.169 s) : 0, 9169014
section profiling
Agent [baseline] (1.165 s) : 0, 1164916
Total [baseline] (8.852 s) : 0, 8851965
Agent [candidate] (1.162 s) : 0, 1162181
Total [candidate] (8.865 s) : 0, 8864739
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 986.485 ms -
Agent appsec 1.08 s 93.81 ms (9.5%)
Agent iast 1.105 s 118.419 ms (12.0%)
Agent profiling 1.165 s 178.431 ms (18.1%)
Total tracing 8.617 s -
Total appsec 8.734 s 116.947 ms (1.4%)
Total iast 9.188 s 571.438 ms (6.6%)
Total profiling 8.852 s 235.079 ms (2.7%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 995.973 ms -
Agent appsec 1.072 s 76.133 ms (7.6%)
Agent iast 1.1 s 104.503 ms (10.5%)
Agent profiling 1.162 s 166.208 ms (16.7%)
Total tracing 8.712 s -
Total appsec 8.703 s -9.197 ms (-0.1%)
Total iast 9.169 s 457.049 ms (5.2%)
Total profiling 8.865 s 152.775 ms (1.8%) 
gantt
    title insecure-bank - break down per module: candidate=1.20.0-SNAPSHOT~48b7a5a8e3, baseline=1.20.0-SNAPSHOT~db25ce9317

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (611.864 ms) : 0, 611864
BytebuddyAgent [candidate] (618.213 ms) : 0, 618213
GlobalTracer [baseline] (283.424 ms) : 0, 283424
GlobalTracer [candidate] (286.134 ms) : 0, 286134
AppSec [baseline] (50.174 ms) : 0, 50174
AppSec [candidate] (50.331 ms) : 0, 50331
Remote Config [baseline] (661.687 µs) : 0, 662
Remote Config [candidate] (665.378 µs) : 0, 665
Telemetry [baseline] (5.994 ms) : 0, 5994
Telemetry [candidate] (6.022 ms) : 0, 6022
section appsec
BytebuddyAgent [baseline] (617.293 ms) : 0, 617293
BytebuddyAgent [candidate] (612.627 ms) : 0, 612627
GlobalTracer [baseline] (286.03 ms) : 0, 286030
GlobalTracer [candidate] (283.75 ms) : 0, 283750
AppSec [baseline] (135.798 ms) : 0, 135798
AppSec [candidate] (135.015 ms) : 0, 135015
Remote Config [baseline] (731.663 µs) : 0, 732
Remote Config [candidate] (731.017 µs) : 0, 731
Telemetry [baseline] (5.818 ms) : 0, 5818
Telemetry [candidate] (5.75 ms) : 0, 5750
section iast
BytebuddyAgent [baseline] (728.127 ms) : 0, 728127
BytebuddyAgent [candidate] (725.129 ms) : 0, 725129
GlobalTracer [baseline] (273.416 ms) : 0, 273416
GlobalTracer [candidate] (272.651 ms) : 0, 272651
AppSec [baseline] (47.92 ms) : 0, 47920
AppSec [candidate] (47.635 ms) : 0, 47635
IAST [baseline] (14.264 ms) : 0, 14264
IAST [candidate] (14.225 ms) : 0, 14225
Remote Config [baseline] (556.788 µs) : 0, 557
Remote Config [candidate] (552.93 µs) : 0, 553
Telemetry [baseline] (5.799 ms) : 0, 5799
Telemetry [candidate] (5.778 ms) : 0, 5778
section profiling
BytebuddyAgent [baseline] (628.303 ms) : 0, 628303
BytebuddyAgent [candidate] (626.146 ms) : 0, 626146
GlobalTracer [baseline] (349.182 ms) : 0, 349182
GlobalTracer [candidate] (349.305 ms) : 0, 349305
AppSec [baseline] (49.432 ms) : 0, 49432
AppSec [candidate] (49.283 ms) : 0, 49283
Remote Config [baseline] (648.257 µs) : 0, 648
Remote Config [candidate] (641.23 µs) : 0, 641
Telemetry [baseline] (6.026 ms) : 0, 6026
Telemetry [candidate] (6.003 ms) : 0, 6003
ProfilingAgent [baseline] (78.307 ms) : 0, 78307
ProfilingAgent [candidate] (77.839 ms) : 0, 77839
Profiling [baseline] (78.331 ms) : 0, 78331
Profiling [candidate] (77.863 ms) : 0, 77863
Loading
Startup time reports for petclinic 
gantt
    title petclinic - global startup overhead: candidate=1.20.0-SNAPSHOT~48b7a5a8e3, baseline=1.20.0-SNAPSHOT~db25ce9317

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (996.09 ms) : 0, 996090
Total [baseline] (9.218 s) : 0, 9218078
Agent [candidate] (989.365 ms) : 0, 989365
Total [candidate] (9.14 s) : 0, 9140281
section appsec
Agent [baseline] (1.079 s) : 0, 1079126
Total [baseline] (9.317 s) : 0, 9317082
Agent [candidate] (1.075 s) : 0, 1075149
Total [candidate] (9.303 s) : 0, 9302518
section iast
Agent [baseline] (1.097 s) : 0, 1096914
Total [baseline] (9.463 s) : 0, 9463330
Agent [candidate] (1.092 s) : 0, 1092257
Total [candidate] (9.42 s) : 0, 9420325
section profiling
Agent [baseline] (1.169 s) : 0, 1169210
Total [baseline] (9.474 s) : 0, 9474218
Agent [candidate] (1.163 s) : 0, 1162701
Total [candidate] (9.451 s) : 0, 9450501
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 996.09 ms -
Agent appsec 1.079 s 83.036 ms (8.3%)
Agent iast 1.097 s 100.824 ms (10.1%)
Agent profiling 1.169 s 173.119 ms (17.4%)
Total tracing 9.218 s -
Total appsec 9.317 s 99.004 ms (1.1%)
Total iast 9.463 s 245.252 ms (2.7%)
Total profiling 9.474 s 256.14 ms (2.8%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 989.365 ms -
Agent appsec 1.075 s 85.784 ms (8.7%)
Agent iast 1.092 s 102.892 ms (10.4%)
Agent profiling 1.163 s 173.336 ms (17.5%)
Total tracing 9.14 s -
Total appsec 9.303 s 162.237 ms (1.8%)
Total iast 9.42 s 280.044 ms (3.1%)
Total profiling 9.451 s 310.22 ms (3.4%) 
gantt
    title petclinic - break down per module: candidate=1.20.0-SNAPSHOT~48b7a5a8e3, baseline=1.20.0-SNAPSHOT~db25ce9317

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (617.92 ms) : 0, 617920
BytebuddyAgent [candidate] (613.308 ms) : 0, 613308
GlobalTracer [baseline] (286.558 ms) : 0, 286558
GlobalTracer [candidate] (285.031 ms) : 0, 285031
AppSec [baseline] (50.268 ms) : 0, 50268
AppSec [candidate] (50.04 ms) : 0, 50040
Remote Config [baseline] (694.062 µs) : 0, 694
Remote Config [candidate] (668.78 µs) : 0, 669
Telemetry [baseline] (6.067 ms) : 0, 6067
Telemetry [candidate] (6.003 ms) : 0, 6003
section appsec
BytebuddyAgent [baseline] (616.4 ms) : 0, 616400
BytebuddyAgent [candidate] (613.594 ms) : 0, 613594
GlobalTracer [baseline] (285.989 ms) : 0, 285989
GlobalTracer [candidate] (285.648 ms) : 0, 285648
AppSec [baseline] (135.662 ms) : 0, 135662
AppSec [candidate] (135.047 ms) : 0, 135047
Remote Config [baseline] (729.529 µs) : 0, 730
Remote Config [candidate] (733.453 µs) : 0, 733
Telemetry [baseline] (5.796 ms) : 0, 5796
Telemetry [candidate] (5.736 ms) : 0, 5736
section iast
BytebuddyAgent [baseline] (723.095 ms) : 0, 723095
BytebuddyAgent [candidate] (719.345 ms) : 0, 719345
GlobalTracer [baseline] (271.502 ms) : 0, 271502
GlobalTracer [candidate] (270.882 ms) : 0, 270882
AppSec [baseline] (47.435 ms) : 0, 47435
AppSec [candidate] (47.321 ms) : 0, 47321
IAST [baseline] (14.1 ms) : 0, 14100
IAST [candidate] (14.054 ms) : 0, 14054
Remote Config [baseline] (550.48 µs) : 0, 550
Remote Config [candidate] (556.792 µs) : 0, 557
Telemetry [baseline] (5.771 ms) : 0, 5771
Telemetry [candidate] (5.72 ms) : 0, 5720
section profiling
BytebuddyAgent [baseline] (630.146 ms) : 0, 630146
BytebuddyAgent [candidate] (626.866 ms) : 0, 626866
GlobalTracer [baseline] (349.567 ms) : 0, 349567
GlobalTracer [candidate] (349.256 ms) : 0, 349256
AppSec [baseline] (49.477 ms) : 0, 49477
AppSec [candidate] (49.258 ms) : 0, 49258
Remote Config [baseline] (662.396 µs) : 0, 662
Remote Config [candidate] (641.132 µs) : 0, 641
Telemetry [baseline] (6.081 ms) : 0, 6081
Telemetry [candidate] (5.982 ms) : 0, 5982
ProfilingAgent [baseline] (79.89 ms) : 0, 79890
ProfilingAgent [candidate] (77.596 ms) : 0, 77596
Profiling [baseline] (79.915 ms) : 0, 79915
Profiling [candidate] (77.62 ms) : 0, 77620
Loading

Load

Parameters

Baseline Candidate
commit 1.20.0-SNAPSHOT~db25ce9317 1.20.0-SNAPSHOT~48b7a5a8e3
config baseline candidate
end_time 2023-08-29T08:29:33 2023-08-29T08:47:36
start_time 2023-08-29T08:29:15 2023-08-29T08:47:19
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
variant appsec appsec

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 24 cases.

Request duration reports for insecure-bank 
gantt
    title insecure-bank - request duration [CI 0.99] : candidate=1.20.0-SNAPSHOT~48b7a5a8e3, baseline=1.20.0-SNAPSHOT~db25ce9317
    dateFormat X
    axisFormat %s
section baseline
no_agent (361.073 µs) : 340, 382
.   : milestone, 361,
appsec (655.929 µs) : 635, 677
.   : milestone, 656,
iast (447.474 µs) : 427, 468
.   : milestone, 447,
iast_FULL (513.109 µs) : 492, 534
.   : milestone, 513,
iast_INACTIVE (427.129 µs) : 406, 448
.   : milestone, 427,
profiling (431.686 µs) : 410, 453
.   : milestone, 432,
tracing (428.333 µs) : 408, 449
.   : milestone, 428,
section candidate
no_agent (366.126 µs) : 344, 388
.   : milestone, 366,
appsec (661.793 µs) : 641, 683
.   : milestone, 662,
iast (451.009 µs) : 430, 472
.   : milestone, 451,
iast_FULL (510.166 µs) : 490, 531
.   : milestone, 510,
iast_INACTIVE (426.547 µs) : 406, 447
.   : milestone, 427,
profiling (438.749 µs) : 417, 460
.   : milestone, 439,
tracing (431.396 µs) : 411, 452
.   : milestone, 431,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 361.073 µs [339.956 µs, 382.189 µs] -
appsec 655.929 µs [634.986 µs, 676.873 µs] 294.857 µs (81.7%)
iast 447.474 µs [426.713 µs, 468.234 µs] 86.401 µs (23.9%)
iast_FULL 513.109 µs [492.472 µs, 533.745 µs] 152.036 µs (42.1%)
iast_INACTIVE 427.129 µs [406.09 µs, 448.168 µs] 66.057 µs (18.3%)
profiling 431.686 µs [409.993 µs, 453.379 µs] 70.613 µs (19.6%)
tracing 428.333 µs [407.655 µs, 449.012 µs] 67.261 µs (18.6%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 366.126 µs [344.437 µs, 387.816 µs] -
appsec 661.793 µs [640.694 µs, 682.893 µs] 295.667 µs (80.8%)
iast 451.009 µs [430.145 µs, 471.873 µs] 84.882 µs (23.2%)
iast_FULL 510.166 µs [489.675 µs, 530.658 µs] 144.04 µs (39.3%)
iast_INACTIVE 426.547 µs [406.008 µs, 447.086 µs] 60.421 µs (16.5%)
profiling 438.749 µs [417.418 µs, 460.08 µs] 72.622 µs (19.8%)
tracing 431.396 µs [410.773 µs, 452.02 µs] 65.27 µs (17.8%)
Request duration reports for petclinic 
gantt
    title petclinic - request duration [CI 0.99] : candidate=1.20.0-SNAPSHOT~48b7a5a8e3, baseline=1.20.0-SNAPSHOT~db25ce9317
    dateFormat X
    axisFormat %s
section baseline
no_agent (1.329 ms) : 1310, 1348
.   : milestone, 1329,
appsec (1.683 ms) : 1659, 1708
.   : milestone, 1683,
iast (1.459 ms) : 1435, 1483
.   : milestone, 1459,
profiling (1.466 ms) : 1441, 1490
.   : milestone, 1466,
tracing (1.442 ms) : 1417, 1467
.   : milestone, 1442,
section candidate
no_agent (1.324 ms) : 1305, 1343
.   : milestone, 1324,
appsec (1.655 ms) : 1631, 1679
.   : milestone, 1655,
iast (1.455 ms) : 1431, 1479
.   : milestone, 1455,
profiling (1.464 ms) : 1440, 1489
.   : milestone, 1464,
tracing (1.442 ms) : 1418, 1466
.   : milestone, 1442,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.329 ms [1.31 ms, 1.348 ms] -
appsec 1.683 ms [1.659 ms, 1.708 ms] 354.416 µs (26.7%)
iast 1.459 ms [1.435 ms, 1.483 ms] 130.54 µs (9.8%)
profiling 1.466 ms [1.441 ms, 1.49 ms] 136.73 µs (10.3%)
tracing 1.442 ms [1.417 ms, 1.467 ms] 113.367 µs (8.5%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.324 ms [1.305 ms, 1.343 ms] -
appsec 1.655 ms [1.631 ms, 1.679 ms] 330.516 µs (25.0%)
iast 1.455 ms [1.431 ms, 1.479 ms] 130.792 µs (9.9%)
profiling 1.464 ms [1.44 ms, 1.489 ms] 140.01 µs (10.6%)
tracing 1.442 ms [1.418 ms, 1.466 ms] 117.599 µs (8.9%)

@amarziali
amarziali force-pushed the andrea.marziali/httpclient-url-fix branch from 156b26a to af90fc0 Compare August 24, 2023 09:17
@amarziali amarziali added the type: bug fix Bug fix label Aug 24, 2023
@amarziali
amarziali force-pushed the andrea.marziali/httpclient-url-fix branch from af90fc0 to e9caeb3 Compare August 24, 2023 09:39
@PerfectSlayer

PerfectSlayer commented Aug 29, 2023

Copy link
Copy Markdown
Contributor

So, what was the outcome of the discussion about the breaking behavior?

@amarziali

amarziali commented Aug 29, 2023

Copy link
Copy Markdown
Contributor Author

So, what was the outcome of the discussion about the breaking behavior?

@PerfectSlayer normally breaking change by default behind a feature flag (as done in this PR). It's ok to ship?

@amarziali
amarziali merged commit c9857f7 into master Aug 29, 2023
@amarziali
amarziali deleted the andrea.marziali/httpclient-url-fix branch August 29, 2023 12:14
@github-actions github-actions Bot added this to the 1.20.0 milestone Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PerfectSlayer PerfectSlayer PerfectSlayer approved these changes

Assignees

No one assigned

Labels

inst: others All other instrumentations tag: breaking change Breaking changes type: bug fix Bug fix

Projects

None yet

Milestone

1.20.0

Development

Successfully merging this pull request may close these issues.

2 participants

@amarziali @PerfectSlayer