Move all HTTP calls to a dedicated class and use Jetty HTTP client instead of raw HttpUrlConnection by nikita-tkachenko-datadog · Pull Request #346 · jenkinsci/datadog-plugin

nikita-tkachenko-datadog · 2023-06-09T10:04:13Z

Requirements for Contributing to this repository

Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
The pull request must only fix one issue at the time.
The pull request must update the test suite to demonstrate the changed functionality.
After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see CONTRIBUTING.

What does this PR do?

This PR does a few improvements to the way the plugin does HTTP calls, making them more efficient and limiting the number of concurrent requests.

The reason to do this is that there is a customer with a Jenkins instance that runs multiple large pipelines: when executed with the plugin enabled, the process runs out of available file descriptors because the requests are slow (since each one opens a new TCP socket) and because there is no limit on the number of simultaneous requests and no backpressure if the network layer cannot keep up.

Description of the Change

all the HTTP calls are now delegated to org.datadog.jenkins.plugins.datadog.clients.HttpClient which provides a layer of abstraction on top of HTTP-calling logic
Jetty HTTP client is now used instead of raw HttpUrlConnection, which allows to pool and reuse TCP sockets, limit the number of simultaneous requests and to make some of the request asynchronously
Retry logic is added to re-execute calls that are failing

Alternate Designs

Possible Drawbacks

Verification Process

Verified manually in Agentless/Evp-proxy/Agentful-APM modes.
Separate set of verifications is done with Jenkins using an HTTP proxy (to ensure the plugin respect Jenkins' proxy settings).

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
PR title must be written as a CHANGELOG entry (see why)
Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
PR should not have do-not-merge/ label attached.
If Applicable, issue must have kind/ and severity/ labels attached at least.

…stead of raw HttpUrlConnection

drodriguezhdez

I think this PR needs to be reviewed by the jenkins-plugin team as well, as AFAIK we wanted to use vanilla Java HTTP classes to avoid affecting Jenkins core with external dependencies.

drodriguezhdez · 2023-06-09T10:50:43Z

+    <dependency>
+      <groupId>org.eclipse.jetty</groupId>
+      <artifactId>jetty-client</artifactId>
+      <version>9.4.51.v20230217</version>
+    </dependency>


Do we know if Jenkins uses jetty-client? If so, this might affect Jenkins core and we should use something under org.jenkins-ci.* groupId.

Jenkins does not use Jetty client, and neither do any of the plugins that we're depending on, so we're good to go here.

drodriguezhdez · 2023-06-09T10:54:55Z

+        this.retryPolicyFactory = new HttpRetryPolicy.Factory(5, 100, 2.0);
+    }
+
+    public <T> T get(String url, Map<String, String> headers, Function<String, T> responseParser) throws ExecutionException, InterruptedException, TimeoutException {


Are we using get and post anywhere? Do we need them?

As far I could check, only asynchronous methods are used.

we use the synchronous versions whenever we need to do something with the response: get is used to fetch the list of agent endpoints and post is used for connection validation (part of the validation is to check that the response is valid JSON)

drodriguezhdez · 2023-06-09T11:00:10Z

+
+  public static final class Factory {
+    private final int maxRetries;
+    private final long initialDelay;


Maybe just delay? It's confusing to find initial as it seems it's only going to be used once, but this is the actual delay for every backoff delay.

The field in the policy class is mutable and is called delay, but the one in the factory class is really just the initial value

drodriguezhdez · 2023-06-09T11:01:35Z

+
+  public long backoff() {
+    long currentDelay = delay;
+    delay = (long) (delay * delayFactor * ThreadLocalRandom.current().nextDouble(0.85, 1.15));


Could we encapsulate the jitter into something like nextJitter() or randomJitter() or something similar?

done, moved to randomJitter()

nikita-tkachenko-datadog · 2023-06-09T13:10:33Z

I think this PR needs to be reviewed by the jenkins-plugin team as well, as AFAIK we wanted to use vanilla Java HTTP classes to avoid affecting Jenkins core with external dependencies.

Jenkins plugins are loaded using isolated classloaders, so including a third-party dependency in a plugin has no effect on Jenkins core.
Plugin classloaders are children of the classloader that is used to load the core, so the worst that can happen in theory is a problem in the plugin in case it tries to load a class that is already available in the core loader (because children CLs delegate to parent CL).
Anyway, this is not the case here, since neither Jenkins core nor the plugins that we depend on use Jetty HTTP.

sarah-witt · 2023-08-22T18:19:42Z

+            try {
+                Throwable failure = result.getFailure();
+                if (failure != null) {
+                    if (retryPolicy.shouldRetry(null)) {


why don't we check if we should retry with the response of the result here?

Having a failure usually means there is no response, i.e. we failed before the response could be received.

Gotcha, thanks! Is there any downside in first getting the response, and then checking shouldRetry with it? That way if the response is null, the behavior hasn't changed, but if there is a response, we can use the additional logic in shouldRetry?

Agree, that would allow to simplify the code a bit. Changed it accordingly

sarah-witt

Awesome!

Move all HTTP calls to a dedicated class and use Jetty HTTP client in…

5833745

…stead of raw HttpUrlConnection

nikita-tkachenko-datadog added the changelog/Fixed Fixed features results into a bug fix version bump label Jun 9, 2023

nikita-tkachenko-datadog requested a review from drodriguezhdez June 9, 2023 10:04

nikita-tkachenko-datadog added 2 commits June 9, 2023 12:10

Remove redundant imports

307f191

Suppress FB warning

5fc6722

drodriguezhdez reviewed Jun 9, 2023

View reviewed changes

nikita-tkachenko-datadog requested a review from sarah-witt June 9, 2023 13:12

Address review comments

7e9fecd

drodriguezhdez previously approved these changes Jun 15, 2023

View reviewed changes

sarah-witt reviewed Aug 22, 2023

View reviewed changes

Address review comments

34bd4a2

nikita-tkachenko-datadog dismissed drodriguezhdez’s stale review via 34bd4a2 August 23, 2023 12:31

nikita-tkachenko-datadog added 2 commits August 24, 2023 13:31

Reorganize request retry logic

7f9febe

Reorganize request retry logic

4da4dc7

sarah-witt approved these changes Aug 25, 2023

View reviewed changes

nikita-tkachenko-datadog merged commit 0c0fea3 into master Aug 28, 2023

nikita-tkachenko-datadog deleted the nikita-tkachenko/network-layer-update branch August 28, 2023 08:43

sarah-witt mentioned this pull request Aug 28, 2023

Update order of hostname detection logic #360

Merged

6 tasks

Uh oh!

Conversation

nikita-tkachenko-datadog commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Requirements for Contributing to this repository

What does this PR do?

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

Uh oh!

drodriguezhdez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikita-tkachenko-datadog commented Jun 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sarah-witt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nikita-tkachenko-datadog commented Jun 9, 2023 •

edited

Loading

drodriguezhdez left a comment •

edited

Loading