-
Notifications
You must be signed in to change notification settings - Fork 320
Create shared DD intake HTTP client #9660
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
Create shared DD intake HTTP client #9660
Conversation
|
🎯 Code Coverage 🔗 Commit SHA: e49843a | Docs | Was this helpful? Give us feedback! |
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 59 metrics, 6 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.54.0-SNAPSHOT~e49843a21e, baseline=1.54.0-SNAPSHOT~55230eb888
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.019 s) : 0, 1019038
Total [baseline] (10.74 s) : 0, 10740403
Agent [candidate] (1.015 s) : 0, 1015396
Total [candidate] (10.631 s) : 0, 10631366
section appsec
Agent [baseline] (1.203 s) : 0, 1202879
Total [baseline] (10.983 s) : 0, 10982619
Agent [candidate] (1.191 s) : 0, 1190870
Total [candidate] (11.057 s) : 0, 11057038
section iast
Agent [baseline] (1.161 s) : 0, 1161417
Total [baseline] (11.021 s) : 0, 11021100
Agent [candidate] (1.151 s) : 0, 1150850
Total [candidate] (10.941 s) : 0, 10940589
section profiling
Agent [baseline] (1.172 s) : 0, 1171649
Total [baseline] (11.09 s) : 0, 11090352
Agent [candidate] (1.162 s) : 0, 1161771
Total [candidate] (11.042 s) : 0, 11041596
gantt
title petclinic - break down per module: candidate=1.54.0-SNAPSHOT~e49843a21e, baseline=1.54.0-SNAPSHOT~55230eb888
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.464 ms) : 0, 1464
crashtracking [candidate] (1.449 ms) : 0, 1449
BytebuddyAgent [baseline] (695.314 ms) : 0, 695314
BytebuddyAgent [candidate] (691.173 ms) : 0, 691173
GlobalTracer [baseline] (242.171 ms) : 0, 242171
GlobalTracer [candidate] (242.59 ms) : 0, 242590
AppSec [baseline] (32.492 ms) : 0, 32492
AppSec [candidate] (32.648 ms) : 0, 32648
Debugger [baseline] (6.452 ms) : 0, 6452
Debugger [candidate] (6.34 ms) : 0, 6340
Remote Config [baseline] (695.787 µs) : 0, 696
Remote Config [candidate] (671.86 µs) : 0, 672
Telemetry [baseline] (9.144 ms) : 0, 9144
Telemetry [candidate] (9.009 ms) : 0, 9009
Flare Poller [baseline] (10.203 ms) : 0, 10203
Flare Poller [candidate] (10.218 ms) : 0, 10218
section appsec
crashtracking [baseline] (1.487 ms) : 0, 1487
crashtracking [candidate] (1.456 ms) : 0, 1456
BytebuddyAgent [baseline] (723.326 ms) : 0, 723326
BytebuddyAgent [candidate] (714.405 ms) : 0, 714405
GlobalTracer [baseline] (235.825 ms) : 0, 235825
GlobalTracer [candidate] (235.062 ms) : 0, 235062
IAST [baseline] (25.049 ms) : 0, 25049
IAST [candidate] (24.786 ms) : 0, 24786
AppSec [baseline] (175.695 ms) : 0, 175695
AppSec [candidate] (173.395 ms) : 0, 173395
Debugger [baseline] (6.227 ms) : 0, 6227
Debugger [candidate] (6.041 ms) : 0, 6041
Remote Config [baseline] (644.159 µs) : 0, 644
Remote Config [candidate] (648.598 µs) : 0, 649
Telemetry [baseline] (9.37 ms) : 0, 9370
Telemetry [candidate] (9.946 ms) : 0, 9946
Flare Poller [baseline] (4.028 ms) : 0, 4028
Flare Poller [candidate] (3.97 ms) : 0, 3970
section iast
crashtracking [baseline] (1.472 ms) : 0, 1472
crashtracking [candidate] (1.453 ms) : 0, 1453
BytebuddyAgent [baseline] (822.286 ms) : 0, 822286
BytebuddyAgent [candidate] (813.394 ms) : 0, 813394
GlobalTracer [baseline] (233.897 ms) : 0, 233897
GlobalTracer [candidate] (233.734 ms) : 0, 233734
IAST [baseline] (27.215 ms) : 0, 27215
IAST [candidate] (26.473 ms) : 0, 26473
AppSec [baseline] (35.606 ms) : 0, 35606
AppSec [candidate] (35.084 ms) : 0, 35084
Debugger [baseline] (6.133 ms) : 0, 6133
Debugger [candidate] (6.109 ms) : 0, 6109
Remote Config [baseline] (617.97 µs) : 0, 618
Remote Config [candidate] (599.802 µs) : 0, 600
Telemetry [baseline] (8.664 ms) : 0, 8664
Telemetry [candidate] (8.292 ms) : 0, 8292
Flare Poller [baseline] (4.169 ms) : 0, 4169
Flare Poller [candidate] (4.295 ms) : 0, 4295
section profiling
crashtracking [baseline] (1.444 ms) : 0, 1444
crashtracking [candidate] (1.429 ms) : 0, 1429
BytebuddyAgent [baseline] (727.739 ms) : 0, 727739
BytebuddyAgent [candidate] (720.721 ms) : 0, 720721
GlobalTracer [baseline] (219.343 ms) : 0, 219343
GlobalTracer [candidate] (218.899 ms) : 0, 218899
AppSec [baseline] (33.428 ms) : 0, 33428
AppSec [candidate] (32.618 ms) : 0, 32618
Debugger [baseline] (8.938 ms) : 0, 8938
Debugger [candidate] (8.153 ms) : 0, 8153
Remote Config [baseline] (720.25 µs) : 0, 720
Remote Config [candidate] (821.292 µs) : 0, 821
Telemetry [baseline] (14.455 ms) : 0, 14455
Telemetry [candidate] (14.887 ms) : 0, 14887
Flare Poller [baseline] (4.186 ms) : 0, 4186
Flare Poller [candidate] (4.141 ms) : 0, 4141
ProfilingAgent [baseline] (108.193 ms) : 0, 108193
ProfilingAgent [candidate] (105.192 ms) : 0, 105192
Profiling [baseline] (108.947 ms) : 0, 108947
Profiling [candidate] (108.068 ms) : 0, 108068
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.54.0-SNAPSHOT~e49843a21e, baseline=1.54.0-SNAPSHOT~55230eb888
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.027 s) : 0, 1026596
Total [baseline] (8.734 s) : 0, 8733755
Agent [candidate] (1.018 s) : 0, 1018014
Total [candidate] (8.631 s) : 0, 8630782
section iast
Agent [baseline] (1.168 s) : 0, 1167554
Total [baseline] (9.264 s) : 0, 9263724
Agent [candidate] (1.155 s) : 0, 1154756
Total [candidate] (9.274 s) : 0, 9273932
gantt
title insecure-bank - break down per module: candidate=1.54.0-SNAPSHOT~e49843a21e, baseline=1.54.0-SNAPSHOT~55230eb888
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.489 ms) : 0, 1489
crashtracking [candidate] (1.46 ms) : 0, 1460
BytebuddyAgent [baseline] (700.113 ms) : 0, 700113
BytebuddyAgent [candidate] (692.818 ms) : 0, 692818
GlobalTracer [baseline] (242.88 ms) : 0, 242880
GlobalTracer [candidate] (243.002 ms) : 0, 243002
AppSec [baseline] (32.847 ms) : 0, 32847
AppSec [candidate] (32.607 ms) : 0, 32607
Debugger [baseline] (6.48 ms) : 0, 6480
Debugger [candidate] (6.403 ms) : 0, 6403
Remote Config [baseline] (706.406 µs) : 0, 706
Remote Config [candidate] (690.749 µs) : 0, 691
Telemetry [baseline] (9.219 ms) : 0, 9219
Telemetry [candidate] (9.076 ms) : 0, 9076
Flare Poller [baseline] (11.616 ms) : 0, 11616
Flare Poller [candidate] (10.776 ms) : 0, 10776
section iast
crashtracking [baseline] (1.488 ms) : 0, 1488
crashtracking [candidate] (1.459 ms) : 0, 1459
BytebuddyAgent [baseline] (827.021 ms) : 0, 827021
BytebuddyAgent [candidate] (816.486 ms) : 0, 816486
GlobalTracer [baseline] (235.098 ms) : 0, 235098
GlobalTracer [candidate] (234.608 ms) : 0, 234608
IAST [baseline] (27.527 ms) : 0, 27527
IAST [candidate] (26.655 ms) : 0, 26655
AppSec [baseline] (35.59 ms) : 0, 35590
AppSec [candidate] (34.979 ms) : 0, 34979
Debugger [baseline] (6.074 ms) : 0, 6074
Debugger [candidate] (6.02 ms) : 0, 6020
Remote Config [baseline] (603.66 µs) : 0, 604
Remote Config [candidate] (596.031 µs) : 0, 596
Telemetry [baseline] (8.504 ms) : 0, 8504
Telemetry [candidate] (8.228 ms) : 0, 8228
Flare Poller [baseline] (4.215 ms) : 0, 4215
Flare Poller [candidate] (4.317 ms) : 0, 4317
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 1 performance regressions! Performance is the same for 9 metrics, 12 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~e49843a21e, baseline=1.54.0-SNAPSHOT~55230eb888
dateFormat X
axisFormat %s
section baseline
no_agent (4.434 ms) : 4379, 4490
. : milestone, 4434,
iast (9.529 ms) : 9372, 9685
. : milestone, 9529,
iast_FULL (14.201 ms) : 13917, 14485
. : milestone, 14201,
iast_GLOBAL (10.41 ms) : 10223, 10598
. : milestone, 10410,
profiling (8.986 ms) : 8846, 9126
. : milestone, 8986,
tracing (7.71 ms) : 7590, 7830
. : milestone, 7710,
section candidate
no_agent (4.445 ms) : 4396, 4494
. : milestone, 4445,
iast (9.657 ms) : 9497, 9818
. : milestone, 9657,
iast_FULL (14.095 ms) : 13809, 14381
. : milestone, 14095,
iast_GLOBAL (10.474 ms) : 10287, 10661
. : milestone, 10474,
profiling (8.918 ms) : 8775, 9061
. : milestone, 8918,
tracing (7.769 ms) : 7651, 7887
. : milestone, 7769,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~e49843a21e, baseline=1.54.0-SNAPSHOT~55230eb888
dateFormat X
axisFormat %s
section baseline
no_agent (37.466 ms) : 37161, 37771
. : milestone, 37466,
appsec (50.699 ms) : 50233, 51166
. : milestone, 50699,
code_origins (43.357 ms) : 42992, 43723
. : milestone, 43357,
iast (47.153 ms) : 46744, 47561
. : milestone, 47153,
profiling (49.946 ms) : 49436, 50455
. : milestone, 49946,
tracing (45.288 ms) : 44885, 45691
. : milestone, 45288,
section candidate
no_agent (36.422 ms) : 36130, 36713
. : milestone, 36422,
appsec (46.411 ms) : 46005, 46817
. : milestone, 46411,
code_origins (45.55 ms) : 45155, 45946
. : milestone, 45550,
iast (46.248 ms) : 45853, 46643
. : milestone, 46248,
profiling (50.562 ms) : 50079, 51045
. : milestone, 50562,
tracing (43.338 ms) : 42971, 43705
. : milestone, 43338,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~e49843a21e, baseline=1.54.0-SNAPSHOT~55230eb888
dateFormat X
axisFormat %s
section baseline
no_agent (14.568 s) : 14568000, 14568000
. : milestone, 14568000,
appsec (14.411 s) : 14411000, 14411000
. : milestone, 14411000,
iast (17.814 s) : 17814000, 17814000
. : milestone, 17814000,
iast_GLOBAL (17.336 s) : 17336000, 17336000
. : milestone, 17336000,
profiling (14.44 s) : 14440000, 14440000
. : milestone, 14440000,
tracing (14.507 s) : 14507000, 14507000
. : milestone, 14507000,
section candidate
no_agent (14.792 s) : 14792000, 14792000
. : milestone, 14792000,
appsec (14.926 s) : 14926000, 14926000
. : milestone, 14926000,
iast (17.776 s) : 17776000, 17776000
. : milestone, 17776000,
iast_GLOBAL (17.291 s) : 17291000, 17291000
. : milestone, 17291000,
profiling (15.127 s) : 15127000, 15127000
. : milestone, 15127000,
tracing (14.725 s) : 14725000, 14725000
. : milestone, 14725000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~e49843a21e, baseline=1.54.0-SNAPSHOT~55230eb888
dateFormat X
axisFormat %s
section baseline
no_agent (1.487 ms) : 1476, 1499
. : milestone, 1487,
appsec (4.21 ms) : 3956, 4463
. : milestone, 4210,
iast (2.567 ms) : 2468, 2666
. : milestone, 2567,
iast_GLOBAL (2.618 ms) : 2518, 2718
. : milestone, 2618,
profiling (2.412 ms) : 2327, 2498
. : milestone, 2412,
tracing (2.338 ms) : 2258, 2419
. : milestone, 2338,
section candidate
no_agent (1.486 ms) : 1474, 1497
. : milestone, 1486,
appsec (4.101 ms) : 3856, 4347
. : milestone, 4101,
iast (2.557 ms) : 2458, 2656
. : milestone, 2557,
iast_GLOBAL (2.605 ms) : 2505, 2706
. : milestone, 2605,
profiling (2.406 ms) : 2320, 2491
. : milestone, 2406,
tracing (2.35 ms) : 2269, 2430
. : milestone, 2350,
|
AlexeyKuznetsov-DD
left a comment
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.
LGTM, just couple of minor notes.
Just curious, should we cleanup some build.gradle as clients now in SharedObjects?
| httpClientTimeout = | ||
| !config.isCiVisibilityEnabled() | ||
| ? TimeUnit.SECONDS.toMillis(config.getAgentTimeout()) | ||
| : config.getCiVisibilityBackendApiTimeoutMillis(); |
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 would reverse boolean check:
| httpClientTimeout = | |
| !config.isCiVisibilityEnabled() | |
| ? TimeUnit.SECONDS.toMillis(config.getAgentTimeout()) | |
| : config.getCiVisibilityBackendApiTimeoutMillis(); | |
| httpClientTimeout = | |
| config.isCiVisibilityEnabled() | |
| ? config.getCiVisibilityBackendApiTimeoutMillis() | |
| : TimeUnit.SECONDS.toMillis(config.getAgentTimeout()); |
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 usually put the common case first, and I think these things are a matter of personal preference, but I have nothing against reversing the check
| agentHttpClient = | ||
| OkHttpUtils.buildHttpClient( | ||
| agentUrl, unixDomainSocket, namedPipe, getHttpClientTimeout(config)); | ||
| agentUrl != null && "http".equals(agentUrl.scheme()), |
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.
would it make sense to use equalsIgnoreCase ?
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.
The scheme() method always returns the scheme in lowercase, and this code was just moved here from a different place. I can switch to equalsIgnoreCase nonetheless
communication/src/main/java/datadog/communication/ddagent/SharedCommunicationObjects.java
Show resolved
Hide resolved
|
|
||
| public static OkHttpClient buildHttpClient(final HttpUrl url, final long timeoutMillis) { | ||
| return buildHttpClient(url, null, null, timeoutMillis); | ||
| return buildHttpClient(url != null && "http".equals(url.scheme()), null, null, timeoutMillis); |
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.
This check url != null && "http".equals(url.scheme()) looks like a pattern, maybe can be extracted to helper function?
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.
Sure, added a helper function
| private final int agentTimeout; | ||
| /** Should be set to {@code true} when running in agentless mode in a JVM without TLS */ | ||
| private final boolean forceClearTextHttpForIntakeClient; | ||
|
|
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.
🎯 suggestion: Should it be added to the Config.toString() method?
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 think this flag will be used very very rarely, and I'd rather not make Config.toString() bigger than it already is. If this setting is misconfigured it will be pretty obvious, besides we have config telemetry
I couldn't find anything that can be removed, the users of the clients still have to depend on the |
What Does This Do
Adds a single shared lazily-initialized HTTP client to send requests to Datadog backend.
Motivation
Previously each component talking to the backend created its own HTTP client, so we ended up with 4-5 clients in agentless mode (tests intake, telemetry, API, logs intake, coverage report intake).
OKHTTP docs advice creating a single HTTP client per JVM (the client creates its own thread and connection pools, so having multiple clients adds some load).
Additional Notes
By default the new single client does NOT force clear-text HTTP.
Prior to the changes, each component's URL was examined and if the schema was
http, clear text connections were forced for corresponding client.This was needed to support running in JVMs that had no modern TLS implementations.
This check is > 5 years old, nowadays it should be rare to encounter such a JVM.
Moreover, this check was introduced when the only communication the tracer did was to the agent, often running locally, which is why unencrypted HTTP was acceptable.
When talking to the backend we should not use unencrypted HTTP anyway.
In case there happens to be a customer using agentless mode and running a JVM that does not support TLS, they can set the
DD_FORCE_CLEAR_TEXT_HTTP_FOR_INTAKE_CLIENTenvironment variable to force clear-text HTTP on the new client.Communication to the agent is unaffected, as there's already a single dedicated client for it (which, depending on the config, can use UDS or named pipes, which we don't want for the intake client).
Contributor Checklist
type:and (comp:orinst:) labels in addition to any useful labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: SDTEST-2562