Refactor logs submission by nikita-tkachenko-datadog · Pull Request #451 · jenkinsci/datadog-plugin

nikita-tkachenko-datadog · 2024-09-19T15:03:42Z

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?

Refactors logic that is used for submitting logs to Datadog.
The following improvements are done:

Logs are submitted asynchronously now to avoid blocking execution of the core Jenkins logic
Circuit breakers are used to avoid spamming Jenkins log with errors if log submission is configured incorrectly
When submitting directly to Datadog intake logs are batched and compressed
When submitting to Datadog agent, incorrect "retry" logic is no longer used

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

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.

jhaumont

Thx ! This will be very helpful to improve the log on Jenkins controller side and resolve #344

drodriguezhdez

Dropped some comments

drodriguezhdez · 2024-10-09T14:48:54Z

+                    out.flush();
+                    out.close();


Can happen that out.flush() throws an exception and the OutputStream is never closed?

Seems possible, added a separate try/catch for this

drodriguezhdez · 2024-10-09T14:53:28Z

+
+            compressedRequest.write(END_JSON_ARRAY);
+            compressedRequest.close();
+            httpClient.post(logIntakeUrl, headers, "application/json", request.toByteArray(), Function.identity());


Suggested change

httpClient.post(logIntakeUrl, headers, "application/json", request.toByteArray(), Function.identity());

httpClient.post(logIntakeUrl, headers, "application/json", compressedRequest.toByteArray(), Function.identity());

Isn't you need to send the compressedRequest?

request is a ByteArrayOutputStream that contains the actual compressed data.
compressedRequest is a GZIPOutputStream that is a wrapper around the request, responsible for doing the compression.

I think it's the naming that creates the confusion here.
I renamed compressedRequest to gzip, but let me know if you can come up with a better name.

drodriguezhdez · 2024-10-09T14:55:45Z

+                if (uncompressedRequestLength + body.length + 2 > PAYLOAD_SIZE_LIMIT) { // + 2 is for comma and array end: ,<payload>]
+                    compressedRequest.write(END_JSON_ARRAY);
+                    compressedRequest.close();
+                    httpClient.post(logIntakeUrl, headers, "application/json", request.toByteArray(), Function.identity());


Suggested change

httpClient.post(logIntakeUrl, headers, "application/json", request.toByteArray(), Function.identity());

httpClient.post(logIntakeUrl, headers, "application/json", compressedRequest.toByteArray(), Function.identity());

Isn't you need to send the compressedRequest?

Same as above

drodriguezhdez · 2024-10-09T14:56:55Z

+                    continue;
+                }
+
+                if (uncompressedRequestLength + body.length + 2 > PAYLOAD_SIZE_LIMIT) { // + 2 is for comma and array end: ,<payload>]


If we're sending the request compressed, why we're checking the uncompressedReq length?

Because the backend limit is applied to the uncompressed request body, the API docs are explicit about that

Let's add a comment for this

drodriguezhdez · 2024-10-09T14:57:40Z

+        }
+
+        private void fallback(List<String> payloads) {
+            // cannot establish connection to agent, do nothing


Suggested change

// cannot establish connection to agent, do nothing

// cannot establish connection to API, do nothing

Maybe? Also, not sure if we should print some errors in this case.

Thanks, the comment was a copy-paste that I overlooked.
The first error will logged in org.datadog.jenkins.plugins.datadog.clients.DatadogApiClient.ApiLogWriteStrategy#handleError above, which is invoked when the circuit breaker goes from active to inactive state.
In the fallback method we shouldn't be logging it, otherwise we'll spam the logs

drodriguezhdez · 2024-10-09T15:20:54Z

+                    continue;
+                }
+
+                if (uncompressedRequestLength + body.length + 2 > PAYLOAD_SIZE_LIMIT) { // + 2 is for comma and array end: ,<payload>]


Let's add a comment for this

Refactor logs submission

5577bdf

nikita-tkachenko-datadog added the changelog/Fixed Fixed features results into a bug fix version bump label Sep 19, 2024

nikita-tkachenko-datadog added 4 commits September 19, 2024 17:25

Fix compilation issue

d4427bb

Remove retry logs checkbox from plugin config UI

5d36dd0

Fix off-by-one error

aaa14ef

Do not activate logs writer if logs are not enabled

0e9e310

nikita-tkachenko-datadog force-pushed the nikita-tkachenko/logs-refactoring branch from 2e62ecf to 0e9e310 Compare September 20, 2024 15:36

nikita-tkachenko-datadog marked this pull request as ready for review September 23, 2024 14:00

nikita-tkachenko-datadog requested a review from sarah-witt September 23, 2024 14:19

jhaumont approved these changes Oct 2, 2024

View reviewed changes

drodriguezhdez reviewed Oct 9, 2024

View reviewed changes

Fix review comments

c059655

nikita-tkachenko-datadog requested a review from drodriguezhdez October 9, 2024 15:18

drodriguezhdez previously approved these changes Oct 9, 2024

View reviewed changes

Add a comment

2df7ea9

nikita-tkachenko-datadog dismissed drodriguezhdez’s stale review via 2df7ea9 October 9, 2024 15:25

drodriguezhdez previously approved these changes Oct 9, 2024

View reviewed changes

Merge branch 'master' into nikita-tkachenko/logs-refactoring

7a684de

nikita-tkachenko-datadog dismissed drodriguezhdez’s stale review via 7a684de October 9, 2024 15:33

drodriguezhdez approved these changes Oct 10, 2024

View reviewed changes

nikita-tkachenko-datadog merged commit 815812a into master Oct 10, 2024

nikita-tkachenko-datadog deleted the nikita-tkachenko/logs-refactoring branch October 10, 2024 08:26

dduportal mentioned this pull request Nov 14, 2024

ci.jenkins.io: upgrade datadog plugin from 7.x to 8.x jenkins-infra/helpdesk#4377

Closed

	httpClient.post(logIntakeUrl, headers, "application/json", request.toByteArray(), Function.identity());
	httpClient.post(logIntakeUrl, headers, "application/json", compressedRequest.toByteArray(), Function.identity());

	// cannot establish connection to agent, do nothing
	// cannot establish connection to API, do nothing

Uh oh!

Conversation

nikita-tkachenko-datadog commented Sep 19, 2024 • 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!

jhaumont left a comment

Choose a reason for hiding this comment

Uh oh!

drodriguezhdez left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nikita-tkachenko-datadog commented Sep 19, 2024 •

edited

Loading