Add setting a tag at the trace-level #1959
Conversation
lloeki
force-pushed
the
add-trace-set-tag
branch
from
20d0e7c
to
9bb182d
Compare
lloeki
force-pushed
the
add-trace-set-tag
branch
2 times, most recently
from
a03d7cd
to
1da0d11
Compare
Since trace transport does not have a concept of trace, tags are set on the topmost span. When partial flushing is involved, the topmost span will be sent upon last flush, therefore tags will only appear in the UI when that final segment is processed.
TraceOperation is largely oblivious to the root span concept. The lack of a concept of trace is part of the Trace API transport, therefore TraceFormatter is more appropriate to carry and decide where to put the trace-level values. Trace tags explicitly do not carry to partially flushed traces in order to avoid double processing.
lloeki
force-pushed
the
add-trace-set-tag
branch
from
1da0d11
to
aeaceef
Compare
lloeki
force-pushed
the
add-trace-set-tag
branch
from
d04f773
to
92ee73c
Compare
TraceOperation passes its tags unconditionally, therefore TraceSegment now carries trace tags all the way to TraceFormatter, which has the responsibility to decide whether to apply them or not to one of the spans.
lloeki
force-pushed
the
add-trace-set-tag
branch
from
92ee73c
to
8ea7720
Compare
This makes explicit the condition laid out in the comments
lib/datadog/tracing/trace_segment.rb
Outdated
| end | ||
|
|
||
| def hostname=(value) | ||
| if value.nil? | ||
| tags.delete(Metadata::Ext::NET::TAG_HOSTNAME) | ||
| clear_tag(Metadata::Ext::NET::TAG_HOSTNAME) |
There was a problem hiding this comment.
I noticed that clear_tag stands out as it only clears keys in meta, whereas both set_tag and get_tag operate on both metrics and meta, as well as tags arguments being able to contain both metrics and meta.
Maybe we should align clear_tag and make it clear the key on both meta and metrics.
lib/datadog/tracing/trace_segment.rb
Outdated
| return | ||
| end | ||
|
|
||
| tags[Metadata::Ext::NET::TAG_HOSTNAME] = value | ||
| set_tag(Metadata::Ext::NET::TAG_HOSTNAME, value) |
There was a problem hiding this comment.
Here I wonder if we should have a slightly stronger typed set_meta, which would not silently allow these keys to land into metrics when they are known not to be metrics.
| @@ -59,7 +59,7 @@ def set_resource! | |||
| # If the trace resource is undefined, or the root span wasn't | |||
| # specified, don't set this. We don't want to overwrite the | |||
| # resource of a span that is in the middle of the trace. | |||
| return if trace.resource.nil? || !@found_root_span | |||
| return if trace.resource.nil? || partial? | |||
There was a problem hiding this comment.
@found_root_span kind of comes from TraceSegment.root_span_id, which comes from voluntarily not passing root_span.id in TraceOperation when it is known partial (from TraceOperation#finished?), so @found_root_span seems like a roundabout way to infer that this is a partial trace.
I wonder if we should just pass the finished?/partial information via TraceSegment instead of relying on second-order divination.
In fact, it wasn't clear to me whether resource being nil was supposed to be part of that partial? predicate or not, but either way it does feel like divination from external behaviour.
| @@ -165,6 +165,10 @@ def tag_sampling_priority! | |||
|
|
|||
| private | |||
|
|
|||
| def partial? | |||
There was a problem hiding this comment.
Can't comment on the line below but it turns out this comment I added because things were unclear to me and I had to retrace logic across three classes might just vanish by using this predicate.
# when root span is not found, fall back to last span (partial flush)
root_span || trace.spans.last
This keeps the meta/metrics split but removes the Metadata::Tagging behaviour which is redundantly executed (TraceOperation is supposed to having handled that). `meta` and `metrics` become protected accessors and internal details. Additionally, setting meta and metrics is split, avoiding a hash merge.
lloeki
force-pushed
the
add-trace-set-tag
branch
from
cadffcc
to
942570e
Compare
Also makes writers private since TraceSegment is supposed to be immutable from the outside.
lloeki
force-pushed
the
add-trace-set-tag
branch
from
b74615e
to
f92df7e
Compare
The previous attempt had the issue of always falling back to the RHS of or-assignments when the values are `nil`.
Even though they are not used anymore after initialization, they are also harmless since they're publicly invisible.
delner
added
core
Since trace transport does not have a concept of trace, tags are set on the topmost span.
When partial flushing is involved, the topmost span will be sent upon last flush, therefore tags will only appear in the UI when that final segment is processed.