Updated implementation of partial trace flushing #845
Conversation
marcotc
added
core
marcotc
force-pushed
the
feat/partial-flushing
branch
from
0af1b42
to
13d2068
Compare
marcotc
force-pushed
the
feat/partial-flushing
branch
from
4809de2
to
7b4907b
Compare
marcotc
changed the title
| @@ -199,15 +250,15 @@ def check_finished_spans | |||
| @finished_spans > 0 && @trace.length == @finished_spans | |||
| end | |||
|
|
|||
| def attach_sampling_priority | |||
| @current_root_span.set_metric( | |||
| def attach_sampling_priority(span) | |||
There was a problem hiding this comment.
Perhaps we could extract these functions + configure_root_span to some kind of Flush base for Finished and Partial; these functions are used only for flushing, and don't describe something generally useful for Contexts.
There was a problem hiding this comment.
Because Context#get calls reset when returning a complete trace, we can't call this method after the fact. There's no good way really to move this logic away from Context given today's state.
I think a good solution is to make the Context "less smart", as methods like #get have a lot of logic in it that could be extracted to be ContextFlush responsibilities, like you suggested. That has to be done in a thread-safe way, respecting the possibility that Context is reused on the same thread after reset is called.
This to me looks like a separate, larger refactor in its own merit. I don't think the changes in PR go against that goal, on the contrary, but do I agree that this PR doesn't fully address the abstraction issue that currently existing with the Context.
I'd like to go ahead with the scope of this PR and schedule this much needed refactor for the near future.
marcotc
force-pushed
the
feat/partial-flushing
branch
from
b1fc6ea
to
9973188
Compare
marcotc
force-pushed
the
feat/partial-flushing
branch
from
5d56cde
to
4ac7b79
Compare
|
All comments should have been addressed, ready for review. |
There was a problem hiding this comment.
This looks really good, and I think we're pretty much done. Just one minor clarifying question about naming then I think its ready to merge.
Given our reflection during this refactor, as we go forward, I think we might want to consider extracting more complexity from the Context and the Tracer as we discussed before. I think this PR makes a good first step, but it would be prudent to ensure the coupling between these components and other processes (such as flushing) is reduced to keep these components more maintainable.
This PR updates partial trace flushing to conform with current requirements of the Datadog agent and processing back-end.
Most restrictions have been lifted regarding flushing partial traces, allowing for a much simpler implementation.
Finished spans from a trace can now be flushed immediately, regardless of their place on the trace tree hierarchy.