From 451fba256a5068b3b40871dea630b5ede4213823 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 2 Jan 2020 08:19:50 -0800 Subject: [PATCH 01/17] Split TraceConsumer into two different disruptors First disruptor (TraceProcessingDisruptor) does processing, which is currently limited to serialization, but in the future can do other processing such as TraceInterceptor invocation. Second disruptor (BatchWritingDisruptor) takes serialized traces and batches them into groups and flushes them periodically based on size and time. --- dd-trace-ot/dd-trace-ot.gradle | 2 + .../trace/common/writer/DDAgentWriter.java | 157 ++++++---------- .../datadog/trace/common/writer/Writer.java | 1 + .../writer/ddagent/AbstractDisruptor.java | 89 +++++++++ .../writer/ddagent/BatchWritingDisruptor.java | 162 +++++++++++++++++ .../common/writer/ddagent/DisruptorEvent.java | 49 +++-- .../common/writer/ddagent/TraceConsumer.java | 150 --------------- .../ddagent/TraceProcessingDisruptor.java | 88 +++++++++ .../ddagent/TraceSerializingDisruptor.java | 117 ------------ .../trace/api/writer/DDAgentWriterTest.groovy | 172 ++++++++++-------- 10 files changed, 534 insertions(+), 453 deletions(-) create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/AbstractDisruptor.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java delete mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceConsumer.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java delete mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceSerializingDisruptor.java diff --git a/dd-trace-ot/dd-trace-ot.gradle b/dd-trace-ot/dd-trace-ot.gradle index f943f74e64..96f9a4bd6d 100644 --- a/dd-trace-ot/dd-trace-ot.gradle +++ b/dd-trace-ot/dd-trace-ot.gradle @@ -12,6 +12,8 @@ minimumInstructionCoverage = 0.6 excludedClassesCoverage += [ 'datadog.trace.common.writer.ListWriter', 'datadog.trace.common.writer.LoggingWriter', + 'datadog.trace.common.writer.DDAgentWriter.Spec', + 'datadog.trace.common.writer.DDAgentWriter.Spec.SpecBuilder', 'datadog.trace.common.sampling.PrioritySampling', // This code is copied from okHttp samples and we have integration tests to verify that it works. 'datadog.trace.common.writer.unixdomainsockets.TunnelingUnixSocket', diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java index c63fe9ff8f..965976d4ba 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java @@ -3,21 +3,16 @@ package datadog.trace.common.writer; import static datadog.trace.api.Config.DEFAULT_AGENT_HOST; import static datadog.trace.api.Config.DEFAULT_AGENT_UNIX_DOMAIN_SOCKET; import static datadog.trace.api.Config.DEFAULT_TRACE_AGENT_PORT; -import static java.util.concurrent.TimeUnit.SECONDS; import datadog.opentracing.DDSpan; -import datadog.trace.common.util.DaemonThreadFactory; +import datadog.trace.common.writer.ddagent.BatchWritingDisruptor; import datadog.trace.common.writer.ddagent.DDAgentApi; import datadog.trace.common.writer.ddagent.DDAgentResponseListener; import datadog.trace.common.writer.ddagent.Monitor; -import datadog.trace.common.writer.ddagent.TraceConsumer; -import datadog.trace.common.writer.ddagent.TraceSerializingDisruptor; +import datadog.trace.common.writer.ddagent.TraceProcessingDisruptor; import java.util.List; -import java.util.concurrent.Executors; -import java.util.concurrent.Phaser; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.ThreadFactory; import java.util.concurrent.atomic.AtomicInteger; +import lombok.Value; import lombok.extern.slf4j.Slf4j; /** @@ -29,86 +24,65 @@ import lombok.extern.slf4j.Slf4j; */ @Slf4j public class DDAgentWriter implements Writer { - private static final int DISRUPTOR_BUFFER_SIZE = 1024; - private static final int SENDER_QUEUE_SIZE = 16; - private static final int FLUSH_PAYLOAD_DELAY = 1; // 1/second + @Value + @lombok.Builder + public static class Spec { + @lombok.Builder.Default public String agentHost = DEFAULT_AGENT_HOST; + @lombok.Builder.Default public int traceAgentPort = DEFAULT_TRACE_AGENT_PORT; + @lombok.Builder.Default public String unixDomainSocket = DEFAULT_AGENT_UNIX_DOMAIN_SOCKET; + @lombok.Builder.Default public int traceBufferSize = DISRUPTOR_BUFFER_SIZE; + @lombok.Builder.Default public Monitor monitor = new Monitor.Noop(); + @lombok.Builder.Default public int flushFrequencySeconds = 1; + } - private static final ThreadFactory SCHEDULED_FLUSH_THREAD_FACTORY = - new DaemonThreadFactory("dd-trace-writer"); + private static final int DISRUPTOR_BUFFER_SIZE = 1024; private final DDAgentApi api; - public final int flushFrequencySeconds; - public final TraceSerializingDisruptor disruptor; + public final TraceProcessingDisruptor traceProcessingDisruptor; + public final BatchWritingDisruptor batchWritingDisruptor; - public final ScheduledExecutorService scheduledWriterExecutor; private final AtomicInteger traceCount = new AtomicInteger(0); - public final Phaser apiPhaser = new Phaser(); // Ensure API calls are completed when flushing; public final Monitor monitor; public DDAgentWriter() { - this( - new DDAgentApi( - DEFAULT_AGENT_HOST, DEFAULT_TRACE_AGENT_PORT, DEFAULT_AGENT_UNIX_DOMAIN_SOCKET), - new Monitor.Noop()); + this(Spec.builder().build()); } + public DDAgentWriter(final Spec spec) { + api = new DDAgentApi(spec.agentHost, spec.traceAgentPort, spec.unixDomainSocket); + monitor = spec.monitor; + + batchWritingDisruptor = + new BatchWritingDisruptor( + spec.traceBufferSize, spec.flushFrequencySeconds, api, monitor, this); + traceProcessingDisruptor = + new TraceProcessingDisruptor( + spec.traceBufferSize, api, batchWritingDisruptor, monitor, this); + } + + @Deprecated public DDAgentWriter(final DDAgentApi api, final Monitor monitor) { - this(api, monitor, DISRUPTOR_BUFFER_SIZE, SENDER_QUEUE_SIZE, FLUSH_PAYLOAD_DELAY); - } - - /** Old signature (pre-Monitor) used in tests */ - private DDAgentWriter(final DDAgentApi api) { - this(api, new Monitor.Noop()); - } - - /** - * Used in the tests. - * - * @param api - * @param disruptorSize Rounded up to next power of 2 - * @param flushFrequencySeconds value < 1 disables scheduled flushes - */ - private DDAgentWriter( - final DDAgentApi api, - final int disruptorSize, - final int senderQueueSize, - final int flushFrequencySeconds) { - this(api, new Monitor.Noop(), disruptorSize, senderQueueSize, flushFrequencySeconds); - } - - // DQH - TODO - Update the tests & remove this - private DDAgentWriter( - final DDAgentApi api, - final Monitor monitor, - final int disruptorSize, - final int flushFrequencySeconds) { - this(api, monitor, disruptorSize, SENDER_QUEUE_SIZE, flushFrequencySeconds); - } - - // DQH - TODO - Update the tests & remove this - private DDAgentWriter( - final DDAgentApi api, final int disruptorSize, final int flushFrequencySeconds) { - this(api, new Monitor.Noop(), disruptorSize, SENDER_QUEUE_SIZE, flushFrequencySeconds); - } - - private DDAgentWriter( - final DDAgentApi api, - final Monitor monitor, - final int disruptorSize, - final int senderQueueSize, - final int flushFrequencySeconds) { this.api = api; this.monitor = monitor; - disruptor = - new TraceSerializingDisruptor( - disruptorSize, this, new TraceConsumer(traceCount, senderQueueSize, this)); + batchWritingDisruptor = new BatchWritingDisruptor(DISRUPTOR_BUFFER_SIZE, 1, api, monitor, this); + traceProcessingDisruptor = + new TraceProcessingDisruptor( + DISRUPTOR_BUFFER_SIZE, api, batchWritingDisruptor, monitor, this); + } - this.flushFrequencySeconds = flushFrequencySeconds; - scheduledWriterExecutor = Executors.newScheduledThreadPool(1, SCHEDULED_FLUSH_THREAD_FACTORY); + @Deprecated + // DQH - TODO - Update the tests & remove this + private DDAgentWriter( + final DDAgentApi api, final int disruptorSize, final int flushFrequencySeconds) { + this.api = api; + monitor = new Monitor.Noop(); - apiPhaser.register(); // Register on behalf of the scheduled executor thread. + batchWritingDisruptor = + new BatchWritingDisruptor(disruptorSize, flushFrequencySeconds, api, monitor, this); + traceProcessingDisruptor = + new TraceProcessingDisruptor(disruptorSize, api, batchWritingDisruptor, monitor, this); } public void addResponseListener(final DDAgentResponseListener listener) { @@ -117,7 +91,7 @@ public class DDAgentWriter implements Writer { // Exposing some statistics for consumption by monitors public final long getDisruptorCapacity() { - return disruptor.getDisruptorCapacity(); + return traceProcessingDisruptor.getDisruptorCapacity(); } public final long getDisruptorUtilizedCapacity() { @@ -125,20 +99,21 @@ public class DDAgentWriter implements Writer { } public final long getDisruptorRemainingCapacity() { - return disruptor.getDisruptorRemainingCapacity(); + return traceProcessingDisruptor.getDisruptorRemainingCapacity(); } @Override public void write(final List trace) { // We can't add events after shutdown otherwise it will never complete shutting down. - if (disruptor.running) { - final boolean published = disruptor.tryPublish(trace); + if (traceProcessingDisruptor.running) { + final int representativeCount = traceCount.getAndSet(0) + 1; + final boolean published = traceProcessingDisruptor.publish(trace, representativeCount); if (published) { monitor.onPublish(DDAgentWriter.this, trace); } else { // We're discarding the trace, but we still want to count it. - traceCount.incrementAndGet(); + traceCount.addAndGet(representativeCount); log.debug("Trace written to overfilled buffer. Counted but dropping trace: {}", trace); monitor.onFailedPublish(this, trace); @@ -150,6 +125,10 @@ public class DDAgentWriter implements Writer { } } + public boolean flush() { + return traceProcessingDisruptor.flush(traceCount.getAndSet(0)); + } + @Override public void incrementTraceCount() { traceCount.incrementAndGet(); @@ -161,32 +140,16 @@ public class DDAgentWriter implements Writer { @Override public void start() { - disruptor.start(); - + batchWritingDisruptor.start(); + traceProcessingDisruptor.start(); monitor.onStart(this); } @Override public void close() { - - boolean flushSuccess = true; - - // We have to shutdown scheduled executor first to make sure no flush events issued after - // disruptor has been shutdown. - // Otherwise those events will never be processed and flush call will wait forever. - scheduledWriterExecutor.shutdown(); - try { - scheduledWriterExecutor.awaitTermination(flushFrequencySeconds, SECONDS); - } catch (final InterruptedException e) { - log.warn("Waiting for flush executor shutdown interrupted.", e); - - flushSuccess = false; - } - flushSuccess |= disruptor.flush(); - - disruptor.close(); - - monitor.onShutdown(this, flushSuccess); + monitor.onShutdown(this, traceProcessingDisruptor.flush(traceCount.getAndSet(0))); + traceProcessingDisruptor.close(); + batchWritingDisruptor.close(); } @Override diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/Writer.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/Writer.java index b14cbfdd23..51e0e85c4b 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/Writer.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/Writer.java @@ -64,6 +64,7 @@ public interface Writer extends Closeable { } private static Writer createAgentWriter(final Config config) { + // TODO: switch to using DDAgentWriter.Spec constructor... return new DDAgentWriter(createApi(config), createMonitor(config)); } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/AbstractDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/AbstractDisruptor.java new file mode 100644 index 0000000000..2bff4028ac --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/AbstractDisruptor.java @@ -0,0 +1,89 @@ +package datadog.trace.common.writer.ddagent; + +import com.lmax.disruptor.EventHandler; +import com.lmax.disruptor.SleepingWaitStrategy; +import com.lmax.disruptor.dsl.Disruptor; +import com.lmax.disruptor.dsl.ProducerType; +import java.io.Closeable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; +import lombok.extern.slf4j.Slf4j; + +@Slf4j +public abstract class AbstractDisruptor implements Closeable { + + protected final Disruptor> disruptor; + + public volatile boolean running = false; + + protected final DisruptorEvent.FlushTranslator flushTranslator = + new DisruptorEvent.FlushTranslator<>(); + protected final DisruptorEvent.DataTranslator dataTranslator = + new DisruptorEvent.DataTranslator<>(); + + public AbstractDisruptor(final int disruptorSize, final EventHandler> handler) { + disruptor = + new Disruptor<>( + new DisruptorEvent.Factory(), + Math.max(2, Integer.highestOneBit(disruptorSize - 1) << 1), // Next power of 2 + getThreadFactory(), + ProducerType.MULTI, + new SleepingWaitStrategy(0, TimeUnit.MILLISECONDS.toNanos(5))); + disruptor.handleEventsWith(handler); + } + + protected abstract ThreadFactory getThreadFactory(); + + public void start() { + disruptor.start(); + running = true; + } + + @Override + public void close() { + running = false; + disruptor.shutdown(); + } + + public abstract boolean publish(final T data, int representativeCount); + + /** + * This method will block until the flush is complete. + * + * @param traceCount - number of unreported traces to include in this batch. + */ + public boolean flush(final int traceCount) { + if (running) { + return flush(traceCount, new CountDownLatch(1)); + } else { + return false; + } + } + + /** This method will block until the flush is complete. */ + protected boolean flush(final int traceCount, final CountDownLatch latch) { + log.info("Flushing any remaining traces."); + disruptor.publishEvent(flushTranslator, traceCount, latch); + try { + latch.await(); + return true; + } catch (final InterruptedException e) { + log.warn("Waiting for flush interrupted.", e); + return false; + } + } + + // Exposing some statistics for consumption by monitors + public final long getDisruptorCapacity() { + return disruptor.getRingBuffer().getBufferSize(); + } + + public final long getDisruptorRemainingCapacity() { + return disruptor.getRingBuffer().remainingCapacity(); + } + + public final long getCurrentCount() { + return disruptor.getCursor() - disruptor.getRingBuffer().getMinimumGatingSequence(); + } +} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java new file mode 100644 index 0000000000..fc08591548 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java @@ -0,0 +1,162 @@ +package datadog.trace.common.writer.ddagent; + +import com.lmax.disruptor.EventHandler; +import datadog.trace.common.util.DaemonThreadFactory; +import datadog.trace.common.writer.DDAgentWriter; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; +import lombok.extern.slf4j.Slf4j; + +@Slf4j +public class BatchWritingDisruptor extends AbstractDisruptor { + private static final int FLUSH_PAYLOAD_BYTES = 5_000_000; // 5 MB + + private final ScheduledExecutorService heartbeatExecutor = + Executors.newScheduledThreadPool(1, new DaemonThreadFactory("dd-trace-heartbeat")); + + private final DisruptorEvent.HeartbeatTranslator heartbeatTranslator = + new DisruptorEvent.HeartbeatTranslator(); + + public BatchWritingDisruptor( + final int disruptorSize, + final int flushFrequencySeconds, + final DDAgentApi api, + final Monitor monitor, + final DDAgentWriter writer) { + super(disruptorSize, new BatchWritingHandler(flushFrequencySeconds, api, monitor, writer)); + + if (0 < flushFrequencySeconds) { + // This provides a steady stream of events to enable flushing with a low throughput. + final Runnable heartbeat = + new Runnable() { + @Override + public void run() { + // Only add if the buffer is empty. + if (running && getCurrentCount() == 0) { + disruptor.getRingBuffer().tryPublishEvent(heartbeatTranslator); + } + } + }; + heartbeatExecutor.scheduleAtFixedRate(heartbeat, 100, 100, TimeUnit.MILLISECONDS); + } + } + + @Override + protected ThreadFactory getThreadFactory() { + return new DaemonThreadFactory("dd-trace-writer"); + } + + @Override + public boolean publish(final byte[] data, final int representativeCount) { + disruptor.getRingBuffer().publishEvent(dataTranslator, data, representativeCount); + return true; + } + + // Intentionally not thread safe. + private static class BatchWritingHandler implements EventHandler> { + + private final long flushFrequencyNanos; + private final DDAgentApi api; + private final Monitor monitor; + private final DDAgentWriter writer; + private final List serializedTraces = new ArrayList<>(); + private int representativeCount = 0; + private int sizeInBytes = 0; + private long nextScheduledFlush; + + private BatchWritingHandler( + final int flushFrequencySeconds, + final DDAgentApi api, + final Monitor monitor, + final DDAgentWriter writer) { + flushFrequencyNanos = TimeUnit.SECONDS.toNanos(flushFrequencySeconds); + scheduleNextFlush(); + this.api = api; + this.monitor = monitor; + this.writer = writer; + } + + @Override + public void onEvent( + final DisruptorEvent event, final long sequence, final boolean endOfBatch) { + try { + if (event.data != null) { + sizeInBytes += event.data.length; + serializedTraces.add(event.data); + } + + // Flush events might increase this with no data. + representativeCount += event.representativeCount; + + if (event.flushLatch != null + || FLUSH_PAYLOAD_BYTES <= sizeInBytes + || nextScheduledFlush <= System.nanoTime()) { + flush(event.flushLatch, FLUSH_PAYLOAD_BYTES <= sizeInBytes); + } + } finally { + event.reset(); + } + } + + private void flush(final CountDownLatch flushLatch, final boolean early) { + try { + if (serializedTraces.isEmpty()) { + // FIXME: this will reset representativeCount without reporting + // anything even if representativeCount > 0. + return; + } + + monitor.onFlush(writer, early); + // TODO add retry and rate limiting + final DDAgentApi.Response response = + api.sendSerializedTraces(representativeCount, sizeInBytes, serializedTraces); + + if (response.success()) { + log.debug("Successfully sent {} traces to the API", serializedTraces.size()); + + monitor.onSend(writer, representativeCount, sizeInBytes, response); + } else { + log.debug( + "Failed to send {} traces (representing {}) of size {} bytes to the API", + serializedTraces.size(), + representativeCount, + sizeInBytes); + + monitor.onFailedSend(writer, representativeCount, sizeInBytes, response); + } + } catch (final Throwable e) { + log.debug("Failed to send traces to the API: {}", e.getMessage()); + + // DQH - 10/2019 - DDApi should wrap most exceptions itself, so this really + // shouldn't occur. + // However, just to be safe to start, create a failed Response to handle any + // spurious Throwable-s. + monitor.onFailedSend( + writer, representativeCount, sizeInBytes, DDAgentApi.Response.failed(e)); + } finally { + serializedTraces.clear(); + sizeInBytes = 0; + representativeCount = 0; + scheduleNextFlush(); + + if (flushLatch != null) { + flushLatch.countDown(); + } + } + } + + private void scheduleNextFlush() { + // TODO: adjust this depending on responsiveness of the agent. + if (0 < flushFrequencyNanos) { + nextScheduledFlush = System.nanoTime() + flushFrequencyNanos; + } else { + nextScheduledFlush = Long.MAX_VALUE; + } + } + } +} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DisruptorEvent.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DisruptorEvent.java index 66ff7a499b..8e754fa3fe 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DisruptorEvent.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DisruptorEvent.java @@ -2,13 +2,19 @@ package datadog.trace.common.writer.ddagent; import com.lmax.disruptor.EventFactory; import com.lmax.disruptor.EventTranslator; -import com.lmax.disruptor.EventTranslatorOneArg; -import datadog.opentracing.DDSpan; -import java.util.List; +import com.lmax.disruptor.EventTranslatorTwoArg; +import java.util.concurrent.CountDownLatch; class DisruptorEvent { - public volatile boolean shouldFlush = false; public volatile T data = null; + public volatile int representativeCount = 0; + public volatile CountDownLatch flushLatch = null; + + public void reset() { + data = null; + representativeCount = 0; + flushLatch = null; + } static class Factory implements EventFactory> { @Override @@ -17,25 +23,38 @@ class DisruptorEvent { } } - static class TraceTranslator - implements EventTranslatorOneArg>, List> { - static final DisruptorEvent.TraceTranslator TRACE_TRANSLATOR = - new DisruptorEvent.TraceTranslator(); + static class DataTranslator implements EventTranslatorTwoArg, T, Integer> { @Override public void translateTo( - final DisruptorEvent> event, final long sequence, final List trace) { - event.data = trace; + final DisruptorEvent event, + final long sequence, + final T data, + final Integer representativeCount) { + event.data = data; + event.representativeCount = representativeCount; } } - static class FlushTranslator implements EventTranslator>> { - static final DisruptorEvent.FlushTranslator FLUSH_TRANSLATOR = - new DisruptorEvent.FlushTranslator(); + static class HeartbeatTranslator implements EventTranslator> { @Override - public void translateTo(final DisruptorEvent> event, final long sequence) { - event.shouldFlush = true; + public void translateTo(final DisruptorEvent event, final long sequence) { + return; + } + } + + static class FlushTranslator + implements EventTranslatorTwoArg, Integer, CountDownLatch> { + + @Override + public void translateTo( + final DisruptorEvent event, + final long sequence, + final Integer representativeCount, + final CountDownLatch latch) { + event.representativeCount = representativeCount; + event.flushLatch = latch; } } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceConsumer.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceConsumer.java deleted file mode 100644 index 1081f4352e..0000000000 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceConsumer.java +++ /dev/null @@ -1,150 +0,0 @@ -package datadog.trace.common.writer.ddagent; - -import com.fasterxml.jackson.core.JsonProcessingException; -import com.lmax.disruptor.EventHandler; -import datadog.opentracing.DDSpan; -import datadog.trace.common.writer.DDAgentWriter; -import java.util.ArrayList; -import java.util.List; -import java.util.concurrent.RejectedExecutionException; -import java.util.concurrent.Semaphore; -import java.util.concurrent.atomic.AtomicInteger; -import lombok.extern.slf4j.Slf4j; - -/** This class is intentionally not threadsafe. */ -@Slf4j -public class TraceConsumer implements EventHandler>> { - private static final int FLUSH_PAYLOAD_BYTES = 5_000_000; // 5 MB - - private final AtomicInteger traceCount; - private final Semaphore senderSemaphore; - private final DDAgentWriter writer; - - private List serializedTraces = new ArrayList<>(); - private int payloadSize = 0; - - public TraceConsumer( - final AtomicInteger traceCount, final int senderQueueSize, final DDAgentWriter writer) { - this.traceCount = traceCount; - senderSemaphore = new Semaphore(senderQueueSize); - this.writer = writer; - } - - @Override - public void onEvent( - final DisruptorEvent> event, final long sequence, final boolean endOfBatch) { - final List trace = event.data; - event.data = null; // clear the event for reuse. - if (trace != null) { - traceCount.incrementAndGet(); - try { - final byte[] serializedTrace = writer.getApi().serializeTrace(trace); - payloadSize += serializedTrace.length; - serializedTraces.add(serializedTrace); - - writer.monitor.onSerialize(writer, trace, serializedTrace); - } catch (final JsonProcessingException e) { - log.warn("Error serializing trace", e); - - writer.monitor.onFailedSerialize(writer, trace, e); - } catch (final Throwable e) { - log.debug("Error while serializing trace", e); - - writer.monitor.onFailedSerialize(writer, trace, e); - } - } - - if (event.shouldFlush || payloadSize >= FLUSH_PAYLOAD_BYTES) { - final boolean early = (payloadSize >= FLUSH_PAYLOAD_BYTES); - - reportTraces(early); - event.shouldFlush = false; - } - } - - private void reportTraces(final boolean early) { - try { - if (serializedTraces.isEmpty()) { - writer.monitor.onFlush(writer, early); - - writer.apiPhaser.arrive(); // Allow flush to return - return; - // scheduleFlush called in finally block. - } - if (writer.scheduledWriterExecutor.isShutdown()) { - writer.monitor.onFailedSend( - writer, traceCount.get(), payloadSize, DDAgentApi.Response.failed(-1)); - writer.apiPhaser.arrive(); // Allow flush to return - return; - } - final List toSend = serializedTraces; - serializedTraces = new ArrayList<>(toSend.size()); - // ^ Initialize with similar size to reduce arraycopy churn. - - final int representativeCount = traceCount.getAndSet(0); - final int sizeInBytes = payloadSize; - - try { - writer.monitor.onFlush(writer, early); - - // Run the actual IO task on a different thread to avoid blocking the consumer. - try { - senderSemaphore.acquire(); - } catch (final InterruptedException e) { - writer.monitor.onFailedSend( - writer, representativeCount, sizeInBytes, DDAgentApi.Response.failed(e)); - - // Finally, we'll schedule another flush - // Any threads awaiting the flush will continue to wait - return; - } - writer.scheduledWriterExecutor.execute( - new Runnable() { - @Override - public void run() { - senderSemaphore.release(); - - try { - final DDAgentApi.Response response = - writer - .getApi() - .sendSerializedTraces(representativeCount, sizeInBytes, toSend); - - if (response.success()) { - log.debug("Successfully sent {} traces to the API", toSend.size()); - - writer.monitor.onSend(writer, representativeCount, sizeInBytes, response); - } else { - log.debug( - "Failed to send {} traces (representing {}) of size {} bytes to the API", - toSend.size(), - representativeCount, - sizeInBytes); - - writer.monitor.onFailedSend(writer, representativeCount, sizeInBytes, response); - } - } catch (final Throwable e) { - log.debug("Failed to send traces to the API: {}", e.getMessage()); - - // DQH - 10/2019 - DDApi should wrap most exceptions itself, so this really - // shouldn't occur. - // However, just to be safe to start, create a failed Response to handle any - // spurious Throwable-s. - writer.monitor.onFailedSend( - writer, representativeCount, sizeInBytes, DDAgentApi.Response.failed(e)); - } finally { - writer.apiPhaser.arrive(); // Flush completed. - } - } - }); - } catch (final RejectedExecutionException ex) { - writer.monitor.onFailedSend( - writer, representativeCount, sizeInBytes, DDAgentApi.Response.failed(ex)); - writer.apiPhaser.arrive(); // Allow flush to return - } - } finally { - payloadSize = 0; - writer.disruptor.scheduleFlush(); - } - } -} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java new file mode 100644 index 0000000000..0088227be7 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java @@ -0,0 +1,88 @@ +package datadog.trace.common.writer.ddagent; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.lmax.disruptor.EventHandler; +import datadog.opentracing.DDSpan; +import datadog.trace.common.util.DaemonThreadFactory; +import datadog.trace.common.writer.DDAgentWriter; +import java.util.List; +import java.util.concurrent.ThreadFactory; +import lombok.extern.slf4j.Slf4j; + +@Slf4j +public class TraceProcessingDisruptor extends AbstractDisruptor> { + + public TraceProcessingDisruptor( + final int disruptorSize, + final DDAgentApi api, + final BatchWritingDisruptor batchWritingDisruptor, + final Monitor monitor, + final DDAgentWriter writer) { + // TODO: add config to enable control over serialization overhead. + super(disruptorSize, new TraceSerializingHandler(api, batchWritingDisruptor, monitor, writer)); + } + + @Override + protected ThreadFactory getThreadFactory() { + return new DaemonThreadFactory("dd-trace-processor"); + } + + @Override + public boolean publish(final List data, final int representativeCount) { + return disruptor.getRingBuffer().tryPublishEvent(dataTranslator, data, representativeCount); + } + + // This class is threadsafe if we want to enable more processors. + public static class TraceSerializingHandler + implements EventHandler>> { + private final DDAgentApi api; + private final BatchWritingDisruptor batchWritingDisruptor; + private final Monitor monitor; + private final DDAgentWriter writer; + + public TraceSerializingHandler( + final DDAgentApi api, + final BatchWritingDisruptor batchWritingDisruptor, + final Monitor monitor, + final DDAgentWriter writer) { + this.api = api; + this.batchWritingDisruptor = batchWritingDisruptor; + this.monitor = monitor; + this.writer = writer; + } + + @Override + public void onEvent( + final DisruptorEvent> event, final long sequence, final boolean endOfBatch) { + try { + if (event.data != null) { + try { + final byte[] serializedTrace = api.serializeTrace(event.data); + monitor.onSerialize(writer, event.data, serializedTrace); + batchWritingDisruptor.publish(serializedTrace, event.representativeCount); + event.representativeCount = 0; // reset in case flush is invoked below. + } catch (final JsonProcessingException e) { + log.debug("Error serializing trace", e); + monitor.onFailedSerialize(writer, event.data, e); + } catch (final Throwable e) { + log.debug("Error while serializing trace", e); + monitor.onFailedSerialize(writer, event.data, e); + } + } + + if (event.flushLatch != null) { + if (batchWritingDisruptor.running) { + // propagate the flush. + batchWritingDisruptor.flush(event.representativeCount, event.flushLatch); + } + if (!batchWritingDisruptor.running) { // check again to protect against race condition. + // got shutdown early somehow? + event.flushLatch.countDown(); + } + } + } finally { + event.reset(); + } + } + } +} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceSerializingDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceSerializingDisruptor.java deleted file mode 100644 index f878cbb178..0000000000 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceSerializingDisruptor.java +++ /dev/null @@ -1,117 +0,0 @@ -package datadog.trace.common.writer.ddagent; - -import static datadog.trace.common.writer.ddagent.DisruptorEvent.FlushTranslator.FLUSH_TRANSLATOR; -import static datadog.trace.common.writer.ddagent.DisruptorEvent.TraceTranslator.TRACE_TRANSLATOR; -import static java.util.concurrent.TimeUnit.SECONDS; - -import com.lmax.disruptor.SleepingWaitStrategy; -import com.lmax.disruptor.dsl.Disruptor; -import com.lmax.disruptor.dsl.ProducerType; -import datadog.opentracing.DDSpan; -import datadog.trace.common.util.DaemonThreadFactory; -import datadog.trace.common.writer.DDAgentWriter; -import java.io.Closeable; -import java.util.List; -import java.util.concurrent.ScheduledFuture; -import java.util.concurrent.ThreadFactory; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicReference; -import lombok.extern.slf4j.Slf4j; - -@Slf4j -public class TraceSerializingDisruptor implements Closeable { - private static final ThreadFactory DISRUPTOR_THREAD_FACTORY = - new DaemonThreadFactory("dd-trace-disruptor"); - private final FlushTask flushTask = new FlushTask(); - - private final Disruptor>> disruptor; - private final DDAgentWriter writer; - - public volatile boolean running = false; - - private final AtomicReference> flushSchedule = new AtomicReference<>(); - - public TraceSerializingDisruptor( - final int disruptorSize, final DDAgentWriter writer, final TraceConsumer handler) { - disruptor = - new Disruptor<>( - new DisruptorEvent.Factory>(), - Math.max(2, Integer.highestOneBit(disruptorSize - 1) << 1), // Next power of 2 - DISRUPTOR_THREAD_FACTORY, - ProducerType.MULTI, - new SleepingWaitStrategy(0, TimeUnit.MILLISECONDS.toNanos(5))); - this.writer = writer; - disruptor.handleEventsWith(handler); - } - - public void start() { - disruptor.start(); - running = true; - scheduleFlush(); - } - - @Override - public void close() { - running = false; - disruptor.shutdown(); - } - - public boolean tryPublish(final List trace) { - return disruptor.getRingBuffer().tryPublishEvent(TRACE_TRANSLATOR, trace); - } - - /** This method will block until the flush is complete. */ - public boolean flush() { - if (running) { - log.info("Flushing any remaining traces."); - // Register with the phaser so we can block until the flush completion. - writer.apiPhaser.register(); - disruptor.publishEvent(FLUSH_TRANSLATOR); - try { - // Allow thread to be interrupted. - writer.apiPhaser.awaitAdvanceInterruptibly(writer.apiPhaser.arriveAndDeregister()); - - return true; - } catch (final InterruptedException e) { - log.warn("Waiting for flush interrupted.", e); - - return false; - } - } else { - return false; - } - } - - public void scheduleFlush() { - if (writer.flushFrequencySeconds > 0 && !writer.scheduledWriterExecutor.isShutdown()) { - final ScheduledFuture previous = - flushSchedule.getAndSet( - writer.scheduledWriterExecutor.schedule( - flushTask, writer.flushFrequencySeconds, SECONDS)); - - final boolean previousIncomplete = (previous != null); - if (previousIncomplete) { - previous.cancel(true); - } - - writer.monitor.onScheduleFlush(writer, previousIncomplete); - } - } - - private class FlushTask implements Runnable { - @Override - public void run() { - // Don't call flush() because it would block the thread also used for sending the traces. - disruptor.publishEvent(FLUSH_TRANSLATOR); - } - } - - // Exposing some statistics for consumption by monitors - public final long getDisruptorCapacity() { - return disruptor.getRingBuffer().getBufferSize(); - } - - public final long getDisruptorRemainingCapacity() { - return disruptor.getRingBuffer().remainingCapacity(); - } -} diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy index 37de61bf23..d954fe6594 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy @@ -7,12 +7,13 @@ import datadog.opentracing.DDTracer import datadog.opentracing.PendingTrace import datadog.trace.api.sampling.PrioritySampling import datadog.trace.common.writer.DDAgentWriter +import datadog.trace.common.writer.ddagent.BatchWritingDisruptor import datadog.trace.common.writer.ddagent.DDAgentApi import datadog.trace.common.writer.ddagent.Monitor -import datadog.trace.common.writer.ddagent.TraceConsumer import datadog.trace.util.test.DDSpecification import spock.lang.Timeout +import java.util.concurrent.Phaser import java.util.concurrent.Semaphore import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicInteger @@ -24,21 +25,41 @@ import static datadog.trace.common.writer.DDAgentWriter.DISRUPTOR_BUFFER_SIZE @Timeout(20) class DDAgentWriterTest extends DDSpecification { - def api = Mock(DDAgentApi) + def phaser = new Phaser() + def api = Mock(DDAgentApi) { + // Define the following response in the spec: +// sendSerializedTraces(_, _, _) >> { +// phaser.arrive() +// return DDAgentApi.Response.success(200) +// } + } + def monitor = Mock(Monitor) + + def setup() { + // Register for two threads. + phaser.register() + phaser.register() + } def "test happy path"() { setup: def writer = new DDAgentWriter(api, 2, -1) writer.start() + when: + writer.flush() + + then: + 0 * _ + when: writer.write(trace) writer.write(trace) - writer.disruptor.flush() + writer.flush() then: 2 * api.serializeTrace(_) >> { trace -> callRealMethod() } - 1 * api.sendSerializedTraces(2, _, { it.size() == 2 }) + 1 * api.sendSerializedTraces(2, _, { it.size() == 2 }) >> DDAgentApi.Response.success(200) 0 * _ cleanup: @@ -57,11 +78,11 @@ class DDAgentWriterTest extends DDSpecification { (1..traceCount).each { writer.write(trace) } - writer.disruptor.flush() + writer.flush() then: _ * api.serializeTrace(_) >> { trace -> callRealMethod() } - 1 * api.sendSerializedTraces(traceCount, _, { it.size() < traceCount }) + 1 * api.sendSerializedTraces(traceCount, _, { it.size() < traceCount }) >> DDAgentApi.Response.success(200) 0 * _ cleanup: @@ -76,9 +97,7 @@ class DDAgentWriterTest extends DDSpecification { def "test flush by size"() { setup: def writer = new DDAgentWriter(api, DISRUPTOR_BUFFER_SIZE, -1) - def phaser = writer.apiPhaser writer.start() - phaser.register() when: (1..6).each { @@ -90,18 +109,21 @@ class DDAgentWriterTest extends DDSpecification { then: 6 * api.serializeTrace(_) >> { trace -> callRealMethod() } - 2 * api.sendSerializedTraces(3, _, { it.size() == 3 }) + 2 * api.sendSerializedTraces(3, _, { it.size() == 3 }) >> { + phaser.arrive() + return DDAgentApi.Response.success(200) + } when: (1..2).each { writer.write(trace) } // Flush the remaining 2 - writer.disruptor.flush() + writer.flush() then: 2 * api.serializeTrace(_) >> { trace -> callRealMethod() } - 1 * api.sendSerializedTraces(2, _, { it.size() == 2 }) + 1 * api.sendSerializedTraces(2, _, { it.size() == 2 }) >> DDAgentApi.Response.success(200) 0 * _ cleanup: @@ -114,11 +136,8 @@ class DDAgentWriterTest extends DDSpecification { def "test flush by time"() { setup: - def writer = new DDAgentWriter(api) - def phaser = writer.apiPhaser - phaser.register() + def writer = new DDAgentWriter(api, monitor) writer.start() - writer.disruptor.flush() when: (1..5).each { @@ -128,7 +147,14 @@ class DDAgentWriterTest extends DDSpecification { then: 5 * api.serializeTrace(_) >> { trace -> callRealMethod() } - 1 * api.sendSerializedTraces(5, _, { it.size() == 5 }) + 1 * api.sendSerializedTraces(5, _, { it.size() == 5 }) >> { + phaser.arrive() + return DDAgentApi.Response.success(200) + } + 5 * monitor.onPublish(_, _) + 5 * monitor.onSerialize(_, _, _) + 1 * monitor.onFlush(_, _) + (0..1) * monitor.onSend(_, _, _, _) // This gets called after phaser.arrive(), so there's a race condition. 0 * _ cleanup: @@ -153,11 +179,11 @@ class DDAgentWriterTest extends DDSpecification { // Busywait because we don't want to fill up the ring buffer } } - writer.disruptor.flush() + writer.flush() then: (maxedPayloadTraceCount + 1) * api.serializeTrace(_) >> { trace -> callRealMethod() } - 1 * api.sendSerializedTraces(maxedPayloadTraceCount, _, { it.size() == maxedPayloadTraceCount }) + 1 * api.sendSerializedTraces(maxedPayloadTraceCount, _, { it.size() == maxedPayloadTraceCount }) >> DDAgentApi.Response.success(200) cleanup: writer.close() @@ -181,39 +207,43 @@ class DDAgentWriterTest extends DDSpecification { minimalSpan = new DDSpan(0, minimalContext) minimalTrace = [minimalSpan] traceSize = DDAgentApi.OBJECT_MAPPER.writeValueAsBytes(minimalTrace).length - maxedPayloadTraceCount = ((int) (TraceConsumer.FLUSH_PAYLOAD_BYTES / traceSize)) + 1 + maxedPayloadTraceCount = ((int) (BatchWritingDisruptor.FLUSH_PAYLOAD_BYTES / traceSize)) + 1 } def "check that are no interactions after close"() { - setup: - def writer = new DDAgentWriter(api) + def writer = new DDAgentWriter(api, monitor) writer.start() when: writer.close() writer.write([]) - writer.disruptor.flush() + writer.flush() then: +// 2 * monitor.onFlush(_, false) + 1 * monitor.onFailedPublish(_, _) + 1 * monitor.onShutdown(_, _) 0 * _ writer.traceCount.get() == 0 } - def "check shutdown if executor stopped first"() { + def "check shutdown if batchWritingDisruptor stopped first"() { setup: - def writer = new DDAgentWriter(api) + def writer = new DDAgentWriter(api, monitor) writer.start() - writer.scheduledWriterExecutor.shutdown() + writer.batchWritingDisruptor.close() when: writer.write([]) - writer.disruptor.flush() + writer.flush() then: 1 * api.serializeTrace(_) >> { trace -> callRealMethod() } + 1 * monitor.onSerialize(writer, _, _) + 1 * monitor.onPublish(writer, _) 0 * _ - writer.traceCount.get() == 1 + writer.traceCount.get() == 0 cleanup: writer.close() @@ -253,9 +283,7 @@ class DDAgentWriterTest extends DDSpecification { } } } - def api = new DDAgentApi("localhost", agent.address.port, null) - def monitor = Mock(Monitor) - def writer = new DDAgentWriter(api, monitor) + def writer = new DDAgentWriter(DDAgentWriter.Spec.builder().traceAgentPort(agent.address.port).monitor(monitor).build()) when: writer.start() @@ -265,12 +293,12 @@ class DDAgentWriterTest extends DDSpecification { when: writer.write(minimalTrace) - writer.disruptor.flush() + writer.flush() then: 1 * monitor.onPublish(writer, minimalTrace) 1 * monitor.onSerialize(writer, minimalTrace, _) - 1 * monitor.onScheduleFlush(writer, _) + 1 * monitor.onFlush(writer, _) 1 * monitor.onSend(writer, 1, _, { response -> response.success() && response.status() == 200 }) when: @@ -302,9 +330,7 @@ class DDAgentWriterTest extends DDSpecification { } } } - def api = new DDAgentApi("localhost", agent.address.port, null) - def monitor = Mock(Monitor) - def writer = new DDAgentWriter(api, monitor) + def writer = new DDAgentWriter(DDAgentWriter.Spec.builder().traceAgentPort(agent.address.port).monitor(monitor).build()) when: writer.start() @@ -314,12 +340,12 @@ class DDAgentWriterTest extends DDSpecification { when: writer.write(minimalTrace) - writer.disruptor.flush() + writer.flush() then: 1 * monitor.onPublish(writer, minimalTrace) 1 * monitor.onSerialize(writer, minimalTrace, _) - 1 * monitor.onScheduleFlush(writer, _) + 1 * monitor.onFlush(writer, _) 1 * monitor.onFailedSend(writer, 1, _, { response -> !response.success() && response.status() == 500 }) when: @@ -345,7 +371,6 @@ class DDAgentWriterTest extends DDSpecification { return DDAgentApi.Response.failed(new IOException("comm error")) } } - def monitor = Mock(Monitor) def writer = new DDAgentWriter(api, monitor) when: @@ -356,12 +381,12 @@ class DDAgentWriterTest extends DDSpecification { when: writer.write(minimalTrace) - writer.disruptor.flush() + writer.flush() then: 1 * monitor.onPublish(writer, minimalTrace) 1 * monitor.onSerialize(writer, minimalTrace, _) - 1 * monitor.onScheduleFlush(writer, _) + 1 * monitor.onFlush(writer, _) 1 * monitor.onFailedSend(writer, 1, _, { response -> !response.success() && response.status() == null }) when: @@ -400,30 +425,30 @@ class DDAgentWriterTest extends DDSpecification { } } } - def api = new DDAgentApi("localhost", agent.address.port, null) // This test focuses just on failed publish, so not verifying every callback - def monitor = Stub(Monitor) - monitor.onPublish(_, _) >> { - numPublished.incrementAndGet() - } - monitor.onFailedPublish(_, _) >> { - numFailedPublish.incrementAndGet() - } - monitor.onFlush(_, _) >> { - numFlushes.incrementAndGet() - } - monitor.onSend(_, _, _, _) >> { - numRequests.incrementAndGet() - } - monitor.onFailedPublish(_, _, _, _) >> { - numFailedRequests.incrementAndGet() + def monitor = Stub(Monitor) { + onPublish(_, _) >> { + numPublished.incrementAndGet() + } + onFailedPublish(_, _) >> { + numFailedPublish.incrementAndGet() + } + onFlush(_, _) >> { + numFlushes.incrementAndGet() + } + onSend(_, _, _, _) >> { + numRequests.incrementAndGet() + } + onFailedPublish(_, _, _, _) >> { + numFailedRequests.incrementAndGet() + } } // sender queue is sized in requests -- not traces def bufferSize = 32 def senderQueueSize = 2 - def writer = new DDAgentWriter(api, monitor, bufferSize, senderQueueSize, DDAgentWriter.FLUSH_PAYLOAD_DELAY) + def writer = new DDAgentWriter(DDAgentWriter.Spec.builder().traceAgentPort(agent.address.port).monitor(monitor).traceBufferSize(bufferSize).build()) writer.start() // gate responses @@ -438,7 +463,7 @@ class DDAgentWriterTest extends DDSpecification { // sanity check coordination mechanism of test // release to allow response to be generated responseSemaphore.release() - writer.disruptor.flush() + writer.flush() // reacquire semaphore to stall further responses responseSemaphore.acquire() @@ -505,21 +530,21 @@ class DDAgentWriterTest extends DDSpecification { } } } - def api = new DDAgentApi("localhost", agent.address.port, null) // This test focuses just on failed publish, so not verifying every callback - def monitor = Stub(Monitor) - monitor.onPublish(_, _) >> { - numPublished.incrementAndGet() - } - monitor.onFailedPublish(_, _) >> { - numFailedPublish.incrementAndGet() - } - monitor.onSend(_, _, _, _) >> { writer, repCount, sizeInBytes, response -> - numRepSent.addAndGet(repCount) + def monitor = Stub(Monitor) { + onPublish(_, _) >> { + numPublished.incrementAndGet() + } + onFailedPublish(_, _) >> { + numFailedPublish.incrementAndGet() + } + onSend(_, _, _, _) >> { writer, repCount, sizeInBytes, response -> + numRepSent.addAndGet(repCount) + } } - def writer = new DDAgentWriter(api, monitor) + def writer = new DDAgentWriter(DDAgentWriter.Spec.builder().traceAgentPort(agent.address.port).monitor(monitor).build()) writer.start() when: @@ -538,7 +563,7 @@ class DDAgentWriterTest extends DDSpecification { t1.join() t2.join() - writer.disruptor.flush() + writer.flush() then: def totalTraces = 100 + 100 @@ -566,7 +591,6 @@ class DDAgentWriterTest extends DDSpecification { } } } - def api = new DDAgentApi("localhost", agent.address.port, null) def statsd = Stub(StatsDClient) statsd.incrementCounter("queue.accepted") >> { stat -> @@ -580,12 +604,12 @@ class DDAgentWriterTest extends DDSpecification { } def monitor = new Monitor.StatsD(statsd) - def writer = new DDAgentWriter(api, monitor) + def writer = new DDAgentWriter(DDAgentWriter.Spec.builder().traceAgentPort(agent.address.port).monitor(monitor).build()) writer.start() when: writer.write(minimalTrace) - writer.disruptor.flush() + writer.flush() then: numTracesAccepted == 1 @@ -633,7 +657,7 @@ class DDAgentWriterTest extends DDSpecification { when: writer.write(minimalTrace) - writer.disruptor.flush() + writer.flush() then: numRequests == 1 From 5ff855737b4fca280519f5fee0750a075b94f052 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 7 Jan 2020 10:07:23 -0800 Subject: [PATCH 02/17] Add documentation, remove volatile/public, improve test reliability. --- .../trace/common/writer/DDAgentWriter.java | 29 +++++++++++------ .../writer/ddagent/AbstractDisruptor.java | 9 +++++- .../writer/ddagent/BatchWritingDisruptor.java | 11 ++++++- .../common/writer/ddagent/DisruptorEvent.java | 9 +++--- .../ddagent/TraceProcessingDisruptor.java | 9 +++++- .../trace/api/writer/DDAgentWriterTest.groovy | 31 ++++++++++--------- 6 files changed, 67 insertions(+), 31 deletions(-) diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java index 965976d4ba..cf725b365b 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java @@ -16,11 +16,18 @@ import lombok.Value; import lombok.extern.slf4j.Slf4j; /** - * This writer buffers traces and sends them to the provided DDApi instance. + * This writer buffers traces and sends them to the provided DDApi instance. Buffering is done with + * a distruptor to limit blocking the application threads. Internally, the trace is serialized and + * put onto a separate disruptor that does block to decouple the CPU intensive from the IO bound + * threads. * - *

Written traces are passed off to a disruptor so as to avoid blocking the application's thread. - * If a flood of traces arrives that exceeds the disruptor ring size, the traces exceeding the - * threshold will be counted and sampled. + *

[Application] -> [trace processing buffer] -> [serialized trace batching buffer] -> [dd-agent] + * + *

Note: the first buffer is non-blocking and will discard if full, the second is blocking and + * will cause back pressure on the trace processing (serializing) thread. + * + *

If the buffer is filled traces are discarded before serializing. Once serialized every effort + * is made to keep, to avoid wasting the serialization effort. */ @Slf4j public class DDAgentWriter implements Writer { @@ -38,8 +45,8 @@ public class DDAgentWriter implements Writer { private static final int DISRUPTOR_BUFFER_SIZE = 1024; private final DDAgentApi api; - public final TraceProcessingDisruptor traceProcessingDisruptor; - public final BatchWritingDisruptor batchWritingDisruptor; + private final TraceProcessingDisruptor traceProcessingDisruptor; + private final BatchWritingDisruptor batchWritingDisruptor; private final AtomicInteger traceCount = new AtomicInteger(0); @@ -147,9 +154,13 @@ public class DDAgentWriter implements Writer { @Override public void close() { - monitor.onShutdown(this, traceProcessingDisruptor.flush(traceCount.getAndSet(0))); - traceProcessingDisruptor.close(); - batchWritingDisruptor.close(); + final boolean flushSuccess = traceProcessingDisruptor.flush(traceCount.getAndSet(0)); + try { + traceProcessingDisruptor.close(); + } finally { // in case first close fails. + batchWritingDisruptor.close(); + } + monitor.onShutdown(this, flushSuccess); } @Override diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/AbstractDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/AbstractDisruptor.java index 2bff4028ac..86c7fca315 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/AbstractDisruptor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/AbstractDisruptor.java @@ -11,7 +11,7 @@ import java.util.concurrent.TimeUnit; import lombok.extern.slf4j.Slf4j; @Slf4j -public abstract class AbstractDisruptor implements Closeable { +abstract class AbstractDisruptor implements Closeable { protected final Disruptor> disruptor; @@ -46,6 +46,13 @@ public abstract class AbstractDisruptor implements Closeable { disruptor.shutdown(); } + /** + * Allows the underlying publish to be defined as a blocking or non blocking call. + * + * @param data + * @param representativeCount + * @return + */ public abstract boolean publish(final T data, int representativeCount); /** diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java index fc08591548..b02f2d2e25 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java @@ -12,10 +12,16 @@ import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import lombok.extern.slf4j.Slf4j; +/** + * Disruptor that takes serialized traces and batches them into appropriately sized requests. + * + *

publishing to the buffer will block if the buffer is full. + */ @Slf4j public class BatchWritingDisruptor extends AbstractDisruptor { private static final int FLUSH_PAYLOAD_BYTES = 5_000_000; // 5 MB + // TODO: move executor to tracer for sharing with other tasks. private final ScheduledExecutorService heartbeatExecutor = Executors.newScheduledThreadPool(1, new DaemonThreadFactory("dd-trace-heartbeat")); @@ -53,6 +59,7 @@ public class BatchWritingDisruptor extends AbstractDisruptor { @Override public boolean publish(final byte[] data, final int representativeCount) { + // blocking call to ensure serialized traces aren't discarded and apply back pressure. disruptor.getRingBuffer().publishEvent(dataTranslator, data, representativeCount); return true; } @@ -81,6 +88,7 @@ public class BatchWritingDisruptor extends AbstractDisruptor { this.writer = writer; } + // TODO: reduce byte[] garbage by keeping the byte[] on the event and copy before returning. @Override public void onEvent( final DisruptorEvent event, final long sequence, final boolean endOfBatch) { @@ -111,11 +119,12 @@ public class BatchWritingDisruptor extends AbstractDisruptor { return; } - monitor.onFlush(writer, early); // TODO add retry and rate limiting final DDAgentApi.Response response = api.sendSerializedTraces(representativeCount, sizeInBytes, serializedTraces); + monitor.onFlush(writer, early); + if (response.success()) { log.debug("Successfully sent {} traces to the API", serializedTraces.size()); diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DisruptorEvent.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DisruptorEvent.java index 8e754fa3fe..65cb6db45c 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DisruptorEvent.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DisruptorEvent.java @@ -6,11 +6,12 @@ import com.lmax.disruptor.EventTranslatorTwoArg; import java.util.concurrent.CountDownLatch; class DisruptorEvent { - public volatile T data = null; - public volatile int representativeCount = 0; - public volatile CountDownLatch flushLatch = null; + // Memory ordering enforced by disruptor's memory fences, so volatile not required. + T data = null; + int representativeCount = 0; + CountDownLatch flushLatch = null; - public void reset() { + void reset() { data = null; representativeCount = 0; flushLatch = null; diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java index 0088227be7..e0cdf3dc37 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java @@ -9,6 +9,13 @@ import java.util.List; import java.util.concurrent.ThreadFactory; import lombok.extern.slf4j.Slf4j; +/** + * Disruptor that takes completed traces and applies processing to them. Upon completion, the + * serialized trace is published to {@link BatchWritingDisruptor}. + * + *

publishing to the buffer will not block the calling thread, but instead will return false if + * the buffer is full. This is to avoid impacting an application thread. + */ @Slf4j public class TraceProcessingDisruptor extends AbstractDisruptor> { @@ -58,8 +65,8 @@ public class TraceProcessingDisruptor extends AbstractDisruptor> { if (event.data != null) { try { final byte[] serializedTrace = api.serializeTrace(event.data); - monitor.onSerialize(writer, event.data, serializedTrace); batchWritingDisruptor.publish(serializedTrace, event.representativeCount); + monitor.onSerialize(writer, event.data, serializedTrace); event.representativeCount = 0; // reset in case flush is invoked below. } catch (final JsonProcessingException e) { log.debug("Error serializing trace", e); diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy index d954fe6594..e05a99dd38 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy @@ -11,6 +11,7 @@ import datadog.trace.common.writer.ddagent.BatchWritingDisruptor import datadog.trace.common.writer.ddagent.DDAgentApi import datadog.trace.common.writer.ddagent.Monitor import datadog.trace.util.test.DDSpecification +import spock.lang.Retry import spock.lang.Timeout import java.util.concurrent.Phaser @@ -396,6 +397,8 @@ class DDAgentWriterTest extends DDSpecification { 1 * monitor.onShutdown(writer, true) } + @Retry(delay = 10) + // if execution is too slow, the http client timeout may trigger. def "slow response test"() { def numWritten = 0 def numFlushes = new AtomicInteger(0) @@ -407,7 +410,6 @@ class DDAgentWriterTest extends DDSpecification { def responseSemaphore = new Semaphore(1) setup: - def minimalTrace = createMinimalTrace() // Need to set-up a dummy agent for the final send callback to work def agent = httpServer { @@ -445,9 +447,6 @@ class DDAgentWriterTest extends DDSpecification { } } - // sender queue is sized in requests -- not traces - def bufferSize = 32 - def senderQueueSize = 2 def writer = new DDAgentWriter(DDAgentWriter.Spec.builder().traceAgentPort(agent.address.port).monitor(monitor).traceBufferSize(bufferSize).build()) writer.start() @@ -477,13 +476,10 @@ class DDAgentWriterTest extends DDSpecification { when: // send many traces to fill the sender queue... // loop until outstanding requests > finished requests - while (numFlushes.get() - (numRequests.get() + numFailedRequests.get()) < senderQueueSize) { - // chunk the loop & wait to allow for flushing to send queue - (1..1_000).each { - writer.write(minimalTrace) - numWritten += 1 - } - Thread.sleep(100) + while (writer.traceProcessingDisruptor.disruptorRemainingCapacity + writer.batchWritingDisruptor.disruptorRemainingCapacity > 0 || numFailedPublish.get() == 0) { + writer.write(minimalTrace) + numWritten += 1 + Thread.sleep(1) // Allow traces to get serialized. } then: @@ -494,17 +490,18 @@ class DDAgentWriterTest extends DDSpecification { def priorNumFailed = numFailedPublish.get() // with both disruptor & queue full, should reject everything - def expectedRejects = 100_000 + def expectedRejects = 100 (1..expectedRejects).each { writer.write(minimalTrace) numWritten += 1 } then: - // If the in-flight requests timeouts and frees up a slot in the sending queue, then - // many of traces will be accepted and batched into a new failing request. + // If the in-flight request times out (we don't currently retry), + // then a new batch will begin processing and many of traces will + // be accepted and batched into a new failing request. // In that case, the reject number will be low. - numFailedPublish.get() - priorNumFailed > expectedRejects * 0.40 + numFailedPublish.get() - priorNumFailed >= expectedRejects * 0.80 numPublished.get() + numFailedPublish.get() == numWritten cleanup: @@ -512,6 +509,10 @@ class DDAgentWriterTest extends DDSpecification { writer.close() agent.close() + + where: + bufferSize = 16 + minimalTrace = createMinimalTrace() } def "multi threaded"() { From a4db31cf79a4fc8ec5bf1ad7ba9d44988daf7383 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Mon, 13 Jan 2020 15:44:51 -0800 Subject: [PATCH 03/17] Apply `_sample_rate` metric to allow dd-agent to do proper scaling of metrics when traces are sampled. --- .../java/datadog/trace/common/writer/DDAgentWriter.java | 8 +++++++- .../common/writer/ddagent/TraceProcessingDisruptor.java | 6 ++++++ .../datadog/trace/api/writer/DDAgentWriterTest.groovy | 4 ++-- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java index cf725b365b..81b884e43d 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java @@ -113,7 +113,13 @@ public class DDAgentWriter implements Writer { public void write(final List trace) { // We can't add events after shutdown otherwise it will never complete shutting down. if (traceProcessingDisruptor.running) { - final int representativeCount = traceCount.getAndSet(0) + 1; + final int representativeCount; + if (trace.isEmpty() || !(trace.get(0).isRootSpan())) { + // We don't want to reset the count if we can't correctly report the value. + representativeCount = 1; + } else { + representativeCount = traceCount.getAndSet(0) + 1; + } final boolean published = traceProcessingDisruptor.publish(trace, representativeCount); if (published) { diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java index e0cdf3dc37..e1a62bafed 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java @@ -63,6 +63,12 @@ public class TraceProcessingDisruptor extends AbstractDisruptor> { final DisruptorEvent> event, final long sequence, final boolean endOfBatch) { try { if (event.data != null) { + if (1 < event.representativeCount && !event.data.isEmpty()) { + // attempt to have agent scale the metrics properly + ((DDSpan) event.data.get(0).getLocalRootSpan()) + .context() + .setMetric("_sample_rate", 1d / event.representativeCount); + } try { final byte[] serializedTrace = api.serializeTrace(event.data); batchWritingDisruptor.publish(serializedTrace, event.representativeCount); diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy index e05a99dd38..24340e32f9 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy @@ -131,7 +131,7 @@ class DDAgentWriterTest extends DDSpecification { writer.close() where: - span = [newSpanOf(0, "fixed-thread-name")] + span = newSpanOf(0, "fixed-thread-name") trace = (0..10000).collect { span } } @@ -162,7 +162,7 @@ class DDAgentWriterTest extends DDSpecification { writer.close() where: - span = [newSpanOf(0, "fixed-thread-name")] + span = newSpanOf(0, "fixed-thread-name") trace = (1..10).collect { span } } From 3aea763769557b2f75da901cf34af1308927495f Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Mon, 13 Jan 2020 16:21:37 -0800 Subject: [PATCH 04/17] Remove test race condition --- .../datadog/trace/api/writer/DDAgentWriterTest.groovy | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy index 24340e32f9..23e3e691ef 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy @@ -148,14 +148,13 @@ class DDAgentWriterTest extends DDSpecification { then: 5 * api.serializeTrace(_) >> { trace -> callRealMethod() } - 1 * api.sendSerializedTraces(5, _, { it.size() == 5 }) >> { - phaser.arrive() - return DDAgentApi.Response.success(200) - } + 1 * api.sendSerializedTraces(5, _, { it.size() == 5 }) >> DDAgentApi.Response.success(200) 5 * monitor.onPublish(_, _) 5 * monitor.onSerialize(_, _, _) 1 * monitor.onFlush(_, _) - (0..1) * monitor.onSend(_, _, _, _) // This gets called after phaser.arrive(), so there's a race condition. + 1 * monitor.onSend(_, _, _, _) >> { + phaser.arrive() + } 0 * _ cleanup: From 5cce4cb783edb1b766f6c22be4b1701739dcc9ae Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 16 Jan 2020 16:43:01 -0800 Subject: [PATCH 05/17] Replace DDAgentWriter.Spec with a proper Builder. Also rename the builder class on DDTracer to default name generated by Lombok. --- dd-trace-ot/dd-trace-ot.gradle | 3 +- .../java/datadog/opentracing/DDTracer.java | 11 ++-- .../trace/common/writer/DDAgentWriter.java | 61 ++++++++++--------- .../datadog/trace/common/writer/Writer.java | 8 ++- .../trace/api/writer/DDAgentWriterTest.groovy | 28 ++++----- 5 files changed, 57 insertions(+), 54 deletions(-) diff --git a/dd-trace-ot/dd-trace-ot.gradle b/dd-trace-ot/dd-trace-ot.gradle index 96f9a4bd6d..cf92790fcb 100644 --- a/dd-trace-ot/dd-trace-ot.gradle +++ b/dd-trace-ot/dd-trace-ot.gradle @@ -12,8 +12,7 @@ minimumInstructionCoverage = 0.6 excludedClassesCoverage += [ 'datadog.trace.common.writer.ListWriter', 'datadog.trace.common.writer.LoggingWriter', - 'datadog.trace.common.writer.DDAgentWriter.Spec', - 'datadog.trace.common.writer.DDAgentWriter.Spec.SpecBuilder', + 'datadog.trace.common.writer.DDAgentWriter.DDAgentWriterBuilder', 'datadog.trace.common.sampling.PrioritySampling', // This code is copied from okHttp samples and we have integration tests to verify that it works. 'datadog.trace.common.writer.unixdomainsockets.TunnelingUnixSocket', diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java index 7f182b5aec..7312fa398c 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java @@ -44,6 +44,7 @@ import java.util.SortedSet; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.ThreadLocalRandom; +import lombok.Builder; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -96,18 +97,18 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace private final HttpCodec.Injector injector; private final HttpCodec.Extractor extractor; - public static class Builder { + public static class DDTracerBuilder { - public Builder() { + public DDTracerBuilder() { // Apply the default values from config. config(Config.get()); } - public Builder withProperties(final Properties properties) { + public DDTracerBuilder withProperties(final Properties properties) { return config(Config.get(properties)); } - public Builder config(final Config config) { + public DDTracerBuilder config(final Config config) { this.config = config; serviceName(config.getServiceName()); // Explicitly skip setting writer to avoid allocating resources prematurely. @@ -265,7 +266,7 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace partialFlushMinSpans); } - @lombok.Builder(builderClassName = "Builder") + @Builder // These field names must be stable to ensure the builder api is stable. private DDTracer( final Config config, diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java index 81b884e43d..44e28da917 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java @@ -12,7 +12,6 @@ import datadog.trace.common.writer.ddagent.Monitor; import datadog.trace.common.writer.ddagent.TraceProcessingDisruptor; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; -import lombok.Value; import lombok.extern.slf4j.Slf4j; /** @@ -31,16 +30,6 @@ import lombok.extern.slf4j.Slf4j; */ @Slf4j public class DDAgentWriter implements Writer { - @Value - @lombok.Builder - public static class Spec { - @lombok.Builder.Default public String agentHost = DEFAULT_AGENT_HOST; - @lombok.Builder.Default public int traceAgentPort = DEFAULT_TRACE_AGENT_PORT; - @lombok.Builder.Default public String unixDomainSocket = DEFAULT_AGENT_UNIX_DOMAIN_SOCKET; - @lombok.Builder.Default public int traceBufferSize = DISRUPTOR_BUFFER_SIZE; - @lombok.Builder.Default public Monitor monitor = new Monitor.Noop(); - @lombok.Builder.Default public int flushFrequencySeconds = 1; - } private static final int DISRUPTOR_BUFFER_SIZE = 1024; @@ -52,20 +41,22 @@ public class DDAgentWriter implements Writer { public final Monitor monitor; - public DDAgentWriter() { - this(Spec.builder().build()); + // Apply defaults to the class generated by lombok. + public static class DDAgentWriterBuilder { + String agentHost = DEFAULT_AGENT_HOST; + int traceAgentPort = DEFAULT_TRACE_AGENT_PORT; + String unixDomainSocket = DEFAULT_AGENT_UNIX_DOMAIN_SOCKET; + int traceBufferSize = DISRUPTOR_BUFFER_SIZE; + Monitor monitor = new Monitor.Noop(); + int flushFrequencySeconds = 1; } - public DDAgentWriter(final Spec spec) { - api = new DDAgentApi(spec.agentHost, spec.traceAgentPort, spec.unixDomainSocket); - monitor = spec.monitor; - - batchWritingDisruptor = - new BatchWritingDisruptor( - spec.traceBufferSize, spec.flushFrequencySeconds, api, monitor, this); - traceProcessingDisruptor = - new TraceProcessingDisruptor( - spec.traceBufferSize, api, batchWritingDisruptor, monitor, this); + @Deprecated + public DDAgentWriter() { + this( + new DDAgentApi( + DEFAULT_AGENT_HOST, DEFAULT_TRACE_AGENT_PORT, DEFAULT_AGENT_UNIX_DOMAIN_SOCKET), + new Monitor.Noop()); } @Deprecated @@ -79,17 +70,27 @@ public class DDAgentWriter implements Writer { DISRUPTOR_BUFFER_SIZE, api, batchWritingDisruptor, monitor, this); } - @Deprecated - // DQH - TODO - Update the tests & remove this + @lombok.Builder + // These field names must be stable to ensure the builder api is stable. private DDAgentWriter( - final DDAgentApi api, final int disruptorSize, final int flushFrequencySeconds) { - this.api = api; - monitor = new Monitor.Noop(); + final DDAgentApi agentApi, + final String agentHost, + final int traceAgentPort, + final String unixDomainSocket, + final int traceBufferSize, + final Monitor monitor, + final int flushFrequencySeconds) { + if (agentApi != null) { + api = agentApi; + } else { + api = new DDAgentApi(agentHost, traceAgentPort, unixDomainSocket); + } + this.monitor = monitor; batchWritingDisruptor = - new BatchWritingDisruptor(disruptorSize, flushFrequencySeconds, api, monitor, this); + new BatchWritingDisruptor(traceBufferSize, flushFrequencySeconds, api, monitor, this); traceProcessingDisruptor = - new TraceProcessingDisruptor(disruptorSize, api, batchWritingDisruptor, monitor, this); + new TraceProcessingDisruptor(traceBufferSize, api, batchWritingDisruptor, monitor, this); } public void addResponseListener(final DDAgentResponseListener listener) { diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/Writer.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/Writer.java index 51e0e85c4b..190c75d0e4 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/Writer.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/Writer.java @@ -53,7 +53,7 @@ public interface Writer extends Closeable { } else { log.warn( "Writer type not configured correctly: No config provided! Defaulting to DDAgentWriter."); - writer = new DDAgentWriter(); + writer = DDAgentWriter.builder().build(); } return writer; @@ -64,8 +64,10 @@ public interface Writer extends Closeable { } private static Writer createAgentWriter(final Config config) { - // TODO: switch to using DDAgentWriter.Spec constructor... - return new DDAgentWriter(createApi(config), createMonitor(config)); + return DDAgentWriter.builder() + .agentApi(createApi(config)) + .monitor(createMonitor(config)) + .build(); } private static DDAgentApi createApi(final Config config) { diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy index 23e3e691ef..46f65e7c0e 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy @@ -44,7 +44,7 @@ class DDAgentWriterTest extends DDSpecification { def "test happy path"() { setup: - def writer = new DDAgentWriter(api, 2, -1) + def writer = DDAgentWriter.builder().agentApi(api).traceBufferSize(2).flushFrequencySeconds(-1).build() writer.start() when: @@ -72,7 +72,7 @@ class DDAgentWriterTest extends DDSpecification { def "test flood of traces"() { setup: - def writer = new DDAgentWriter(api, disruptorSize, -1) + def writer = DDAgentWriter.builder().agentApi(api).traceBufferSize(disruptorSize).flushFrequencySeconds(-1).build() writer.start() when: @@ -97,7 +97,7 @@ class DDAgentWriterTest extends DDSpecification { def "test flush by size"() { setup: - def writer = new DDAgentWriter(api, DISRUPTOR_BUFFER_SIZE, -1) + def writer = DDAgentWriter.builder().agentApi(api).traceBufferSize(DISRUPTOR_BUFFER_SIZE).flushFrequencySeconds(-1).build() writer.start() when: @@ -137,7 +137,7 @@ class DDAgentWriterTest extends DDSpecification { def "test flush by time"() { setup: - def writer = new DDAgentWriter(api, monitor) + def writer = DDAgentWriter.builder().agentApi(api).monitor(monitor).build() writer.start() when: @@ -167,7 +167,7 @@ class DDAgentWriterTest extends DDSpecification { def "test default buffer size"() { setup: - def writer = new DDAgentWriter(api, DISRUPTOR_BUFFER_SIZE, -1) + def writer = DDAgentWriter.builder().agentApi(api).traceBufferSize(DISRUPTOR_BUFFER_SIZE).flushFrequencySeconds(-1).build() writer.start() when: @@ -212,7 +212,7 @@ class DDAgentWriterTest extends DDSpecification { def "check that are no interactions after close"() { setup: - def writer = new DDAgentWriter(api, monitor) + def writer = DDAgentWriter.builder().agentApi(api).monitor(monitor).build() writer.start() when: @@ -230,7 +230,7 @@ class DDAgentWriterTest extends DDSpecification { def "check shutdown if batchWritingDisruptor stopped first"() { setup: - def writer = new DDAgentWriter(api, monitor) + def writer = DDAgentWriter.builder().agentApi(api).monitor(monitor).build() writer.start() writer.batchWritingDisruptor.close() @@ -283,7 +283,7 @@ class DDAgentWriterTest extends DDSpecification { } } } - def writer = new DDAgentWriter(DDAgentWriter.Spec.builder().traceAgentPort(agent.address.port).monitor(monitor).build()) + def writer = DDAgentWriter.builder().traceAgentPort(agent.address.port).monitor(monitor).build() when: writer.start() @@ -330,7 +330,7 @@ class DDAgentWriterTest extends DDSpecification { } } } - def writer = new DDAgentWriter(DDAgentWriter.Spec.builder().traceAgentPort(agent.address.port).monitor(monitor).build()) + def writer = DDAgentWriter.builder().traceAgentPort(agent.address.port).monitor(monitor).build() when: writer.start() @@ -371,7 +371,7 @@ class DDAgentWriterTest extends DDSpecification { return DDAgentApi.Response.failed(new IOException("comm error")) } } - def writer = new DDAgentWriter(api, monitor) + def writer = DDAgentWriter.builder().agentApi(api).monitor(monitor).build() when: writer.start() @@ -446,7 +446,7 @@ class DDAgentWriterTest extends DDSpecification { } } - def writer = new DDAgentWriter(DDAgentWriter.Spec.builder().traceAgentPort(agent.address.port).monitor(monitor).traceBufferSize(bufferSize).build()) + def writer = DDAgentWriter.builder().traceAgentPort(agent.address.port).monitor(monitor).traceBufferSize(bufferSize).build() writer.start() // gate responses @@ -544,7 +544,7 @@ class DDAgentWriterTest extends DDSpecification { } } - def writer = new DDAgentWriter(DDAgentWriter.Spec.builder().traceAgentPort(agent.address.port).monitor(monitor).build()) + def writer = DDAgentWriter.builder().traceAgentPort(agent.address.port).monitor(monitor).build() writer.start() when: @@ -604,7 +604,7 @@ class DDAgentWriterTest extends DDSpecification { } def monitor = new Monitor.StatsD(statsd) - def writer = new DDAgentWriter(DDAgentWriter.Spec.builder().traceAgentPort(agent.address.port).monitor(monitor).build()) + def writer = DDAgentWriter.builder().traceAgentPort(agent.address.port).monitor(monitor).build() writer.start() when: @@ -652,7 +652,7 @@ class DDAgentWriterTest extends DDSpecification { } def monitor = new Monitor.StatsD(statsd) - def writer = new DDAgentWriter(api, monitor) + def writer = DDAgentWriter.builder().agentApi(api).monitor(monitor).build() writer.start() when: From 4e58643bd07105b6fee12ca4afdf4be91fd1567f Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Wed, 22 Jan 2020 18:09:10 -0500 Subject: [PATCH 06/17] Initial finatra instrumentation --- .../instrumentation/finatra/finatra.gradle | 42 ++++++ .../finatra/FinatraDecorator.java | 51 +++++++ .../finatra/FinatraInstrumentation.java | 142 ++++++++++++++++++ .../ExecutorInstrumentationUtils.java | 20 ++- settings.gradle | 1 + 5 files changed, 251 insertions(+), 5 deletions(-) create mode 100644 dd-java-agent/instrumentation/finatra/finatra.gradle create mode 100644 dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java create mode 100644 dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java diff --git a/dd-java-agent/instrumentation/finatra/finatra.gradle b/dd-java-agent/instrumentation/finatra/finatra.gradle new file mode 100644 index 0000000000..f85086a2c6 --- /dev/null +++ b/dd-java-agent/instrumentation/finatra/finatra.gradle @@ -0,0 +1,42 @@ +// Set properties before any plugins get loaded +ext { + minJavaVersionForTests = JavaVersion.VERSION_1_8 +} + +apply from: "${rootDir}/gradle/java.gradle" +apply from: "${rootDir}/gradle/test-with-scala.gradle" + +// apply plugin: 'org.unbroken-dome.test-sets' +//testSets { +// +// latestDepTest { +// dirName = 'test' +// } +//} + +muzzle { + +} + +dependencies { + compileOnly group: 'com.twitter', name: 'finagle-http_2.11', version: '19.1.0' + compileOnly group: 'com.twitter', name: 'finatra-http_2.11', version: '19.1.0' + + testCompile project(':dd-java-agent:instrumentation:netty-4.1') + testCompile project(':dd-java-agent:instrumentation:java-concurrent') + testCompile project(':dd-java-agent:instrumentation:trace-annotation') + + testCompile group: 'com.twitter', name: 'finagle-http_2.11', version: '19.1.0' + + testCompile(group: 'com.fasterxml.jackson.module', name: 'jackson-module-scala_2.11', version: '2.10.2') { + force = true + } + +// latestDepTestCompile project(':dd-java-agent:instrumentation:java-concurrent') + // latestDepTestCompile project(':dd-java-agent:instrumentation:trace-annotation') +} + +//compileLatestDepTestGroovy { +// classpath = classpath.plus(files(compileLatestDepTestScala.destinationDir)) +// dependsOn compileLatestDepTestScala +//} diff --git a/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java b/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java new file mode 100644 index 0000000000..3d8cd1d1df --- /dev/null +++ b/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java @@ -0,0 +1,51 @@ +package datadog.trace.instrumentation.finatra; + +import com.twitter.finagle.http.Request; +import com.twitter.finagle.http.Response; +import datadog.trace.agent.decorator.HttpServerDecorator; +import java.net.URI; +import java.net.URISyntaxException; + +public class FinatraDecorator extends HttpServerDecorator { + public static final FinatraDecorator DECORATE = new FinatraDecorator(); + + @Override + protected String component() { + return "finatra"; + } + + @Override + protected String method(final Request request) { + return request.method().name(); + } + + @Override + protected URI url(final Request request) throws URISyntaxException { + return URI.create(request.uri()); + } + + @Override + protected String peerHostname(final Request request) { + return request.remoteHost(); + } + + @Override + protected String peerHostIP(final Request request) { + return request.remoteAddress().getHostAddress(); + } + + @Override + protected Integer peerPort(final Request request) { + return request.remotePort(); + } + + @Override + protected Integer status(final Response response) { + return response.statusCode(); + } + + @Override + protected String[] instrumentationNames() { + return new String[] {"finatra"}; + } +} diff --git a/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java b/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java new file mode 100644 index 0000000000..2911bd58bf --- /dev/null +++ b/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java @@ -0,0 +1,142 @@ +package datadog.trace.instrumentation.finatra; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; +import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; +import static datadog.trace.instrumentation.api.AgentTracer.startSpan; +import static datadog.trace.instrumentation.finatra.FinatraDecorator.DECORATE; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isInterface; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; + +import com.google.auto.service.AutoService; +import com.twitter.finagle.http.Request; +import com.twitter.finagle.http.Response; +import com.twitter.util.Future; +import com.twitter.util.FutureEventListener; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.api.DDTags; +import datadog.trace.instrumentation.api.AgentScope; +import datadog.trace.instrumentation.api.AgentSpan; +import datadog.trace.instrumentation.api.Tags; +import java.lang.reflect.Method; +import java.util.Map; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; +import scala.Some; + +@AutoService(Instrumenter.class) +public class FinatraInstrumentation extends Instrumenter.Default { + public FinatraInstrumentation() { + super("finatra"); + } + + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.agent.decorator.BaseDecorator", + "datadog.trace.agent.decorator.ServerDecorator", + "datadog.trace.agent.decorator.HttpServerDecorator", + packageName + ".FinatraDecorator", + FinatraInstrumentation.class.getName() + "$Listener" + }; + } + + @Override + public ElementMatcher typeMatcher() { + return not(isInterface()) + .and(safeHasSuperType(named("com.twitter.finatra.http.internal.routing.Route"))); + } + + @Override + public Map, String> transformers() { + return singletonMap( + isMethod() + .and(named("handleMatch")) + .and(takesArguments(2)) + .and(takesArgument(0, named("com.twitter.finagle.http.Request"))), + FinatraInstrumentation.class.getName() + "$RouteAdvice"); + } + + public static class RouteAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static AgentScope nameSpan( + @Advice.Argument(0) final Request request, + @Advice.FieldValue("path") final String path, + @Advice.FieldValue("clazz") final Class clazz, + @Advice.Origin final Method method) { + + final AgentSpan parent = activeSpan(); + + final AgentSpan span = startSpan("finatra.request"); + DECORATE.afterStart(span); + DECORATE.onConnection(span, request); + DECORATE.onRequest(span, request); + + if (parent != null && "netty.request".equals(parent.getSpanName())) { + parent.setTag(DDTags.RESOURCE_NAME, request.method().name() + " " + path); + parent.setTag(Tags.COMPONENT, "finatra"); + parent.setSpanName("finatra.request"); + + span.setSpanName("finatra.controller"); + span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForClass(clazz)); + } + + final AgentScope scope = activateSpan(span, false); + scope.setAsyncPropagation(true); + return scope; + } + + @Advice.OnMethodExit(onThrowable = Throwable.class) + public static void setupCallback( + @Advice.Enter final AgentScope scope, + @Advice.Thrown final Throwable throwable, + @Advice.Return final Some> responseOption) { + + if (scope == null) { + return; + } + + final AgentSpan span = scope.span(); + if (throwable != null) { + DECORATE.onError(span, throwable); + DECORATE.beforeFinish(span); + span.finish(); + scope.close(); + return; + } + + responseOption.get().addEventListener(new Listener(scope)); + } + } + + public static class Listener implements FutureEventListener { + private final AgentScope scope; + + public Listener(final AgentScope scope) { + this.scope = scope; + } + + @Override + public void onSuccess(final Response response) { + DECORATE.onResponse(scope.span(), response); + DECORATE.beforeFinish(scope.span()); + scope.span().finish(); + scope.close(); + } + + @Override + public void onFailure(final Throwable cause) { + DECORATE.onError(scope.span(), cause); + DECORATE.beforeFinish(scope.span()); + scope.span().finish(); + scope.close(); + } + } +} diff --git a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java index a7ca0eaf54..6967415e8d 100644 --- a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java +++ b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java @@ -44,12 +44,22 @@ public class ExecutorInstrumentationUtils { */ public static State setupState( final ContextStore contextStore, final T task, final TraceScope scope) { + final State state = contextStore.putIfAbsent(task, State.FACTORY); - final TraceScope.Continuation continuation = scope.capture(); - if (state.setContinuation(continuation)) { - log.debug("created continuation {} from scope {}, state: {}", continuation, scope, state); - } else { - continuation.close(false); + + // Don't instrument the executor's own runnables. These runnables may never return until + // netty shuts down. Any created continuations will be open until that time preventing traces + // from being reported + if (!task.getClass() + .getName() + .startsWith("io.netty.util.concurrent.SingleThreadEventExecutor$")) { + + final TraceScope.Continuation continuation = scope.capture(); + if (state.setContinuation(continuation)) { + log.debug("created continuation {} from scope {}, state: {}", continuation, scope, state); + } else { + continuation.close(false); + } } return state; } diff --git a/settings.gradle b/settings.gradle index 8a982d5967..0914d6bd70 100644 --- a/settings.gradle +++ b/settings.gradle @@ -60,6 +60,7 @@ include ':dd-java-agent:instrumentation:elasticsearch:transport-2' include ':dd-java-agent:instrumentation:elasticsearch:transport-5' include ':dd-java-agent:instrumentation:elasticsearch:transport-5.3' include ':dd-java-agent:instrumentation:elasticsearch:transport-6' +include ':dd-java-agent:instrumentation:finatra' include ':dd-java-agent:instrumentation:glassfish' include ':dd-java-agent:instrumentation:google-http-client' include ':dd-java-agent:instrumentation:grizzly-2' From 68e52497d65503f70c5b0fe311fd2a54a51b2c22 Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Thu, 23 Jan 2020 10:51:38 -0500 Subject: [PATCH 07/17] Move to versioned folder name --- .../finatra-2.9/finatra-2.9.gradle | 55 +++++++++++++++++++ .../finatra/FinatraDecorator.java | 0 .../finatra/FinatraInstrumentation.java | 0 .../instrumentation/finatra/finatra.gradle | 42 -------------- settings.gradle | 2 +- 5 files changed, 56 insertions(+), 43 deletions(-) create mode 100644 dd-java-agent/instrumentation/finatra-2.9/finatra-2.9.gradle rename dd-java-agent/instrumentation/{finatra => finatra-2.9}/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java (100%) rename dd-java-agent/instrumentation/{finatra => finatra-2.9}/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java (100%) delete mode 100644 dd-java-agent/instrumentation/finatra/finatra.gradle diff --git a/dd-java-agent/instrumentation/finatra-2.9/finatra-2.9.gradle b/dd-java-agent/instrumentation/finatra-2.9/finatra-2.9.gradle new file mode 100644 index 0000000000..b856b7d3ee --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/finatra-2.9.gradle @@ -0,0 +1,55 @@ +// Set properties before any plugins get loaded +ext { + minJavaVersionForTests = JavaVersion.VERSION_1_8 +} + +apply from: "${rootDir}/gradle/java.gradle" +apply from: "${rootDir}/gradle/test-with-scala.gradle" + +apply plugin: 'org.unbroken-dome.test-sets' + +testSets { + + latestDepTest { + dirName = 'test' + } +} + +muzzle { + // There are some weird library issues below 2.9 so can't assert inverse + pass { + group = 'com.twitter' + module = 'finatra-http_2.11' + versions = '[2.9.0,]' + } + + pass { + group = 'com.twitter' + module = 'finatra-http_2.12' + versions = '[2.9.0,]' + } +} + +dependencies { + compileOnly group: 'com.twitter', name: 'finatra-http_2.11', version: '2.9.0' + + testCompile project(':dd-java-agent:instrumentation:netty-4.1') + testCompile project(':dd-java-agent:instrumentation:java-concurrent') + + testCompile group: 'com.twitter', name: 'finatra-http_2.11', version: '19.12.0' + testCompile(group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: '2.9.10') { + force = true + } + + // Required for older versions of finatra on JDKs >= 11 + testCompile group: 'com.sun.activation', name: 'javax.activation', version: '1.2.0' + + latestDepTestCompile project(':dd-java-agent:instrumentation:netty-4.1') + latestDepTestCompile project(':dd-java-agent:instrumentation:java-concurrent') + latestDepTestCompile group: 'com.twitter', name: 'finatra-http_2.11', version: '+' +} + +compileLatestDepTestGroovy { + classpath = classpath.plus(files(compileLatestDepTestScala.destinationDir)) + dependsOn compileLatestDepTestScala +} diff --git a/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java similarity index 100% rename from dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java rename to dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java diff --git a/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java similarity index 100% rename from dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java rename to dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java diff --git a/dd-java-agent/instrumentation/finatra/finatra.gradle b/dd-java-agent/instrumentation/finatra/finatra.gradle deleted file mode 100644 index f85086a2c6..0000000000 --- a/dd-java-agent/instrumentation/finatra/finatra.gradle +++ /dev/null @@ -1,42 +0,0 @@ -// Set properties before any plugins get loaded -ext { - minJavaVersionForTests = JavaVersion.VERSION_1_8 -} - -apply from: "${rootDir}/gradle/java.gradle" -apply from: "${rootDir}/gradle/test-with-scala.gradle" - -// apply plugin: 'org.unbroken-dome.test-sets' -//testSets { -// -// latestDepTest { -// dirName = 'test' -// } -//} - -muzzle { - -} - -dependencies { - compileOnly group: 'com.twitter', name: 'finagle-http_2.11', version: '19.1.0' - compileOnly group: 'com.twitter', name: 'finatra-http_2.11', version: '19.1.0' - - testCompile project(':dd-java-agent:instrumentation:netty-4.1') - testCompile project(':dd-java-agent:instrumentation:java-concurrent') - testCompile project(':dd-java-agent:instrumentation:trace-annotation') - - testCompile group: 'com.twitter', name: 'finagle-http_2.11', version: '19.1.0' - - testCompile(group: 'com.fasterxml.jackson.module', name: 'jackson-module-scala_2.11', version: '2.10.2') { - force = true - } - -// latestDepTestCompile project(':dd-java-agent:instrumentation:java-concurrent') - // latestDepTestCompile project(':dd-java-agent:instrumentation:trace-annotation') -} - -//compileLatestDepTestGroovy { -// classpath = classpath.plus(files(compileLatestDepTestScala.destinationDir)) -// dependsOn compileLatestDepTestScala -//} diff --git a/settings.gradle b/settings.gradle index 0914d6bd70..d649c059f2 100644 --- a/settings.gradle +++ b/settings.gradle @@ -60,7 +60,7 @@ include ':dd-java-agent:instrumentation:elasticsearch:transport-2' include ':dd-java-agent:instrumentation:elasticsearch:transport-5' include ':dd-java-agent:instrumentation:elasticsearch:transport-5.3' include ':dd-java-agent:instrumentation:elasticsearch:transport-6' -include ':dd-java-agent:instrumentation:finatra' +include ':dd-java-agent:instrumentation:finatra-2.9' include ':dd-java-agent:instrumentation:glassfish' include ':dd-java-agent:instrumentation:google-http-client' include ':dd-java-agent:instrumentation:grizzly-2' From 8ff985afdbaf3fdf8ddf1f6d42396cf70130625a Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Thu, 23 Jan 2020 12:04:56 -0500 Subject: [PATCH 08/17] Cleaner way to skip netty executor's tasks --- .../ExecutorInstrumentationUtils.java | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java index 6967415e8d..c61aab869c 100644 --- a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java +++ b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java @@ -26,11 +26,24 @@ public class ExecutorInstrumentationUtils { * @return true iff given task object should be wrapped */ public static boolean shouldAttachStateToTask(final Object task, final Executor executor) { + if (task == null) { + return false; + } + final TraceScope scope = activeScope(); - return (scope != null + final Class enclosingClass = task.getClass().getEnclosingClass(); + + return scope != null && scope.isAsyncPropagating() - && task != null - && !ExecutorInstrumentationUtils.isExecutorDisabledForThisTask(executor, task)); + && !ExecutorInstrumentationUtils.isExecutorDisabledForThisTask(executor, task) + + // Don't instrument the executor's own runnables. These runnables may never return until + // netty shuts down. Any created continuations will be open until that time preventing + // traces from being reported + && (enclosingClass == null + || !enclosingClass + .getName() + .equals("io.netty.util.concurrent.SingleThreadEventExecutor")); } /** @@ -47,20 +60,13 @@ public class ExecutorInstrumentationUtils { final State state = contextStore.putIfAbsent(task, State.FACTORY); - // Don't instrument the executor's own runnables. These runnables may never return until - // netty shuts down. Any created continuations will be open until that time preventing traces - // from being reported - if (!task.getClass() - .getName() - .startsWith("io.netty.util.concurrent.SingleThreadEventExecutor$")) { - - final TraceScope.Continuation continuation = scope.capture(); - if (state.setContinuation(continuation)) { - log.debug("created continuation {} from scope {}, state: {}", continuation, scope, state); - } else { - continuation.close(false); - } + final TraceScope.Continuation continuation = scope.capture(); + if (state.setContinuation(continuation)) { + log.debug("created continuation {} from scope {}, state: {}", continuation, scope, state); + } else { + continuation.close(false); } + return state; } From 37a279069b87af184930d70209077a03b23b50f5 Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Thu, 30 Jan 2020 15:23:26 -0500 Subject: [PATCH 09/17] Add server tests --- .../finatra/FinatraInstrumentation.java | 13 ++- .../src/test/groovy/FinatraServerTest.groovy | 91 +++++++++++++++++++ .../NettyServerTestInstrumentation.java | 20 ++++ .../src/test/scala/FinatraController.scala | 56 ++++++++++++ .../src/test/scala/FinatraServer.scala | 13 +++ .../ResponseSettingExceptionMapper.scala | 15 +++ 6 files changed, 205 insertions(+), 3 deletions(-) create mode 100644 dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy create mode 100644 dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/NettyServerTestInstrumentation.java create mode 100644 dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala create mode 100644 dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraServer.scala create mode 100644 dd-java-agent/instrumentation/finatra-2.9/src/test/scala/ResponseSettingExceptionMapper.scala diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java index 2911bd58bf..3c426fbdde 100644 --- a/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java +++ b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java @@ -19,6 +19,7 @@ import com.twitter.finagle.http.Response; import com.twitter.util.Future; import com.twitter.util.FutureEventListener; import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.api.Config; import datadog.trace.api.DDTags; import datadog.trace.instrumentation.api.AgentScope; import datadog.trace.instrumentation.api.AgentSpan; @@ -76,8 +77,6 @@ public class FinatraInstrumentation extends Instrumenter.Default { final AgentSpan span = startSpan("finatra.request"); DECORATE.afterStart(span); - DECORATE.onConnection(span, request); - DECORATE.onRequest(span, request); if (parent != null && "netty.request".equals(parent.getSpanName())) { parent.setTag(DDTags.RESOURCE_NAME, request.method().name() + " " + path); @@ -86,6 +85,9 @@ public class FinatraInstrumentation extends Instrumenter.Default { span.setSpanName("finatra.controller"); span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForClass(clazz)); + } else { + DECORATE.onConnection(span, request); + DECORATE.onRequest(span, request); } final AgentScope scope = activateSpan(span, false); @@ -125,7 +127,12 @@ public class FinatraInstrumentation extends Instrumenter.Default { @Override public void onSuccess(final Response response) { - DECORATE.onResponse(scope.span(), response); + if ("finatra.request".equals(scope.span().getSpanName())) { + DECORATE.onResponse(scope.span(), response); + } else if (Config.get().getHttpServerErrorStatuses().contains(DECORATE.status(response))) { + scope.span().setError(true); + } + DECORATE.beforeFinish(scope.span()); scope.span().finish(); scope.close(); diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy b/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy new file mode 100644 index 0000000000..da891d5fc7 --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy @@ -0,0 +1,91 @@ +import com.twitter.finatra.http.HttpServer +import com.twitter.util.Await +import com.twitter.util.Closable +import com.twitter.util.Duration +import datadog.opentracing.DDSpan +import datadog.trace.agent.test.asserts.TraceAssert +import datadog.trace.agent.test.base.HttpServerTest +import datadog.trace.api.DDSpanTypes +import datadog.trace.instrumentation.api.Tags +import datadog.trace.instrumentation.finatra.FinatraDecorator + +import java.util.concurrent.TimeoutException + +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +class FinatraServerTest extends HttpServerTest { + private static final Duration TIMEOUT = Duration.fromSeconds(5) + private static final long STARTUP_TIMEOUT = 20 * 1000 + + static closeAndWait(Closable closable) { + if (closable != null) { + Await.ready(closable.close(), TIMEOUT) + } + } + + @Override + HttpServer startServer(int port) { + HttpServer testServer = new FinatraServer() + + // Starting the server is blocking so start it in a separate thread + Thread startupThread = new Thread({ + testServer.main("-admin.port=:0", "-http.port=:" + port) + }) + startupThread.setDaemon(true) + startupThread.start() + + long startupDeadline = System.currentTimeMillis() + STARTUP_TIMEOUT + while (!testServer.started()) { + if (System.currentTimeMillis() > startupDeadline) { + throw new TimeoutException("Timed out waiting for server startup") + } + } + + return testServer + } + + @Override + boolean hasHandlerSpan() { + return true + } + + @Override + void stopServer(HttpServer httpServer) { + Await.ready(httpServer.close(), TIMEOUT) + } + + @Override + FinatraDecorator decorator() { + return FinatraDecorator.DECORATE + } + + @Override + String expectedOperationName() { + return "finatra.request" + } + + void handlerSpan(TraceAssert trace, int index, Object parent, ServerEndpoint endpoint = SUCCESS) { + def errorEndpoint = endpoint == EXCEPTION || endpoint == ERROR + trace.span(index) { + serviceName expectedServiceName() + operationName "finatra.controller" + resourceName "FinatraController" + spanType DDSpanTypes.HTTP_SERVER + errored errorEndpoint + childOf(parent as DDSpan) + tags { + "$Tags.COMPONENT" FinatraDecorator.DECORATE.component() + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + + // Finatra doesn't propagate the stack trace or exception to the instrumentation + // so the normal errorTags() method can't be used + if (errorEndpoint) { + "$Tags.ERROR" true + } + defaultTags() + } + } + } +} diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/NettyServerTestInstrumentation.java b/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/NettyServerTestInstrumentation.java new file mode 100644 index 0000000000..224b08f4af --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/NettyServerTestInstrumentation.java @@ -0,0 +1,20 @@ +import static net.bytebuddy.matcher.ElementMatchers.named; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.test.base.HttpServerTestAdvice; +import datadog.trace.agent.tooling.Instrumenter; +import net.bytebuddy.agent.builder.AgentBuilder; + +@AutoService(Instrumenter.class) +public class NettyServerTestInstrumentation implements Instrumenter { + + @Override + public AgentBuilder instrument(final AgentBuilder agentBuilder) { + return agentBuilder + .type(named("io.netty.handler.codec.ByteToMessageDecoder")) + .transform( + new AgentBuilder.Transformer.ForAdvice() + .advice( + named("channelRead"), HttpServerTestAdvice.ServerEntryAdvice.class.getName())); + } +} diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala new file mode 100644 index 0000000000..6a5503c8a4 --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala @@ -0,0 +1,56 @@ +import com.twitter.finagle.http.{Request, Response} +import com.twitter.finatra.http.Controller +import com.twitter.util.Future +import datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint._ +import datadog.trace.agent.test.base.HttpServerTest.controller +import groovy.lang.Closure + +class FinatraController extends Controller { + any(SUCCESS.getPath) { request: Request => + controller(SUCCESS, new Closure[Response](null) { + override def call(): Response = { + response.ok(SUCCESS.getBody) + } + }) + } + + any(ERROR.getPath) { request: Request => + controller(ERROR, new Closure[Response](null) { + override def call(): Response = { + response.internalServerError(ERROR.getBody) + } + }) + } + + any(NOT_FOUND.getPath) { request: Request => + controller(NOT_FOUND, new Closure[Response](null) { + override def call(): Response = { + response.notFound(NOT_FOUND.getBody) + } + }) + } + + any(QUERY_PARAM.getPath) { request: Request => + controller(QUERY_PARAM, new Closure[Response](null) { + override def call(): Response = { + response.ok(QUERY_PARAM.getBody) + } + }) + } + + any(EXCEPTION.getPath) { request: Request => + controller(EXCEPTION, new Closure[Future[Response]](null) { + override def call(): Future[Response] = { + throw new Exception(EXCEPTION.getBody) + } + }) + } + + any(REDIRECT.getPath) { request: Request => + controller(REDIRECT, new Closure[Response](null) { + override def call(): Response = { + response.found.location(REDIRECT.getBody) + } + }) + } +} diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraServer.scala b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraServer.scala new file mode 100644 index 0000000000..0bc035e017 --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraServer.scala @@ -0,0 +1,13 @@ +import com.twitter.finagle.http.Request +import com.twitter.finatra.http.HttpServer +import com.twitter.finatra.http.filters.ExceptionMappingFilter +import com.twitter.finatra.http.routing.HttpRouter + +class FinatraServer extends HttpServer { + override protected def configureHttp(router: HttpRouter): Unit = { + router + .filter[ExceptionMappingFilter[Request]] + .add[FinatraController] + .exceptionMapper[ResponseSettingExceptionMapper] + } +} diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/ResponseSettingExceptionMapper.scala b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/ResponseSettingExceptionMapper.scala new file mode 100644 index 0000000000..de5a31928d --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/ResponseSettingExceptionMapper.scala @@ -0,0 +1,15 @@ + + +import com.twitter.finagle.http.{Request, Response} +import com.twitter.finatra.http.exceptions.ExceptionMapper +import com.twitter.finatra.http.response.ResponseBuilder +import javax.inject.{Inject, Singleton} + +@Singleton +class ResponseSettingExceptionMapper @Inject()(response: ResponseBuilder) + extends ExceptionMapper[Exception] { + + override def toResponse(request: Request, exception: Exception): Response = { + response.internalServerError(exception.getMessage) + } +} From ed12af6994f43ff2496a6ad3bfddd5b66ab6ce51 Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Thu, 30 Jan 2020 18:21:49 -0500 Subject: [PATCH 10/17] Assume parent span is netty --- .../finatra/FinatraInstrumentation.java | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java index 3c426fbdde..19269c0d16 100644 --- a/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java +++ b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java @@ -73,22 +73,15 @@ public class FinatraInstrumentation extends Instrumenter.Default { @Advice.FieldValue("clazz") final Class clazz, @Advice.Origin final Method method) { + // Update the parent "netty.request" final AgentSpan parent = activeSpan(); + parent.setTag(DDTags.RESOURCE_NAME, request.method().name() + " " + path); + parent.setTag(Tags.COMPONENT, "finatra"); + parent.setSpanName("finatra.request"); - final AgentSpan span = startSpan("finatra.request"); + final AgentSpan span = startSpan("finatra.controller"); DECORATE.afterStart(span); - - if (parent != null && "netty.request".equals(parent.getSpanName())) { - parent.setTag(DDTags.RESOURCE_NAME, request.method().name() + " " + path); - parent.setTag(Tags.COMPONENT, "finatra"); - parent.setSpanName("finatra.request"); - - span.setSpanName("finatra.controller"); - span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForClass(clazz)); - } else { - DECORATE.onConnection(span, request); - DECORATE.onRequest(span, request); - } + span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForClass(clazz)); final AgentScope scope = activateSpan(span, false); scope.setAsyncPropagation(true); @@ -127,9 +120,8 @@ public class FinatraInstrumentation extends Instrumenter.Default { @Override public void onSuccess(final Response response) { - if ("finatra.request".equals(scope.span().getSpanName())) { - DECORATE.onResponse(scope.span(), response); - } else if (Config.get().getHttpServerErrorStatuses().contains(DECORATE.status(response))) { + // Don't use DECORATE.onResponse because this is the controller span + if (Config.get().getHttpServerErrorStatuses().contains(DECORATE.status(response))) { scope.span().setError(true); } From 6ae4ab3880c826993ffac425453038d688c38c77 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Fri, 31 Jan 2020 14:43:07 -0500 Subject: [PATCH 11/17] Begin 0.43.0 --- dd-trace-java.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-trace-java.gradle b/dd-trace-java.gradle index 8523e660ae..b4da4e78f2 100644 --- a/dd-trace-java.gradle +++ b/dd-trace-java.gradle @@ -16,7 +16,7 @@ def isCI = System.getenv("CI") != null allprojects { group = 'com.datadoghq' - version = '0.42.0' + version = '0.43.0-SNAPSHOT' if (isCI) { buildDir = "${rootDir}/workspace/${projectDir.path.replace(rootDir.path, '')}/build/" From 697d4972a8cb75be75cbf62dde4166be7b72e1c9 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 22 Jan 2020 09:05:07 -0800 Subject: [PATCH 12/17] Remove Jackson from dd-trace-ot Reduced the size of dd-java-agent jar by about 2 MB. Jackson is not removed completely though as it is still a dependency of jmxfetch. Trace serialization is primarily done directly with msgpack. Response deserialization and LoggingWriter serialization is done with mochi. Msgpack Serialization buffer still not being reused though. --- dd-trace-ot/dd-trace-ot.gradle | 4 +- .../main/java/datadog/opentracing/DDSpan.java | 24 +--- .../datadog/opentracing/DDSpanContext.java | 3 - .../common/sampling/RateByServiceSampler.java | 24 +--- .../common/serialization/FormatWriter.java | 101 ++++++++++++++ .../serialization/JsonFormatWriter.java | 130 ++++++++++++++++++ .../serialization/MsgpackFormatWriter.java | 87 ++++++++++++ .../trace/common/writer/LoggingWriter.java | 10 +- .../common/writer/ddagent/DDAgentApi.java | 81 +++++------ .../ddagent/DDAgentResponseListener.java | 4 +- .../ddagent/TraceProcessingDisruptor.java | 4 - .../DDSpanSerializationTest.groovy | 68 ++++----- .../sampling/RateByServiceSamplerTest.groovy | 19 ++- .../trace/api/writer/DDAgentApiTest.groovy | 15 +- .../trace/api/writer/DDAgentWriterTest.groovy | 13 +- .../trace/api/writer/LoggingWriterTest.groovy | 21 +++ .../groovy/DDApiIntegrationTest.groovy | 11 +- gradle/dependencies.gradle | 7 - 18 files changed, 465 insertions(+), 161 deletions(-) create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/serialization/FormatWriter.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/serialization/JsonFormatWriter.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/serialization/MsgpackFormatWriter.java create mode 100644 dd-trace-ot/src/test/groovy/datadog/trace/api/writer/LoggingWriterTest.groovy diff --git a/dd-trace-ot/dd-trace-ot.gradle b/dd-trace-ot/dd-trace-ot.gradle index cf92790fcb..256213b2d8 100644 --- a/dd-trace-ot/dd-trace-ot.gradle +++ b/dd-trace-ot/dd-trace-ot.gradle @@ -37,9 +37,10 @@ dependencies { compile group: 'com.datadoghq', name: 'java-dogstatsd-client', version: '2.1.1' - compile deps.jackson compile deps.slf4j + compile group: 'org.msgpack', name: 'msgpack-core', version: '0.8.20' compile group: 'com.squareup.okhttp3', name: 'okhttp', version: '3.11.0' // Last version to support Java7 + compile group: 'com.squareup.moshi', name: 'moshi', version: '1.9.2' compile group: 'com.github.jnr', name: 'jnr-unixsocket', version: '0.23' compile group: 'com.lmax', name: 'disruptor', version: '3.4.2' @@ -50,6 +51,7 @@ dependencies { testCompile project(":dd-java-agent:testing") testCompile project(':utils:gc-utils') testCompile group: 'com.github.stefanbirkner', name: 'system-rules', version: '1.17.1' + testCompile group: 'org.msgpack', name: 'jackson-dataformat-msgpack', version: '0.8.20' traceAgentTestCompile deps.testcontainers diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java index bf6984d6ff..2160d1cf01 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java @@ -3,8 +3,6 @@ package datadog.opentracing; import static io.opentracing.log.Fields.ERROR_OBJECT; import static io.opentracing.log.Fields.MESSAGE; -import com.fasterxml.jackson.annotation.JsonGetter; -import com.fasterxml.jackson.annotation.JsonIgnore; import datadog.trace.api.DDTags; import datadog.trace.api.interceptor.MutableSpan; import datadog.trace.api.sampling.PrioritySampling; @@ -77,7 +75,6 @@ public class DDSpan implements Span, MutableSpan { context.getTrace().registerSpan(this); } - @JsonIgnore public boolean isFinished() { return durationNano.get() != 0; } @@ -120,20 +117,17 @@ public class DDSpan implements Span, MutableSpan { * * @return true if root, false otherwise */ - @JsonIgnore public final boolean isRootSpan() { return BigInteger.ZERO.equals(context.getParentId()); } @Override @Deprecated - @JsonIgnore public MutableSpan getRootSpan() { return getLocalRootSpan(); } @Override - @JsonIgnore public MutableSpan getLocalRootSpan() { return context().getTrace().getRootSpan(); } @@ -297,14 +291,13 @@ public class DDSpan implements Span, MutableSpan { return this; } - // Getters and JSON serialisation instructions + // Getters /** * Meta merges baggage and tags (stringified values) * * @return merged context baggage and tags */ - @JsonGetter public Map getMeta() { final Map meta = new HashMap<>(); for (final Map.Entry entry : context().getBaggageItems().entrySet()) { @@ -321,58 +314,48 @@ public class DDSpan implements Span, MutableSpan { * * @return metrics for this span */ - @JsonGetter public Map getMetrics() { return context.getMetrics(); } @Override - @JsonGetter("start") public long getStartTime() { return startTimeNano > 0 ? startTimeNano : TimeUnit.MICROSECONDS.toNanos(startTimeMicro); } @Override - @JsonGetter("duration") public long getDurationNano() { return durationNano.get(); } @Override - @JsonGetter("service") public String getServiceName() { return context.getServiceName(); } - @JsonGetter("trace_id") public BigInteger getTraceId() { return context.getTraceId(); } - @JsonGetter("span_id") public BigInteger getSpanId() { return context.getSpanId(); } - @JsonGetter("parent_id") public BigInteger getParentId() { return context.getParentId(); } @Override - @JsonGetter("resource") public String getResourceName() { return context.getResourceName(); } @Override - @JsonGetter("name") public String getOperationName() { return context.getOperationName(); } @Override - @JsonIgnore public Integer getSamplingPriority() { final int samplingPriority = context.getSamplingPriority(); if (samplingPriority == PrioritySampling.UNSET) { @@ -383,29 +366,24 @@ public class DDSpan implements Span, MutableSpan { } @Override - @JsonIgnore public String getSpanType() { return context.getSpanType(); } @Override - @JsonIgnore public Map getTags() { return context().getTags(); } - @JsonGetter public String getType() { return context.getSpanType(); } @Override - @JsonIgnore public Boolean isError() { return context.getErrorFlag(); } - @JsonGetter public int getError() { return context.getErrorFlag() ? 1 : 0; } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java index 9ad2d54202..766cc82f2a 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java @@ -1,6 +1,5 @@ package datadog.opentracing; -import com.fasterxml.jackson.annotation.JsonIgnore; import datadog.opentracing.decorators.AbstractDecorator; import datadog.trace.api.DDTags; import datadog.trace.api.sampling.PrioritySampling; @@ -289,12 +288,10 @@ public class DDSpanContext implements io.opentracing.SpanContext { return baggageItems.entrySet(); } - @JsonIgnore public PendingTrace getTrace() { return trace; } - @JsonIgnore public DDTracer getTracer() { return tracer; } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/sampling/RateByServiceSampler.java b/dd-trace-ot/src/main/java/datadog/trace/common/sampling/RateByServiceSampler.java index b8191b1a8f..03417eb729 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/sampling/RateByServiceSampler.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/sampling/RateByServiceSampler.java @@ -3,13 +3,10 @@ package datadog.trace.common.sampling; import static java.util.Collections.singletonMap; import static java.util.Collections.unmodifiableMap; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.node.NumericNode; import datadog.opentracing.DDSpan; import datadog.trace.api.sampling.PrioritySampling; import datadog.trace.common.writer.ddagent.DDAgentResponseListener; import java.util.HashMap; -import java.util.Iterator; import java.util.Map; import lombok.extern.slf4j.Slf4j; @@ -70,23 +67,16 @@ public class RateByServiceSampler implements Sampler, PrioritySampler, DDAgentRe } @Override - public void onResponse(final String endpoint, final JsonNode responseJson) { - final JsonNode newServiceRates = responseJson.get("rate_by_service"); + public void onResponse( + final String endpoint, final Map> responseJson) { + final Map newServiceRates = responseJson.get("rate_by_service"); if (null != newServiceRates) { log.debug("Update service sampler rates: {} -> {}", endpoint, responseJson); final Map updatedServiceRates = new HashMap<>(); - final Iterator itr = newServiceRates.fieldNames(); - while (itr.hasNext()) { - final String key = itr.next(); - final JsonNode value = newServiceRates.get(key); - try { - if (value instanceof NumericNode) { - updatedServiceRates.put(key, createRateSampler(value.doubleValue())); - } else { - log.debug("Unable to parse new service rate {} -> {}", key, value); - } - } catch (final NumberFormatException nfe) { - log.debug("Unable to parse new service rate {} -> {}", key, value); + for (final Map.Entry entry : newServiceRates.entrySet()) { + if (entry.getValue() != null) { + updatedServiceRates.put( + entry.getKey(), createRateSampler(entry.getValue().doubleValue())); } } if (!updatedServiceRates.containsKey(DEFAULT_KEY)) { diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/serialization/FormatWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/serialization/FormatWriter.java new file mode 100644 index 0000000000..4bc1093616 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/serialization/FormatWriter.java @@ -0,0 +1,101 @@ +package datadog.trace.common.serialization; + +import datadog.opentracing.DDSpan; +import java.io.IOException; +import java.math.BigInteger; +import java.util.List; +import java.util.Map; + +public abstract class FormatWriter { + public abstract void writeKey(String key, DEST destination) throws IOException; + + public abstract void writeListHeader(int size, DEST destination) throws IOException; + + public abstract void writeListFooter(DEST destination) throws IOException; + + public abstract void writeMapHeader(int size, DEST destination) throws IOException; + + public abstract void writeMapFooter(DEST destination) throws IOException; + + public abstract void writeString(String key, String value, DEST destination) throws IOException; + + public abstract void writeShort(String key, short value, DEST destination) throws IOException; + + public abstract void writeByte(String key, byte value, DEST destination) throws IOException; + + public abstract void writeInt(String key, int value, DEST destination) throws IOException; + + public abstract void writeLong(String key, long value, DEST destination) throws IOException; + + public abstract void writeFloat(String key, float value, DEST destination) throws IOException; + + public abstract void writeDouble(String key, double value, DEST destination) throws IOException; + + public abstract void writeBigInteger(String key, BigInteger value, DEST destination) + throws IOException; + + public void writeNumber(final String key, final Number value, final DEST destination) + throws IOException { + if (value instanceof Double) { + writeDouble(key, value.doubleValue(), destination); + } else if (value instanceof Long) { + writeLong(key, value.longValue(), destination); + } else if (value instanceof Integer) { + writeInt(key, value.intValue(), destination); + } else if (value instanceof Float) { + writeFloat(key, value.floatValue(), destination); + } else if (value instanceof Byte) { + writeByte(key, value.byteValue(), destination); + } else if (value instanceof Short) { + writeShort(key, value.shortValue(), destination); + } + } + + public void writeNumberMap( + final String key, final Map value, final DEST destination) + throws IOException { + writeKey(key, destination); + writeMapHeader(value.size(), destination); + for (final Map.Entry entry : value.entrySet()) { + writeNumber(entry.getKey(), entry.getValue(), destination); + } + writeMapFooter(destination); + } + + public void writeStringMap( + final String key, final Map value, final DEST destination) + throws IOException { + writeKey(key, destination); + writeMapHeader(value.size(), destination); + for (final Map.Entry entry : value.entrySet()) { + writeString(entry.getKey(), entry.getValue(), destination); + } + writeMapFooter(destination); + } + + public void writeTrace(final List trace, final DEST destination) throws IOException { + writeListHeader(trace.size(), destination); + for (final DDSpan span : trace) { + writeDDSpan(span, destination); + } + writeListFooter(destination); + } + + public void writeDDSpan(final DDSpan span, final DEST destination) throws IOException { + // Some of the tests rely on the specific ordering here. + writeMapHeader(12, destination); // must match count below. + /* 1 */ writeString("service", span.getServiceName(), destination); + /* 2 */ writeString("name", span.getOperationName(), destination); + /* 3 */ writeString("resource", span.getResourceName(), destination); + /* 4 */ writeBigInteger("trace_id", span.getTraceId(), destination); + /* 5 */ writeBigInteger("span_id", span.getSpanId(), destination); + /* 6 */ writeBigInteger("parent_id", span.getParentId(), destination); + /* 7 */ writeLong("start", span.getStartTime(), destination); + /* 8 */ writeLong("duration", span.getDurationNano(), destination); + /* 9 */ writeString("type", span.getType(), destination); + /* 10 */ writeInt("error", span.getError(), destination); + /* 11 */ writeNumberMap("metrics", span.getMetrics(), destination); + /* 12 */ writeStringMap("meta", span.getMeta(), destination); + writeMapFooter(destination); + } +} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/serialization/JsonFormatWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/serialization/JsonFormatWriter.java new file mode 100644 index 0000000000..429fc15742 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/serialization/JsonFormatWriter.java @@ -0,0 +1,130 @@ +package datadog.trace.common.serialization; + +import com.squareup.moshi.JsonAdapter; +import com.squareup.moshi.JsonReader; +import com.squareup.moshi.JsonWriter; +import com.squareup.moshi.Moshi; +import com.squareup.moshi.Types; +import datadog.opentracing.DDSpan; +import java.io.IOException; +import java.lang.annotation.Annotation; +import java.lang.reflect.Type; +import java.math.BigInteger; +import java.util.List; +import java.util.Set; + +public class JsonFormatWriter extends FormatWriter { + private static final Moshi MOSHI = new Moshi.Builder().add(DDSpanAdapter.FACTORY).build(); + + public static final JsonAdapter> TRACE_ADAPTER = + MOSHI.adapter(Types.newParameterizedType(List.class, DDSpan.class)); + public static final JsonAdapter SPAN_ADAPTER = MOSHI.adapter(DDSpan.class); + + public static JsonFormatWriter JSON_WRITER = new JsonFormatWriter(); + + @Override + public void writeKey(final String key, final JsonWriter destination) throws IOException { + destination.name(key); + } + + @Override + public void writeListHeader(final int size, final JsonWriter destination) throws IOException { + destination.beginArray(); + } + + @Override + public void writeListFooter(final JsonWriter destination) throws IOException { + destination.endArray(); + } + + @Override + public void writeMapHeader(final int size, final JsonWriter destination) throws IOException { + destination.beginObject(); + } + + @Override + public void writeMapFooter(final JsonWriter destination) throws IOException { + destination.endObject(); + } + + @Override + public void writeString(final String key, final String value, final JsonWriter destination) + throws IOException { + destination.name(key); + destination.value(value); + } + + @Override + public void writeShort(final String key, final short value, final JsonWriter destination) + throws IOException { + destination.name(key); + destination.value(value); + } + + @Override + public void writeByte(final String key, final byte value, final JsonWriter destination) + throws IOException { + destination.name(key); + destination.value(value); + } + + @Override + public void writeInt(final String key, final int value, final JsonWriter destination) + throws IOException { + destination.name(key); + destination.value(value); + } + + @Override + public void writeLong(final String key, final long value, final JsonWriter destination) + throws IOException { + destination.name(key); + destination.value(value); + } + + @Override + public void writeFloat(final String key, final float value, final JsonWriter destination) + throws IOException { + destination.name(key); + destination.value(value); + } + + @Override + public void writeDouble(final String key, final double value, final JsonWriter destination) + throws IOException { + destination.name(key); + destination.value(value); + } + + @Override + public void writeBigInteger( + final String key, final BigInteger value, final JsonWriter destination) throws IOException { + destination.name(key); + destination.value(value); + } + + static class DDSpanAdapter extends JsonAdapter { + public static final JsonAdapter.Factory FACTORY = + new JsonAdapter.Factory() { + @Override + public JsonAdapter create( + final Type type, final Set annotations, final Moshi moshi) { + final Class rawType = Types.getRawType(type); + if (rawType.isAssignableFrom(DDSpan.class)) { + return new DDSpanAdapter(); + } + return null; + } + }; + + @Override + public DDSpan fromJson(final JsonReader reader) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public void toJson(final JsonWriter writer, final DDSpan value) throws IOException { + JSON_WRITER.writeDDSpan(value, writer); + } + } +} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/serialization/MsgpackFormatWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/serialization/MsgpackFormatWriter.java new file mode 100644 index 0000000000..ca5579da7e --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/serialization/MsgpackFormatWriter.java @@ -0,0 +1,87 @@ +package datadog.trace.common.serialization; + +import java.io.IOException; +import java.math.BigInteger; +import org.msgpack.core.MessagePacker; + +public class MsgpackFormatWriter extends FormatWriter { + public static MsgpackFormatWriter MSGPACK_WRITER = new MsgpackFormatWriter(); + + @Override + public void writeKey(final String key, final MessagePacker destination) throws IOException { + destination.packString(key); + } + + @Override + public void writeListHeader(final int size, final MessagePacker destination) throws IOException { + destination.packArrayHeader(size); + } + + @Override + public void writeListFooter(final MessagePacker destination) throws IOException {} + + @Override + public void writeMapHeader(final int size, final MessagePacker destination) throws IOException { + destination.packMapHeader(size); + } + + @Override + public void writeMapFooter(final MessagePacker destination) throws IOException {} + + @Override + public void writeString(final String key, final String value, final MessagePacker destination) + throws IOException { + destination.packString(key); + destination.packString(value); + } + + @Override + public void writeShort(final String key, final short value, final MessagePacker destination) + throws IOException { + destination.packString(key); + destination.packShort(value); + } + + @Override + public void writeByte(final String key, final byte value, final MessagePacker destination) + throws IOException { + destination.packString(key); + destination.packByte(value); + } + + @Override + public void writeInt(final String key, final int value, final MessagePacker destination) + throws IOException { + destination.packString(key); + destination.packInt(value); + } + + @Override + public void writeLong(final String key, final long value, final MessagePacker destination) + throws IOException { + destination.packString(key); + destination.packLong(value); + } + + @Override + public void writeFloat(final String key, final float value, final MessagePacker destination) + throws IOException { + destination.packString(key); + destination.packFloat(value); + } + + @Override + public void writeDouble(final String key, final double value, final MessagePacker destination) + throws IOException { + destination.packString(key); + destination.packDouble(value); + } + + @Override + public void writeBigInteger( + final String key, final BigInteger value, final MessagePacker destination) + throws IOException { + destination.packString(key); + destination.packBigInteger(value); + } +} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/LoggingWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/LoggingWriter.java index cb95d07a72..c4a793f3b2 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/LoggingWriter.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/LoggingWriter.java @@ -1,25 +1,29 @@ package datadog.trace.common.writer; -import com.fasterxml.jackson.databind.ObjectMapper; +import static datadog.trace.common.serialization.JsonFormatWriter.TRACE_ADAPTER; + import datadog.opentracing.DDSpan; import java.util.List; import lombok.extern.slf4j.Slf4j; @Slf4j public class LoggingWriter implements Writer { - private final ObjectMapper serializer = new ObjectMapper(); @Override public void write(final List trace) { if (log.isInfoEnabled()) { try { - log.info("write(trace): {}", serializer.writeValueAsString(trace)); + log.info("write(trace): {}", toString(trace)); } catch (final Exception e) { log.error("error writing(trace): {}", trace); } } } + private String toString(final List trace) { + return TRACE_ADAPTER.toJson(trace); + } + @Override public void incrementTraceCount() { log.info("incrementTraceCount()"); diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DDAgentApi.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DDAgentApi.java index 17a607c102..64725297aa 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DDAgentApi.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DDAgentApi.java @@ -1,9 +1,10 @@ package datadog.trace.common.writer.ddagent; -import com.fasterxml.jackson.core.JsonParseException; -import com.fasterxml.jackson.core.JsonProcessingException; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; +import static datadog.trace.common.serialization.MsgpackFormatWriter.MSGPACK_WRITER; + +import com.squareup.moshi.JsonAdapter; +import com.squareup.moshi.Moshi; +import com.squareup.moshi.Types; import datadog.opentracing.ContainerInfo; import datadog.opentracing.DDSpan; import datadog.opentracing.DDTraceOTInfo; @@ -12,8 +13,8 @@ import java.io.File; import java.io.IOException; import java.io.OutputStream; import java.util.ArrayList; -import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.concurrent.TimeUnit; import lombok.extern.slf4j.Slf4j; import okhttp3.HttpUrl; @@ -24,7 +25,7 @@ import okhttp3.RequestBody; import okio.BufferedSink; import org.msgpack.core.MessagePack; import org.msgpack.core.MessagePacker; -import org.msgpack.jackson.dataformat.MessagePackFactory; +import org.msgpack.core.buffer.ArrayBufferOutput; /** The API pointing to a DD agent */ @Slf4j @@ -47,7 +48,14 @@ public class DDAgentApi { private volatile long nextAllowedLogTime = 0; - private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(new MessagePackFactory()); + private static final JsonAdapter>> RESPONSE_ADAPTER = + new Moshi.Builder() + .build() + .adapter( + Types.newParameterizedType( + Map.class, + String.class, + Types.newParameterizedType(Map.class, String.class, Double.class))); private static final MediaType MSGPACK = MediaType.get("application/msgpack"); private final OkHttpClient httpClient; @@ -57,7 +65,7 @@ public class DDAgentApi { this( host, port, - traceEndpointAvailable(getUrl(host, port, TRACES_ENDPOINT_V4), unixDomainSocketPath), + endpointAvailable(getUrl(host, port, TRACES_ENDPOINT_V4), unixDomainSocketPath, true), unixDomainSocketPath); } @@ -97,7 +105,7 @@ public class DDAgentApi { final byte[] serializedTrace = serializeTrace(trace); sizeInBytes += serializedTrace.length; serializedTraces.add(serializedTrace); - } catch (final JsonProcessingException e) { + } catch (final IOException e) { log.warn("Error serializing trace", e); // TODO: DQH - Incorporate the failed serialization into the Response object??? @@ -107,8 +115,13 @@ public class DDAgentApi { return sendSerializedTraces(serializedTraces.size(), sizeInBytes, serializedTraces); } - byte[] serializeTrace(final List trace) throws JsonProcessingException { - return OBJECT_MAPPER.writeValueAsBytes(trace); + byte[] serializeTrace(final List trace) throws IOException { + // TODO: reuse byte array buffer + final ArrayBufferOutput output = new ArrayBufferOutput(); + final MessagePacker packer = MessagePack.newDefaultPacker(output); + MSGPACK_WRITER.writeTrace(trace, packer); + packer.flush(); + return output.toByteArray(); } Response sendSerializedTraces( @@ -183,17 +196,16 @@ public class DDAgentApi { final String responseString = response.body().string().trim(); try { if (!"".equals(responseString) && !"OK".equalsIgnoreCase(responseString)) { - final JsonNode parsedResponse = OBJECT_MAPPER.readTree(responseString); + final Map> parsedResponse = + RESPONSE_ADAPTER.fromJson(responseString); final String endpoint = tracesUrl.toString(); for (final DDAgentResponseListener listener : responseListeners) { listener.onResponse(endpoint, parsedResponse); } - return Response.success(response.code(), parsedResponse); } - return Response.success(response.code()); - } catch (final JsonParseException e) { + } catch (final IOException e) { log.debug("Failed to parse DD agent response: " + responseString, e); return Response.success(response.code(), e); @@ -222,19 +234,13 @@ public class DDAgentApi { } } - private static boolean traceEndpointAvailable( - final HttpUrl url, final String unixDomainSocketPath) { - return endpointAvailable(url, unixDomainSocketPath, Collections.emptyList(), true); - } + private static final byte[] EMPTY_LIST = new byte[] {MessagePack.Code.FIXARRAY_PREFIX}; private static boolean endpointAvailable( - final HttpUrl url, - final String unixDomainSocketPath, - final Object data, - final boolean retry) { + final HttpUrl url, final String unixDomainSocketPath, final boolean retry) { try { final OkHttpClient client = buildHttpClient(unixDomainSocketPath); - final RequestBody body = RequestBody.create(MSGPACK, OBJECT_MAPPER.writeValueAsBytes(data)); + final RequestBody body = RequestBody.create(MSGPACK, EMPTY_LIST); final Request request = prepareRequest(url).put(body).build(); try (final okhttp3.Response response = client.newCall(request).execute()) { @@ -242,7 +248,7 @@ public class DDAgentApi { } } catch (final IOException e) { if (retry) { - return endpointAvailable(url, unixDomainSocketPath, data, false); + return endpointAvailable(url, unixDomainSocketPath, false); } } return false; @@ -307,42 +313,31 @@ public class DDAgentApi { public static final class Response { /** Factory method for a successful request with a trivial response body */ public static final Response success(final int status) { - return new Response(true, status, null, null); - } - - /** Factory method for a successful request with a well-formed JSON response body */ - public static final Response success(final int status, final JsonNode json) { - return new Response(true, status, json, null); + return new Response(true, status, null); } /** Factory method for a successful request will a malformed response body */ public static final Response success(final int status, final Throwable exception) { - return new Response(true, status, null, exception); + return new Response(true, status, exception); } /** Factory method for a request that receive an error status in response */ public static final Response failed(final int status) { - return new Response(false, status, null, null); + return new Response(false, status, null); } /** Factory method for a failed communication attempt */ public static final Response failed(final Throwable exception) { - return new Response(false, null, null, exception); + return new Response(false, null, exception); } private final boolean success; private final Integer status; - private final JsonNode json; private final Throwable exception; - private Response( - final boolean success, - final Integer status, - final JsonNode json, - final Throwable exception) { + private Response(final boolean success, final Integer status, final Throwable exception) { this.success = success; this.status = status; - this.json = json; this.exception = exception; } @@ -355,10 +350,6 @@ public class DDAgentApi { return status; } - public final JsonNode json() { - return json; - } - // TODO: DQH - In Java 8, switch to Optional? public final Throwable exception() { return exception; diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DDAgentResponseListener.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DDAgentResponseListener.java index a0cbc72fda..63f1ebd3df 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DDAgentResponseListener.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DDAgentResponseListener.java @@ -1,8 +1,8 @@ package datadog.trace.common.writer.ddagent; -import com.fasterxml.jackson.databind.JsonNode; +import java.util.Map; public interface DDAgentResponseListener { /** Invoked after the api receives a response from the core agent. */ - void onResponse(String endpoint, JsonNode responseJson); + void onResponse(String endpoint, Map> responseJson); } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java index e1a62bafed..67025b67f2 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java @@ -1,6 +1,5 @@ package datadog.trace.common.writer.ddagent; -import com.fasterxml.jackson.core.JsonProcessingException; import com.lmax.disruptor.EventHandler; import datadog.opentracing.DDSpan; import datadog.trace.common.util.DaemonThreadFactory; @@ -74,9 +73,6 @@ public class TraceProcessingDisruptor extends AbstractDisruptor> { batchWritingDisruptor.publish(serializedTrace, event.representativeCount); monitor.onSerialize(writer, event.data, serializedTrace); event.representativeCount = 0; // reset in case flush is invoked below. - } catch (final JsonProcessingException e) { - log.debug("Error serializing trace", e); - monitor.onFailedSerialize(writer, event.data, e); } catch (final Throwable e) { log.debug("Error while serializing trace", e); monitor.onFailedSerialize(writer, event.data, e); diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanSerializationTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanSerializationTest.groovy index 757f562cb0..8b5e04a336 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanSerializationTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanSerializationTest.groovy @@ -1,43 +1,48 @@ package datadog.opentracing -import com.fasterxml.jackson.databind.ObjectMapper -import com.google.common.collect.Maps +import com.squareup.moshi.Moshi import datadog.trace.api.DDTags import datadog.trace.api.sampling.PrioritySampling import datadog.trace.common.writer.ListWriter import datadog.trace.util.test.DDSpecification import org.msgpack.core.MessagePack import org.msgpack.core.buffer.ArrayBufferInput -import org.msgpack.jackson.dataformat.MessagePackFactory +import org.msgpack.core.buffer.ArrayBufferOutput import org.msgpack.value.ValueType +import static datadog.trace.common.serialization.JsonFormatWriter.SPAN_ADAPTER +import static datadog.trace.common.serialization.MsgpackFormatWriter.MSGPACK_WRITER + class DDSpanSerializationTest extends DDSpecification { def "serialize spans with sampling #samplingPriority"() throws Exception { setup: - final Map baggage = new HashMap<>() - baggage.put("a-baggage", "value") - final Map tags = new HashMap<>() - baggage.put("k1", "v1") + def jsonAdapter = new Moshi.Builder().build().adapter(Map) - Map expected = Maps.newHashMap() - expected.put("meta", baggage) - expected.put("service", "service") - expected.put("error", 0) - expected.put("type", "type") - expected.put("name", "operation") - expected.put("duration", 33000) - expected.put("resource", "operation") - final Map metrics = new HashMap<>() - metrics.put("_sampling_priority_v1", 1) + final Map metrics = ["_sampling_priority_v1": 1] if (samplingPriority == PrioritySampling.UNSET) { // RateByServiceSampler sets priority metrics.put("_dd.agent_psr", 1.0d) } - expected.put("metrics", metrics) - expected.put("start", 100000) - expected.put("span_id", 2l) - expected.put("parent_id", 0l) - expected.put("trace_id", 1l) + + Map expected = [ + service : "service", + name : "operation", + resource : "operation", + trace_id : 1l, + span_id : 2l, + parent_id: 0l, + start : 100000, + duration : 33000, + type : "type", + error : 0, + metrics : metrics, + meta : [ + "a-baggage" : "value", + "k1" : "v1", + (DDTags.THREAD_NAME): Thread.currentThread().getName(), + (DDTags.THREAD_ID) : String.valueOf(Thread.currentThread().getId()), + ], + ] def writer = new ListWriter() def tracer = DDTracer.builder().writer(writer).build() @@ -51,23 +56,19 @@ class DDSpanSerializationTest extends DDSpecification { null, samplingPriority, null, - new HashMap<>(baggage), + ["a-baggage": "value"], false, "type", - tags, + ["k1": "v1"], new PendingTrace(tracer, 1G, [:]), tracer) - baggage.put(DDTags.THREAD_NAME, Thread.currentThread().getName()) - baggage.put(DDTags.THREAD_ID, String.valueOf(Thread.currentThread().getId())) - DDSpan span = new DDSpan(100L, context) span.finish(133L) - ObjectMapper serializer = new ObjectMapper() - def actualTree = serializer.readTree(serializer.writeValueAsString(span)) - def expectedTree = serializer.readTree(serializer.writeValueAsString(expected)) + def actualTree = jsonAdapter.fromJson(SPAN_ADAPTER.toJson(span)) + def expectedTree = jsonAdapter.fromJson(jsonAdapter.toJson(expected)) expect: actualTree == expectedTree @@ -79,7 +80,6 @@ class DDSpanSerializationTest extends DDSpecification { def "serialize trace/span with id #value as int"() { setup: - def objectMapper = new ObjectMapper(new MessagePackFactory()) def writer = new ListWriter() def tracer = DDTracer.builder().writer(writer).build() def context = new DDSpanContext( @@ -98,7 +98,11 @@ class DDSpanSerializationTest extends DDSpecification { new PendingTrace(tracer, 1G, [:]), tracer) def span = new DDSpan(0, context) - byte[] bytes = objectMapper.writeValueAsBytes(span) + def buffer = new ArrayBufferOutput() + def packer = MessagePack.newDefaultPacker(buffer) + MSGPACK_WRITER.writeDDSpan(span, packer) + packer.flush() + byte[] bytes = buffer.toByteArray() def unpacker = MessagePack.newDefaultUnpacker(new ArrayBufferInput(bytes)) int size = unpacker.unpackMapHeader() diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/sampling/RateByServiceSamplerTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/sampling/RateByServiceSamplerTest.groovy index b5709f1066..5b147764f3 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/api/sampling/RateByServiceSamplerTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/sampling/RateByServiceSamplerTest.groovy @@ -1,24 +1,25 @@ package datadog.trace.api.sampling -import com.fasterxml.jackson.databind.ObjectMapper + import datadog.opentracing.DDSpan import datadog.opentracing.DDTracer import datadog.opentracing.SpanFactory import datadog.trace.api.DDTags import datadog.trace.common.sampling.RateByServiceSampler import datadog.trace.common.writer.LoggingWriter +import datadog.trace.common.writer.ddagent.DDAgentApi import datadog.trace.util.test.DDSpecification import static datadog.trace.common.sampling.RateByServiceSampler.DEFAULT_KEY class RateByServiceSamplerTest extends DDSpecification { + static serializer = DDAgentApi.RESPONSE_ADAPTER def "invalid rate -> 1"() { setup: RateByServiceSampler serviceSampler = new RateByServiceSampler() - ObjectMapper serializer = new ObjectMapper() String response = '{"rate_by_service": {"service:,env:":' + rate + '}}' - serviceSampler.onResponse("traces", serializer.readTree(response)) + serviceSampler.onResponse("traces", serializer.fromJson(response)) expect: serviceSampler.serviceRates[DEFAULT_KEY].sampleRate == expectedRate @@ -35,11 +36,10 @@ class RateByServiceSamplerTest extends DDSpecification { def "rate by service name"() { setup: RateByServiceSampler serviceSampler = new RateByServiceSampler() - ObjectMapper serializer = new ObjectMapper() when: String response = '{"rate_by_service": {"service:spock,env:test":0.0}}' - serviceSampler.onResponse("traces", serializer.readTree(response)) + serviceSampler.onResponse("traces", serializer.fromJson(response)) DDSpan span1 = SpanFactory.newSpanOf("foo", "bar") serviceSampler.setSamplingPriority(span1) then: @@ -48,7 +48,7 @@ class RateByServiceSamplerTest extends DDSpecification { when: response = '{"rate_by_service": {"service:spock,env:test":1.0}}' - serviceSampler.onResponse("traces", serializer.readTree(response)) + serviceSampler.onResponse("traces", serializer.fromJson(response)) DDSpan span2 = SpanFactory.newSpanOf("spock", "test") serviceSampler.setSamplingPriority(span2) then: @@ -59,9 +59,8 @@ class RateByServiceSamplerTest extends DDSpecification { def "sampling priority set on context"() { setup: RateByServiceSampler serviceSampler = new RateByServiceSampler() - ObjectMapper serializer = new ObjectMapper() String response = '{"rate_by_service": {"service:,env:":1.0}}' - serviceSampler.onResponse("traces", serializer.readTree(response)) + serviceSampler.onResponse("traces", serializer.fromJson(response)) DDSpan span = SpanFactory.newSpanOf("foo", "bar") serviceSampler.setSamplingPriority(span) @@ -76,8 +75,8 @@ class RateByServiceSamplerTest extends DDSpecification { def sampler = new RateByServiceSampler() def tracer = DDTracer.builder().writer(new LoggingWriter()).sampler(sampler).build() - sampler.onResponse("test", new ObjectMapper() - .readTree('{"rate_by_service":{"service:,env:":1.0,"service:spock,env:":0.0}}')) + sampler.onResponse("test", serializer + .fromJson('{"rate_by_service":{"service:,env:":1.0,"service:spock,env:":0.0}}')) when: def span = tracer.buildSpan("test").start() diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy index 8d5ed0dcdd..6e6a0fb5e6 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy @@ -1,11 +1,12 @@ package datadog.trace.api.writer import com.fasterxml.jackson.core.type.TypeReference -import com.fasterxml.jackson.databind.JsonNode +import com.fasterxml.jackson.databind.ObjectMapper import datadog.opentracing.SpanFactory import datadog.trace.common.writer.ddagent.DDAgentApi import datadog.trace.common.writer.ddagent.DDAgentResponseListener import datadog.trace.util.test.DDSpecification +import org.msgpack.jackson.dataformat.MessagePackFactory import java.util.concurrent.atomic.AtomicLong import java.util.concurrent.atomic.AtomicReference @@ -13,7 +14,7 @@ import java.util.concurrent.atomic.AtomicReference import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer class DDAgentApiTest extends DDSpecification { - static mapper = DDAgentApi.OBJECT_MAPPER + static mapper = new ObjectMapper(new MessagePackFactory()) def "sending an empty list of traces returns no errors"() { setup: @@ -124,15 +125,15 @@ class DDAgentApiTest extends DDSpecification { def "Api ResponseListeners see 200 responses"() { setup: - def agentResponse = new AtomicReference(null) - DDAgentResponseListener responseListener = { String endpoint, JsonNode responseJson -> - agentResponse.set(responseJson.toString()) + def agentResponse = new AtomicReference(null) + DDAgentResponseListener responseListener = { String endpoint, Map responseJson -> + agentResponse.set(responseJson) } def agent = httpServer { handlers { put("v0.4/traces") { def status = request.contentLength > 0 ? 200 : 500 - response.status(status).send('{"hello":"test"}') + response.status(status).send('{"hello":{}}') } } } @@ -142,7 +143,7 @@ class DDAgentApiTest extends DDSpecification { when: client.sendTraces([[], [], []]) then: - agentResponse.get() == '{"hello":"test"}' + agentResponse.get() == ["hello": [:]] agent.lastRequest.headers.get("Datadog-Meta-Lang") == "java" agent.lastRequest.headers.get("Datadog-Meta-Lang-Version") == System.getProperty("java.version", "unknown") agent.lastRequest.headers.get("Datadog-Meta-Tracer-Version") == "Stubbed-Test-Version" diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy index 46f65e7c0e..e0a0342eae 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy @@ -11,6 +11,8 @@ import datadog.trace.common.writer.ddagent.BatchWritingDisruptor import datadog.trace.common.writer.ddagent.DDAgentApi import datadog.trace.common.writer.ddagent.Monitor import datadog.trace.util.test.DDSpecification +import org.msgpack.core.MessagePack +import org.msgpack.core.buffer.ArrayBufferOutput import spock.lang.Retry import spock.lang.Timeout @@ -21,6 +23,7 @@ import java.util.concurrent.atomic.AtomicInteger import static datadog.opentracing.SpanFactory.newSpanOf import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer +import static datadog.trace.common.serialization.MsgpackFormatWriter.MSGPACK_WRITER import static datadog.trace.common.writer.DDAgentWriter.DISRUPTOR_BUFFER_SIZE @Timeout(20) @@ -206,7 +209,7 @@ class DDAgentWriterTest extends DDSpecification { Mock(DDTracer)) minimalSpan = new DDSpan(0, minimalContext) minimalTrace = [minimalSpan] - traceSize = DDAgentApi.OBJECT_MAPPER.writeValueAsBytes(minimalTrace).length + traceSize = calculateSize(minimalTrace) maxedPayloadTraceCount = ((int) (BatchWritingDisruptor.FLUSH_PAYLOAD_BYTES / traceSize)) + 1 } @@ -667,4 +670,12 @@ class DDAgentWriterTest extends DDSpecification { cleanup: writer.close() } + + static int calculateSize(List trace) { + def buffer = new ArrayBufferOutput() + def packer = MessagePack.newDefaultPacker(buffer) + MSGPACK_WRITER.writeTrace(trace, packer) + packer.flush() + return buffer.size + } } diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/LoggingWriterTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/LoggingWriterTest.groovy new file mode 100644 index 0000000000..63cacf0684 --- /dev/null +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/LoggingWriterTest.groovy @@ -0,0 +1,21 @@ +package datadog.trace.api.writer + + +import datadog.opentracing.DDTracer +import datadog.opentracing.SpanFactory +import datadog.trace.common.writer.LoggingWriter +import datadog.trace.util.test.DDSpecification +import spock.lang.Subject + +class LoggingWriterTest extends DDSpecification { + @Subject + def writer = new LoggingWriter() + + def tracer = Mock(DDTracer) + def sampleTrace = [SpanFactory.newSpanOf(tracer), SpanFactory.newSpanOf(tracer)] + + def "test toString"() { + expect: + writer.toString(sampleTrace).startsWith('[{"service":"fakeService","name":"fakeOperation","resource":"fakeResource","trace_id":1,"span_id":1,"parent_id":0,"start":1000,"duration":0,"type":"fakeType","error":0,"metrics":{},"meta":{') + } +} diff --git a/dd-trace-ot/src/traceAgentTest/groovy/DDApiIntegrationTest.groovy b/dd-trace-ot/src/traceAgentTest/groovy/DDApiIntegrationTest.groovy index ff793e9aaf..620e85044d 100644 --- a/dd-trace-ot/src/traceAgentTest/groovy/DDApiIntegrationTest.groovy +++ b/dd-trace-ot/src/traceAgentTest/groovy/DDApiIntegrationTest.groovy @@ -1,4 +1,3 @@ -import com.fasterxml.jackson.databind.JsonNode import datadog.opentracing.DDSpan import datadog.opentracing.DDSpanContext import datadog.opentracing.DDTracer @@ -63,11 +62,11 @@ class DDApiIntegrationTest { def unixDomainSocketApi def endpoint = new AtomicReference(null) - def agentResponse = new AtomicReference(null) + def agentResponse = new AtomicReference>>(null) - DDAgentResponseListener responseListener = { String receivedEndpoint, JsonNode responseJson -> + DDAgentResponseListener responseListener = { String receivedEndpoint, Map> responseJson -> endpoint.set(receivedEndpoint) - agentResponse.set(responseJson.toString()) + agentResponse.set(responseJson) } def setupSpec() { @@ -126,7 +125,7 @@ class DDApiIntegrationTest { api.sendTraces(traces) if (v4()) { assert endpoint.get() == "http://${agentContainerHost}:${agentContainerPort}/v0.4/traces" - assert agentResponse.get() == '{"rate_by_service":{"service:,env:":1}}' + assert agentResponse.get() == [rate_by_service: ["service:,env:": 1]] } where: @@ -147,7 +146,7 @@ class DDApiIntegrationTest { unixDomainSocketApi.sendTraces(traces) if (v4()) { assert endpoint.get() == "http://${SOMEHOST}:${SOMEPORT}/v0.4/traces" - assert agentResponse.get() == '{"rate_by_service":{"service:,env:":1}}' + assert agentResponse.get() == [rate_by_service: ["service:,env:": 1]] } where: diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index bea606ba2b..774c6d259a 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -8,9 +8,6 @@ ext { slf4j : "1.7.29", guava : "20.0", // Last version to support Java 7 - // When upgrading for security fixes, ensure corresponding change is reflected in jmxfetch. - jackson : "2.10.0", // https://nvd.nist.gov/vuln/detail/CVE-2019-16942 et al - spock : "1.3-groovy-$spockGroovyVer", groovy : groovyVer, logback : "1.2.3", @@ -34,10 +31,6 @@ ext { // General slf4j : "org.slf4j:slf4j-api:${versions.slf4j}", guava : "com.google.guava:guava:$versions.guava", - jackson : [ - dependencies.create(group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: versions.jackson), - dependencies.create(group: 'org.msgpack', name: 'jackson-dataformat-msgpack', version: '0.8.18'), - ], bytebuddy : dependencies.create(group: 'net.bytebuddy', name: 'byte-buddy', version: "${versions.bytebuddy}"), bytebuddyagent : dependencies.create(group: 'net.bytebuddy', name: 'byte-buddy-agent', version: "${versions.bytebuddy}"), autoservice : [ From cab139e90518217c3152b8edd3bbb8c09370c216 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 4 Feb 2020 13:06:32 -0800 Subject: [PATCH 13/17] Upgrade OkHttp client to 3.12.8 --- .../trace/agent/test/utils/OkHttpUtils.groovy | 20 -------------- .../trace/agent/test/utils/OkHttpUtils.java | 27 +++++++++++++++++++ .../src/test/groovy/server/ServerTest.groovy | 2 +- dd-trace-ot/dd-trace-ot.gradle | 2 +- gradle/dependencies.gradle | 6 +++-- 5 files changed, 33 insertions(+), 24 deletions(-) delete mode 100644 dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy create mode 100644 dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.java diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy deleted file mode 100644 index aa630a2c52..0000000000 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.groovy +++ /dev/null @@ -1,20 +0,0 @@ -package datadog.trace.agent.test.utils - -import okhttp3.OkHttpClient - -import java.util.concurrent.TimeUnit - -class OkHttpUtils { - - static clientBuilder() { - def unit = TimeUnit.MINUTES - new OkHttpClient.Builder() - .connectTimeout(1, unit) - .writeTimeout(1, unit) - .readTimeout(1, unit) - } - - static client(boolean followRedirects = false) { - clientBuilder().followRedirects(followRedirects).build() - } -} diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.java b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.java new file mode 100644 index 0000000000..e78633ed2d --- /dev/null +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/OkHttpUtils.java @@ -0,0 +1,27 @@ +package datadog.trace.agent.test.utils; + +import java.util.concurrent.TimeUnit; +import okhttp3.OkHttpClient; + +/** + * This class was moved from groovy to java because groovy kept trying to introspect on the + * OkHttpClient class which contains java 8 only classes, which caused the build to fail for java 7. + */ +public class OkHttpUtils { + + static OkHttpClient.Builder clientBuilder() { + final TimeUnit unit = TimeUnit.MINUTES; + return new OkHttpClient.Builder() + .connectTimeout(1, unit) + .writeTimeout(1, unit) + .readTimeout(1, unit); + } + + public static OkHttpClient client() { + return client(false); + } + + public static OkHttpClient client(final boolean followRedirects) { + return clientBuilder().followRedirects(followRedirects).build(); + } +} diff --git a/dd-java-agent/testing/src/test/groovy/server/ServerTest.groovy b/dd-java-agent/testing/src/test/groovy/server/ServerTest.groovy index 9761f01f6e..3d842bc8c3 100644 --- a/dd-java-agent/testing/src/test/groovy/server/ServerTest.groovy +++ b/dd-java-agent/testing/src/test/groovy/server/ServerTest.groovy @@ -231,7 +231,7 @@ class ServerTest extends AgentTestRunner { def "server redirect"() { setup: - client = OkHttpUtils.clientBuilder().followRedirects(followRedirects).build() + client = OkHttpUtils.client(followRedirects) def server = httpServer { handlers { get("/redirect") { diff --git a/dd-trace-ot/dd-trace-ot.gradle b/dd-trace-ot/dd-trace-ot.gradle index 256213b2d8..b4934dfb53 100644 --- a/dd-trace-ot/dd-trace-ot.gradle +++ b/dd-trace-ot/dd-trace-ot.gradle @@ -38,8 +38,8 @@ dependencies { compile group: 'com.datadoghq', name: 'java-dogstatsd-client', version: '2.1.1' compile deps.slf4j + compile deps.okhttp compile group: 'org.msgpack', name: 'msgpack-core', version: '0.8.20' - compile group: 'com.squareup.okhttp3', name: 'okhttp', version: '3.11.0' // Last version to support Java7 compile group: 'com.squareup.moshi', name: 'moshi', version: '1.9.2' compile group: 'com.github.jnr', name: 'jnr-unixsocket', version: '0.23' compile group: 'com.lmax', name: 'disruptor', version: '3.4.2' diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 774c6d259a..0634997c5c 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -7,6 +7,7 @@ ext { slf4j : "1.7.29", guava : "20.0", // Last version to support Java 7 + okhttp : "3.12.8", // 3.12.x is last version to support Java7) spock : "1.3-groovy-$spockGroovyVer", groovy : groovyVer, @@ -31,8 +32,9 @@ ext { // General slf4j : "org.slf4j:slf4j-api:${versions.slf4j}", guava : "com.google.guava:guava:$versions.guava", - bytebuddy : dependencies.create(group: 'net.bytebuddy', name: 'byte-buddy', version: "${versions.bytebuddy}"), - bytebuddyagent : dependencies.create(group: 'net.bytebuddy', name: 'byte-buddy-agent', version: "${versions.bytebuddy}"), + okhttp : dependencies.create(group: 'com.squareup.okhttp3', name: 'okhttp', version: versions.okhttp), + bytebuddy : dependencies.create(group: 'net.bytebuddy', name: 'byte-buddy', version: versions.bytebuddy), + bytebuddyagent : dependencies.create(group: 'net.bytebuddy', name: 'byte-buddy-agent', version: versions.bytebuddy), autoservice : [ dependencies.create(group: 'com.google.auto.service', name: 'auto-service', version: '1.0-rc3'), dependencies.create(group: 'com.google.auto', name: 'auto-common', version: '0.8'), From d0959782239f2492e6bebe02bea9e9b2d252bff7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9cile=20Terpin?= Date: Thu, 6 Feb 2020 17:02:17 +0100 Subject: [PATCH 14/17] Fix setError --- dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java index bf6984d6ff..71da4da33a 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java @@ -109,7 +109,7 @@ public class DDSpan implements Span, MutableSpan { @Override public DDSpan setError(final boolean error) { - context.setErrorFlag(true); + context.setErrorFlag(error); return this; } From 5265a43c6d140a17dbef2234251fa7ef916a673f Mon Sep 17 00:00:00 2001 From: Lev Priima Date: Thu, 6 Feb 2020 22:18:23 -0500 Subject: [PATCH 15/17] Reduce use of Thread::setContextClassLoader --- CONTRIBUTING.md | 4 ++-- .../java/datadog/trace/bootstrap/Agent.java | 8 -------- .../trace/agent/tooling/AgentInstaller.java | 3 ++- .../trace/agent/test/AgentTestRunner.java | 17 ++++++----------- 4 files changed, 10 insertions(+), 22 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ffc85ece5f..cec83b9b54 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -20,8 +20,8 @@ Other source files (Groovy, Scala, etc) should ideally be formatted by Intellij Suggested plugins and settings: * Editor > Code Style > Java/Groovy > Imports - * Class count to use import with '*': `50` (some number sufficiently large that is unlikely to matter) - * Names count to use static import with '*': `50` + * Class count to use import with '*': `9999` (some number sufficiently large that is unlikely to matter) + * Names count to use static import with '*': `9999` * With java use the following import layout (groovy should still use the default) to ensure consistency with google-java-format: ![import layout](https://user-images.githubusercontent.com/734411/43430811-28442636-94ae-11e8-86f1-f270ddcba023.png) * [Google Java Format](https://plugins.jetbrains.com/plugin/8527-google-java-format) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 4cfd50a828..512fcd0004 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -163,11 +163,9 @@ public class Agent { private static synchronized void startDatadogAgent( final Instrumentation inst, final URL bootstrapURL) { if (AGENT_CLASSLOADER == null) { - final ClassLoader contextLoader = Thread.currentThread().getContextClassLoader(); try { final ClassLoader agentClassLoader = createDatadogClassLoader("agent-tooling-and-instrumentation.isolated", bootstrapURL); - Thread.currentThread().setContextClassLoader(agentClassLoader); final Class agentInstallerClass = agentClassLoader.loadClass("datadog.trace.agent.tooling.AgentInstaller"); final Method agentInstallerMethod = @@ -176,8 +174,6 @@ public class Agent { AGENT_CLASSLOADER = agentClassLoader; } catch (final Throwable ex) { log.error("Throwable thrown while installing the Datadog Agent", ex); - } finally { - Thread.currentThread().setContextClassLoader(contextLoader); } } } @@ -186,11 +182,9 @@ public class Agent { if (AGENT_CLASSLOADER == null) { throw new IllegalStateException("Datadog agent should have been started already"); } - final ClassLoader contextLoader = Thread.currentThread().getContextClassLoader(); // TracerInstaller.installGlobalTracer can be called multiple times without any problem // so there is no need to have a 'datadogTracerInstalled' flag here. try { - Thread.currentThread().setContextClassLoader(AGENT_CLASSLOADER); // install global tracer final Class tracerInstallerClass = AGENT_CLASSLOADER.loadClass("datadog.trace.agent.tooling.TracerInstaller"); @@ -200,8 +194,6 @@ public class Agent { logVersionInfoMethod.invoke(null); } catch (final Throwable ex) { log.error("Throwable thrown while installing the Datadog Tracer", ex); - } finally { - Thread.currentThread().setContextClassLoader(contextLoader); } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java index 8af04df97e..0f466e1a49 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java @@ -143,7 +143,8 @@ public class AgentInstaller { agentBuilder = agentBuilder.with(listener); } int numInstrumenters = 0; - for (final Instrumenter instrumenter : ServiceLoader.load(Instrumenter.class)) { + for (final Instrumenter instrumenter : + ServiceLoader.load(Instrumenter.class, AgentInstaller.class.getClassLoader())) { log.debug("Loading instrumentation {}", instrumenter.getClass().getName()); try { diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/AgentTestRunner.java b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/AgentTestRunner.java index 6c93f4cc66..82cc227e0b 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/AgentTestRunner.java +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/AgentTestRunner.java @@ -139,20 +139,15 @@ public abstract class AgentTestRunner extends DDSpecification { } @BeforeClass - public static synchronized void agentSetup() throws Exception { + public static synchronized void agentSetup() { if (null != activeTransformer) { throw new IllegalStateException("transformer already in place: " + activeTransformer); } - - final ClassLoader contextLoader = Thread.currentThread().getContextClassLoader(); - try { - Thread.currentThread().setContextClassLoader(AgentTestRunner.class.getClassLoader()); - assert ServiceLoader.load(Instrumenter.class).iterator().hasNext() - : "No instrumentation found"; - activeTransformer = AgentInstaller.installBytebuddyAgent(INSTRUMENTATION, TEST_LISTENER); - } finally { - Thread.currentThread().setContextClassLoader(contextLoader); - } + assert ServiceLoader.load(Instrumenter.class, AgentTestRunner.class.getClassLoader()) + .iterator() + .hasNext() + : "No instrumentation found"; + activeTransformer = AgentInstaller.installBytebuddyAgent(INSTRUMENTATION, TEST_LISTENER); INSTRUMENTATION_ERROR_COUNT.set(0); } From 7e62bca7fe1144eb445e6a484b25618d077f5721 Mon Sep 17 00:00:00 2001 From: heathkd Date: Fri, 7 Feb 2020 17:57:34 -0500 Subject: [PATCH 16/17] limit hibernate latest dependency test to 5.x --- .../instrumentation/hibernate/core-4.3/core-4.3.gradle | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/instrumentation/hibernate/core-4.3/core-4.3.gradle b/dd-java-agent/instrumentation/hibernate/core-4.3/core-4.3.gradle index b937b53fbb..334cfb47a1 100644 --- a/dd-java-agent/instrumentation/hibernate/core-4.3/core-4.3.gradle +++ b/dd-java-agent/instrumentation/hibernate/core-4.3/core-4.3.gradle @@ -32,8 +32,8 @@ dependencies { testCompile group: 'org.hsqldb', name: 'hsqldb', version: '2.0.0' testCompile group: 'org.springframework.data', name: 'spring-data-jpa', version: '1.5.1.RELEASE' - latestDepTestCompile group: 'org.hibernate', name: 'hibernate-core', version: '5.+' - latestDepTestCompile group: 'org.hibernate', name: 'hibernate-entitymanager', version: '5.+' + latestDepTestCompile group: 'org.hibernate', name: 'hibernate-core', version: '(,6.0.0.Final)' + latestDepTestCompile group: 'org.hibernate', name: 'hibernate-entitymanager', version: '(,6.0.0.Final)' latestDepTestCompile group: 'org.hsqldb', name: 'hsqldb', version: '2.0.0' latestDepTestCompile group: 'org.springframework.data', name: 'spring-data-jpa', version: '+' } From 66400c9b3721729385faa32153307733d94d0e9b Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 7 Feb 2020 15:58:49 -0800 Subject: [PATCH 17/17] Fix NPE on serialization with no span type Add tests. --- .../serialization/MsgpackFormatWriter.java | 12 +++++++-- .../DDSpanSerializationTest.groovy | 26 +++++++++---------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/serialization/MsgpackFormatWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/serialization/MsgpackFormatWriter.java index ca5579da7e..f892c80f6b 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/serialization/MsgpackFormatWriter.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/serialization/MsgpackFormatWriter.java @@ -32,7 +32,11 @@ public class MsgpackFormatWriter extends FormatWriter { public void writeString(final String key, final String value, final MessagePacker destination) throws IOException { destination.packString(key); - destination.packString(value); + if (value == null) { + destination.packNil(); + } else { + destination.packString(value); + } } @Override @@ -82,6 +86,10 @@ public class MsgpackFormatWriter extends FormatWriter { final String key, final BigInteger value, final MessagePacker destination) throws IOException { destination.packString(key); - destination.packBigInteger(value); + if (value == null) { + destination.packNil(); + } else { + destination.packBigInteger(value); + } } } diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanSerializationTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanSerializationTest.groovy index 8b5e04a336..7e3ee405b2 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanSerializationTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanSerializationTest.groovy @@ -33,7 +33,7 @@ class DDSpanSerializationTest extends DDSpecification { parent_id: 0l, start : 100000, duration : 33000, - type : "type", + type : spanType, error : 0, metrics : metrics, meta : [ @@ -58,7 +58,7 @@ class DDSpanSerializationTest extends DDSpecification { null, ["a-baggage": "value"], false, - "type", + spanType, ["k1": "v1"], new PendingTrace(tracer, 1G, [:]), tracer) @@ -73,9 +73,9 @@ class DDSpanSerializationTest extends DDSpecification { actualTree == expectedTree where: - samplingPriority | _ - PrioritySampling.SAMPLER_KEEP | _ - PrioritySampling.UNSET | _ + samplingPriority | spanType + PrioritySampling.SAMPLER_KEEP | null + PrioritySampling.UNSET | "some-type" } def "serialize trace/span with id #value as int"() { @@ -93,7 +93,7 @@ class DDSpanSerializationTest extends DDSpecification { null, Collections.emptyMap(), false, - "fakeType", + spanType, Collections.emptyMap(), new PendingTrace(tracer, 1G, [:]), tracer) @@ -122,12 +122,12 @@ class DDSpanSerializationTest extends DDSpecification { } where: - value | _ - 0G | _ - 1G | _ - 8223372036854775807G | _ - BigInteger.valueOf(Long.MAX_VALUE).subtract(1G) | _ - BigInteger.valueOf(Long.MAX_VALUE).add(1G) | _ - 2G.pow(64).subtract(1G) | _ + value | spanType + 0G | null + 1G | "some-type" + 8223372036854775807G | null + BigInteger.valueOf(Long.MAX_VALUE).subtract(1G) | "some-type" + BigInteger.valueOf(Long.MAX_VALUE).add(1G) | null + 2G.pow(64).subtract(1G) | "some-type" } }