Implement batch submission for traces #469

nikita-tkachenko-datadog · 2024-11-07T13:16:51Z

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?

Adds batch submission of traces for Agentless and Agentfull modes

Traces are submitted in batches instead of each event being submitted in a separate HTTP request.
For now batching is disabled by default, and can be enabled with the DD_JENKINS_ENABLE_TRACES_BATCHING environment variable set to true.

Extends diagnostic flare with health stats for async submitters

Various metrics are added, such as:

number of submitted logs and traces
number of dropped logs and traces
logs and traces batch size histogram
timing of logs and traces submit and dispatch

Increases the default batch size for logs and traces

Batch size is increased from 100 to 500, as stress testing showed that larger batch sizes increase throughput.

Updates the default behaviour of the async submission of logs and traces to not block

When a log or a trace element is submitted, it is added to a queue that is processed asynchronously by a separate thread.
The queue has a limited size, so it is possible that it's full when a submission takes place.

The old logic blocked the submitting thread in such cases (5s for logs, 30s for traces).
Blocking the thread has a disadvantage: it slows down the execution of pipelines.

The new logic will drop submitted log/trace when the queue is full.

The queue is there to account for short bursts of logs/traces.
If it gets full than it is likely that the consumer is consistently slower than the producer, which will lead to severe performance degradation in case of blocking on submission.

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.

… when lagging behind

drodriguezhdez

Dropped some comments

pom.xml

drodriguezhdez · 2024-11-18T14:39:44Z

src/main/java/org/datadog/jenkins/plugins/datadog/clients/CompressedBatchSender.java

+import java.util.logging.Logger;
+import java.util.zip.GZIPOutputStream;
+
+public class CompressedBatchSender<T> implements JsonPayloadSender<T> {


Would it be possible to we have unit tests for this class?

Makes sense, added unit tests

drodriguezhdez · 2024-11-18T14:48:46Z

src/main/java/org/datadog/jenkins/plugins/datadog/clients/DatadogApiClient.java

@@ -176,7 +174,7 @@ private static boolean postLogs(HttpClient httpClient, String logIntakeUrl, Secr

        byte[] body = payload.getBytes(StandardCharsets.UTF_8);
        try {
-            httpClient.postAsynchronously(logIntakeUrl, headers, "application/json", body);
+            httpClient.post(logIntakeUrl, headers, "application/json", body, Function.identity());


Is postLogs private static method only used in validateLogIntakeConnection method?

If so, should we add all the logic to the validateLogIntakeConnection method? It was unexpected to me read postLogs and see that's only invoked in the validation.

Good catch, I think it was used by some other logic in the past, now it's only used for validation. Inlined the method.

src/main/java/org/datadog/jenkins/plugins/datadog/clients/CompressedBatchSender.java

drodriguezhdez · 2024-11-18T14:53:02Z

src/main/java/org/datadog/jenkins/plugins/datadog/flare/WritersHealthFlare.java

+
+    @Override
+    public int order() {
+        return 0;


It would be great to have in a static variable the order of all FlareContributor. Currently, I don't know if the number is used in other places without checking all the impls for FlareContributor.

It's not that much of a problem if the order is the same, this only affects sorting in UI. Still a valid point, added some named constants for the orders.

drodriguezhdez · 2024-11-18T14:53:38Z

src/main/java/org/datadog/jenkins/plugins/datadog/logs/LogWriterFactory.java

-    private static final int DEFAULT_QUEUE_CAPACITY = 5_000;
-    private static final int DEFAULT_SUBMIT_TIMEOUT_SECONDS = 5;
+    private static final int DEFAULT_QUEUE_CAPACITY = 10_000;
+    private static final int DEFAULT_SUBMIT_TIMEOUT_SECONDS = 0;


What's the impact of no timeout, infinite timeout??

The opposite actually, with 0 timeout any trace/log that does not fit into the queue will be dropped immediately.
The idea is to avoid blocking the client's code and the Jenkins core. If the queue fills up, it means traces/logs are being produced faster than they're being dispatched, and a timeout will not resolve this problem, just postpone it.

drodriguezhdez · 2024-11-18T15:01:25Z

src/main/java/org/datadog/jenkins/plugins/datadog/util/AsyncWriter.java

+    private final Timer submit;
+    private final Meter submitDropped;
+    private final Timer dispatch;
+    private final Gauge<Integer> queueSize;
+    private final Histogram batchSize;


This is for internal usage right? We're not reporting these metrics to Datadog.

Correct, this only shows up in diagnostic flares for now. It'd be nice to integrate this with actual telemetry, but this is too much of an effort for now.

Fix logs and traces async submission to avoid blocking jobs execution…

ee934ba

… when lagging behind

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

nikita-tkachenko-datadog added 4 commits November 13, 2024 15:24

Add a stress test pipeline

e59c24d

Extend diagnostic flare with writers health info

32c496e

Implement batch submit for traces

8e7a1da

Increase default batch limits in async writers

9d4453f

nikita-tkachenko-datadog marked this pull request as ready for review November 14, 2024 19:19

Disable traces batching by default

b613d96

nikita-tkachenko-datadog changed the title ~~Fix logs and traces async submission to avoid blocking jobs execution when lagging behind~~ Implement batch submission for traces Nov 15, 2024

nikita-tkachenko-datadog added 2 commits November 15, 2024 11:05

Disable traces batching by default

206e9db

Disable traces batching by default

6c8f98b

nikita-tkachenko-datadog requested a review from drodriguezhdez November 18, 2024 09:48

drodriguezhdez reviewed Nov 18, 2024

View reviewed changes

Address review comments

4a5c1a1

nikita-tkachenko-datadog requested a review from drodriguezhdez November 19, 2024 14:09

drodriguezhdez approved these changes Nov 20, 2024

View reviewed changes

nikita-tkachenko-datadog merged commit 4687cf3 into master Nov 20, 2024
19 checks passed

nikita-tkachenko-datadog deleted the nikita-tkachenko/fix-async-submit-blocks branch November 20, 2024 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement batch submission for traces #469

Implement batch submission for traces #469

nikita-tkachenko-datadog commented Nov 7, 2024 •

edited

Loading

drodriguezhdez left a comment

drodriguezhdez Nov 18, 2024

nikita-tkachenko-datadog Nov 19, 2024

drodriguezhdez Nov 18, 2024

nikita-tkachenko-datadog Nov 19, 2024

drodriguezhdez Nov 18, 2024

nikita-tkachenko-datadog Nov 19, 2024

drodriguezhdez Nov 18, 2024

nikita-tkachenko-datadog Nov 19, 2024

drodriguezhdez Nov 18, 2024

nikita-tkachenko-datadog Nov 19, 2024 •

edited

Loading

Implement batch submission for traces #469

Implement batch submission for traces #469

Conversation

nikita-tkachenko-datadog commented Nov 7, 2024 • edited Loading

Requirements for Contributing to this repository

What does this PR do?

Adds batch submission of traces for Agentless and Agentfull modes

Extends diagnostic flare with health stats for async submitters

Increases the default batch size for logs and traces

Updates the default behaviour of the async submission of logs and traces to not block

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

drodriguezhdez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikita-tkachenko-datadog Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

nikita-tkachenko-datadog commented Nov 7, 2024 •

edited

Loading

nikita-tkachenko-datadog Nov 19, 2024 •

edited

Loading