Support long running spans#3452
Conversation
59850da to
a75ab92
Compare
devinsba
left a comment
devinsba
left a comment
There was a problem hiding this comment.
We need to address the pendingTraces map
| public static final long MAX_SPAN_AGE_NANOS = TimeUnit.HOURS.toNanos(12); | ||
|
|
||
| private final Object dummy = new Object(); | ||
| private final ConcurrentMap<PendingTrace, Object> pendingTraces = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
We can't have this be an unbounded size. If there is a bug (in the tracer or manual tracing code) that causes traces to not get finished we would rather have those get garbage collected and lost then to effectively cause a memory leak
| span.setPartialVersion(partialVersion); | ||
| span.setMetric(DDTags.DD_PARTIAL_VERSION, partialVersion); |
There was a problem hiding this comment.
Instead of storing this twice we should convert the field to a metric when writing the span out
There was a problem hiding this comment.
+1 - I don't think should even be a tag / metric on the span. Logically, partialness is just transport concept. It doesn't really exist in the current data model.
There was a problem hiding this comment.
Maybe putting in the metadata like measured or top_level could be a better option so?
|
|
||
| int samplingPriority(); | ||
|
|
||
| Long getPartialVersion(); |
There was a problem hiding this comment.
I don't think I want partially pushed onto the span itself. It isn't really a data model concept. It is a transport concept.
| private final HttpCodec.Extractor extractor; | ||
|
|
||
| private final InstrumentationGateway instrumentationGateway; | ||
| private final TraceKeepAlive traceKeepAlive; |
There was a problem hiding this comment.
I don't think we need another object for this. I was thinking it would be handled inside of PendingTrace.
There was a problem hiding this comment.
Do you prefer having PendingTrace handling a scheduled job?
| * A monitor thread scheduled on a regular basis. It triggers keep-alive spans to be sent for | ||
| * long-running traces. | ||
| */ | ||
| public class TraceKeepAlive implements AgentTaskScheduler.Task<Boolean> { |
There was a problem hiding this comment.
As noted elsewhere, I don't think we need a separate class for this. I think this would better be handled inside of PendingTrace.
| return null == mapped ? serviceName : mapped; | ||
| } | ||
|
|
||
| boolean isDisableSamplingMechanismValidation() { |
There was a problem hiding this comment.
| return durationNano != 0; | ||
| } | ||
|
|
||
| public Long getPartialVersion() { |
There was a problem hiding this comment.
These shouldn't be part of the public span API. I don't think should be on span at all.
|
|
||
| public void processTagsAndBaggage(final MetadataConsumer consumer) { | ||
| synchronized (unsafeTags) { | ||
| final boolean incomplete = unsafeTags.containsKey(DDTags.DD_PARTIAL_VERSION); |
There was a problem hiding this comment.
We shouldn't be using a tag for partialness. It isn't really a tag in the data model -- that's just a trick to fit into the existing protocol.
| return null != span ? span.context() : this; | ||
| } | ||
|
|
||
| String getParentServiceName() { |
There was a problem hiding this comment.
Why are we adding these? I don't think this is the right place for them.
There was a problem hiding this comment.
Because this information is needed to construct a new SpanContext here: https://github.com/DataDog/dd-trace-java/blob/7b5d30060e0d3bc2218bc230809440a831fd8522/dd-trace-core/src/main/java/datadog/trace/core/PendingTrace.java#L201
I gave only package visibility. Maybe something can be simplified. Please advice
| this.strictTraceWrites = strictTraceWrites; | ||
|
|
||
| if (tracer.getTraceKeepAlive() != null) { | ||
| tracer.getTraceKeepAlive().onPendingTraceBegins(this); |
There was a problem hiding this comment.
This is mostly confirming that should just be part of PendingTrace.
There was a problem hiding this comment.
As far I can see, on PendingTrace I would have the possibility to emit keepalives when write is called (e.g. a span of the same trace is finished). But what in case nothing happens and I've just to keep alive an unfinished span? I don't see any other solution than using a scheduled thread. The advantage of externalizing is just use one thread for all the pending traces and separate roles
There was a problem hiding this comment.
This could be done as part of the existing writer loop in datadog.trace.core.PendingTraceBuffer.DelayingPendingTraceBuffer.Worker so we wouldn't need a new thread.
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
I've commented on this PR, but I'd really suggest that the PR just be abandoned.
I think we need to back and talk about the protocol and how to make the changes more before jumping into code.
7b5d300 to
9e6265f
Compare
0c360bf to
9d5d72d
Compare
9d5d72d to
b19684f
Compare
3 participants
What Does This Do
Add support for long running spans. Allows incomplete spans to be reported to datadog and partial results to be displayed into the live view.
As well, this PR enables collecting complete traces for spans lasting more than 15 minutes.
The feature is released behind a configuration flag and it's disabled by default.
How does it work
Pending traces are scanned on a scheduled basis and if a long lasting span is found:
Motivation
Today, due to internal datadog architecture, spans lasting more than 15 minutes may result in incomplete tracing. Additionally to this, spans are sent only when all pending ones are closed. Hence, live view won't display those ones.
Additional Notes