Remove APM Java Tracer dependency by drodriguezhdez · Pull Request #234 · jenkinsci/datadog-plugin

drodriguezhdez · 2021-07-21T09:00:01Z

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 removes the dependency with the APM Java Tracer.

Additionally, it adds a mechanism to send the information asynchronously to the Datadog Agent via public HTTP endpoint using this spec https://docs.datadoghq.com/api/latest/tracing/

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.

edrevo · 2021-07-21T12:33:39Z

+                }
+            } catch (final InterruptedException e) {
+                if (shutdown) {
+                    return;


Why are you returning here? Wouldn't it be good for the caller to know that not all messages could be sent?

I'd say this exception may happen when we try to shut down the executor, and we're trying to do a graceful shutdown (waiting 30 secs), so I think it should be fine here.

In any case, we could refactor the approach in the next PRs basing the logic on runnable tasks, or something similar.

edrevo · 2021-07-21T12:36:15Z

+
+
+    boolean isShutdown() {
+        return shutdown;


With the current code, isShutdown could return false while the sender is still actively sending messages. Not sure if that is a concern.

I guess you mean the client is still actively sending messages. I think it should be fine for the moment because the shutdown only could happen when Jenkins is restarted, and all running pipelines must finish before the restart.

edrevo · 2021-07-21T12:50:19Z

+public interface HttpClient {
+
+    void send(PayloadMessage obj);
+
+    void stop();
+
+    void close();
+}


Is this plugin JRE8+ only? If so, you might want to consider using the async stuff that was added in Java8 (CompletableFuture & co). That would allow you to:

use your executor's threads more efficiently through the use of non-blocking http calls

not have to manage a manual queue of work (it would be managed by the executor)

allow the callers to decide what to do if their request failed

As we discussed offline, the best approach here had been to use the java.net.http.HttpClient to implement the HTTP calls asynchronously, but unfortunately, this class is only available since JDK11.

So this change would need an external library, which is something that could introduce issues in terms of dependency version clashes. To reduce the risk, we will use the approach based on java.net.HttpURLConnection for the moment.

drodriguezhdez · 2021-07-23T07:47:34Z

+            if(!spanBuffer.isEmpty()) {
+                final List<PayloadMessage> spanSendBuffer = new ArrayList<>(SIZE_SPANS_SEND_BUFFER);
+                for(int i = 0; i < spanBuffer.size(); i++) {
+                    spanSendBuffer.add(spanBuffer.get(i));
+
+                    // Send every 100 spans or the last one.
+                    if(spanSendBuffer.size() == SIZE_SPANS_SEND_BUFFER || i == (spanBuffer.size() - 1)) {
+                        agentHttpClient.send(Collections.unmodifiableList(spanSendBuffer));
+                        spanSendBuffer.clear();
+                    }
+                }
+            }


Avoid unbounded payloads

Could this be an internal of the client?

Yes, totally. 👌🏼

AdrianLC

I have some questions that I could not decide a line of code for:

What is the usage of DostatsgClient after this? Is it still called internally inside DatadogAgentHttpClient? is it called separately for metrics? is it not called at all anymore? (This is separate from the tracer AFAIK)
Could you share some info about test coverage for the client and flushing/buffering code? I've seen there are some tests but I'm not sure of what is really covered.
Could you comment a TL;DR for how the requests and flushing works? Is it a single separate thread doing blocking calls with timeout of a minute? What about retries, are we not retrying calls if there is a single 5XX error?

AdrianLC · 2021-07-23T08:10:02Z

-            tracePipelineLogic = new DatadogTracePipelineLogic(ddTracer);
+            this.stopAgentHttpClient();
+            logger.info("Re/Initialize Datadog-Plugin Agent Http Client");
+            final URL tracesURL = new URL("http://"+this.hostname+":"+this.traceCollectionPort+"/v0.3/traces");


this seems important enough to have it's own function or a constant with a template string

AdrianLC · 2021-07-23T08:14:00Z

-            final Tracer ddTracer = tracerBuilder.build();
-            traceBuildLogic = new DatadogTraceBuildLogic(ddTracer);
-            tracePipelineLogic = new DatadogTracePipelineLogic(ddTracer);
+            this.stopAgentHttpClient();


Move this out of the exception block as you are catching all exceptions inside. As it is now you're calling it again in this block's catch so it looks like a bug in exception handling.

Moved outside.

As it is now you're calling it again in this block's catch so it looks like a bug in exception handling.

Why a bug? I want to be sure that the agent client is stopped if an exception happened.

AdrianLC · 2021-07-23T08:28:45Z

    private static final Logger logger = Logger.getLogger(DatadogTracePipelineLogic.class.getName());

-    private final Tracer tracer;
+    private static final int SIZE_SPANS_SEND_BUFFER = 100;


How did you decide this number? It seems a bit too small.

It was arbitrary. I don't think we're going to have those amounts of spans in a single pipeline, though.
I'm opened to suggestions, but probably a greater number will not affect so much.

AdrianLC · 2021-07-23T08:31:04Z

+            if(!spanBuffer.isEmpty()) {
+                final List<PayloadMessage> spanSendBuffer = new ArrayList<>(SIZE_SPANS_SEND_BUFFER);
+                for(int i = 0; i < spanBuffer.size(); i++) {
+                    spanSendBuffer.add(spanBuffer.get(i));
+
+                    // Send every 100 spans or the last one.
+                    if(spanSendBuffer.size() == SIZE_SPANS_SEND_BUFFER || i == (spanBuffer.size() - 1)) {
+                        agentHttpClient.send(Collections.unmodifiableList(spanSendBuffer));
+                        spanSendBuffer.clear();
+                    }
+                }
+            }


Could this be an internal of the client?

AdrianLC · 2021-07-23T08:36:04Z

+public class IdGenerator {
+
+    public static long generate(){
+        return ThreadLocalRandom.current().nextLong(1, Long.MAX_VALUE);


is this copy paste from the tracer? will it be random enough?

Yes, it's the same approach that is used in the Java Tracer: https://github.com/DataDog/dd-trace-java/blob/master/dd-trace-api/src/main/java/datadog/trace/api/IdGenerationStrategy.java#L10

AdrianLC · 2021-07-23T08:42:03Z

+    }
+
+    @Override
+    public void run() {


cold you add a comment here explaining how this works?

I'm guessing it polls the queue every second until it's empty or shutdown is triggered but I have no idea.

Why would this stop if the queue is empty? If we have a very long span that doesn't flush for a while will this be empty and trigger stop?

It will stop only if shutdown=true and queue is empty. I refactored the conditions to be clearer.

AdrianLC · 2021-07-23T08:46:02Z

+        }
+    }
+
+    private void blockingSend(HttpMessage message) {


is this called only in the separate thread?

yes, the HttpSender instance is executed in a separate threaad.

drodriguezhdez · 2021-07-23T09:12:06Z

@AdrianLC

What is the usage of DostatsgClient after this? Is it still called internally inside DatadogAgentHttpClient? is it called separately for metrics? is it not called at all anymore? (This is separate from the tracer AFAIK)

I have refactored the DogStatsDClient class name to DatadogAgentClient which is the most accurate one. Inside of this class, we gather all "clients" (metrics, and traces).

Could you share some info about test coverage for the client and flushing/buffering code? I've seen there are some tests but I'm not sure of what is really covered.

Yes, I'll try to do this.

Could you comment a TL;DR for how the requests and flushing works? Is it a single separate thread doing blocking calls with a timeout of a minute? What about retries, are we not retrying calls if there is a single 5XX error?

Yes,

The mechanism is an immediate publisher-consumer approach with a queue in a single thread.
There is no flush mechanism, as soon as the consumer receives the message performs the call.
There is no retries mechanism in this PR because we're assuming that we're sending data to the Datadog Agent, so these kinds of errors are very unlikely to happen.
The HTTP calls have a 1 min timeout (again, we're sending data to the Datadog Agent)

AdrianLC · 2021-07-26T10:44:36Z

I have refactored the DogStatsDClient class name to DatadogAgentClient which is the most accurate one. Inside of this class, we gather all "clients" (metrics, and traces).

Why? Was the DogStatsDClient separate from the APM requests before?
https://docs.datadoghq.com/developers/dogstatsd/?tab=hostagent AFAIK is separate from traces as this implements the statsd open source standard so I'm not sure you should merge everything together.

There is no retries mechanism in this PR because we're assuming that we're sending data to the Datadog Agent, so these kinds of errors are very unlikely to happen.
The HTTP calls have a 1 min timeout (again, we're sending data to the Datadog Agent)

If we think the connectivity with the agent is going to work so well why would we need timeouts of 1min? This reads contradictory to me. I think it would be safer to have at least one retry and much lower timeouts (5 or 10 seconds).
What does the tracer do for these numbers?

drodriguezhdez · 2021-07-26T11:24:29Z

Why? Was the DogStatsDClient separate from the APM requests before?

The class name was wrong in the past because we're using it as the class to create the clients for DogStatsD, Logs, and Traces clients for the Agent. So the correct name should be DatadogAgentClient, and inside we gather or the other clients (Metrics, Logs, Traces, etc).

drodriguezhdez · 2021-07-26T11:53:17Z

If we think the connectivity with the agent is going to work so well why would we need timeouts of 1min? This reads contradictory to me

This method is used to create HttpURLConnection for both flavors, HTTP and Datadog Agent, that's why the default timeout is 1 minute. I'm going to refactor the method to set the timeout via argument.

I think it would be safer to have at least one retry and much lower timeouts (5 or 10 seconds). What does the tracer do for these numbers?

Tracer is using 10 seconds of timeout and no retries, AFAIK. I'll follow the same strategy.

Daniel Rodriguez Hernandez added 23 commits July 19, 2021 10:29

Rename to DatadogAgentClient

a0e2107

Adapt BuildLogic

030a649

Add TraceSpanContext

7ccc2cc

Transform TracePipelineLogic

2e6d545

Scaffold DatadogAgentHttpClient

9230c15

Fix TraceSpan constructor

d28684a

Use HTTP

5851e59

Merge with master

25479e8

Merge branch 'master' into drodriguezhdez/remove_java_tracer

0445ab2

Adapt tests Build

651e9e8

Adapt some tests

346306b

Adapt pipeline tests

caa3b98

Fix Serializable

3054ee1

NonBlocking Trace Sender

cf493f4

SuppressFBWarnings

d876669

Remove Java Tracer

1ca293d

Refactor

c3b2724

Refactor

0a07fbe

Add LoggerHttpErrorHandler

10fefa4

Add tests

ea55a16

Add sysout print

84c433d

Remove unnecesary code

f8d8eb5

Fix flaky test

84e55e7

edrevo reviewed Jul 21, 2021

View reviewed changes

Daniel Rodriguez Hernandez added 2 commits July 21, 2021 15:07

Add spans Buffer for pipelines

dc0211c

Fix tests

b3e3050

drodriguezhdez marked this pull request as ready for review July 21, 2021 15:55

Improve log messages

21ad2f8

drodriguezhdez commented Jul 23, 2021

View reviewed changes

edrevo previously approved these changes Jul 23, 2021

View reviewed changes

AdrianLC reviewed Jul 23, 2021

View reviewed changes

Daniel Rodriguez Hernandez added 3 commits July 26, 2021 12:49

Buffer strategy inside of HttpClient

9a9f9c4

Add comments to consumer loop

936b578

Refactor reinitialize agent Http client method

1967033

drodriguezhdez dismissed edrevo’s stale review via 1967033 July 26, 2021 11:17

Daniel Rodriguez Hernandez added 2 commits July 26, 2021 14:04

Use parameterized HTTP timeout

afcf1a5

Add HttpSender test

57ff91a

drodriguezhdez added the changelog/Added Added features results into a minor version bump label Jul 26, 2021

AdrianLC approved these changes Jul 26, 2021

View reviewed changes

drodriguezhdez merged commit e12186f into master Jul 27, 2021

drodriguezhdez deleted the drodriguezhdez/remove_java_tracer branch July 27, 2021 09:39

Uh oh!

Conversation

drodriguezhdez commented Jul 21, 2021 • 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!

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!

drodriguezhdez Jul 22, 2021 • 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!

AdrianLC 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!

drodriguezhdez Jul 26, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

drodriguezhdez commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AdrianLC commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodriguezhdez commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodriguezhdez commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

drodriguezhdez commented Jul 21, 2021 •

edited

Loading

drodriguezhdez Jul 22, 2021 •

edited

Loading

drodriguezhdez Jul 26, 2021 •

edited

Loading

drodriguezhdez commented Jul 23, 2021 •

edited

Loading

AdrianLC commented Jul 26, 2021 •

edited

Loading

drodriguezhdez commented Jul 26, 2021 •

edited

Loading

drodriguezhdez commented Jul 26, 2021 •

edited

Loading