Add setting a tag at the trace-level #1959

lloeki · 2022-04-04T10:28:18Z

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.

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.

delner

Generally speaking this is what I was expecting. Shared a few points that likely need to be addressed but looking good so far.

lib/datadog/tracing/trace_operation.rb

lib/datadog/appsec/event.rb

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.

This makes explicit the condition laid out in the comments

lib/datadog/tracing/trace_operation.rb

lib/datadog/tracing/trace_segment.rb

lib/ddtrace/transport/trace_formatter.rb

lloeki · 2022-04-12T21:48:31Z

lib/datadog/tracing/trace_segment.rb

      end

      def hostname=(value)
        if value.nil?
-          tags.delete(Metadata::Ext::NET::TAG_HOSTNAME)
+          clear_tag(Metadata::Ext::NET::TAG_HOSTNAME)


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.

lloeki · 2022-04-12T21:54:14Z

lib/datadog/tracing/trace_segment.rb

          return
        end

-        tags[Metadata::Ext::NET::TAG_HOSTNAME] = value
+        set_tag(Metadata::Ext::NET::TAG_HOSTNAME, value)


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.

lloeki · 2022-04-12T22:02:32Z

lib/ddtrace/transport/trace_formatter.rb

@@ -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?


@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.

lloeki · 2022-04-12T22:06:00Z

lib/ddtrace/transport/trace_formatter.rb

@@ -165,6 +165,10 @@ def tag_sampling_priority!

      private

+      def partial?


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.

Also makes writers private since TraceSegment is supposed to be immutable from the outside.

delner

Looking pretty good, but one suggestion about managing attributes/tags in TraceSegment.

lib/datadog/tracing/trace_segment.rb

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

Looks good. Thanks!

lloeki mentioned this pull request Apr 4, 2022

Add user attribution API #1954

Merged

lloeki force-pushed the add-trace-set-tag branch from 20d0e7c to 9bb182d Compare April 4, 2022 12:24

lloeki marked this pull request as ready for review April 4, 2022 12:30

lloeki requested a review from a team April 4, 2022 12:30

lloeki force-pushed the add-trace-set-tag branch 2 times, most recently from a03d7cd to 1da0d11 Compare April 5, 2022 12:45

lloeki added 5 commits April 6, 2022 13:16

Remove references to root span from AppSec

700b398

Add missing spec cases

958f95e

Add #get_tag

aeaceef

lloeki force-pushed the add-trace-set-tag branch from 1da0d11 to aeaceef Compare April 6, 2022 11:34

delner reviewed Apr 6, 2022

View reviewed changes

lib/datadog/tracing/trace_operation.rb Outdated Show resolved Hide resolved

lib/datadog/tracing/trace_operation.rb Outdated Show resolved Hide resolved

lib/datadog/appsec/event.rb Outdated Show resolved Hide resolved

Leverage Metadata::Analytics on TraceOperation

90614bb

lloeki force-pushed the add-trace-set-tag branch from d04f773 to 92ee73c Compare April 12, 2022 19:47

Move partial trace tagging condition to TraceFormatter

8ea7720

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 April 12, 2022 19:48

lloeki added 6 commits April 12, 2022 21:59

Split set_error out of Metadata::Tagging

24a294d

Add only tagging behaviour to TraceOperation

643f0f0

Stop using send for public Metadata::Tagging methods

7a52171

Use Metadata::Tagging for TraceSegment

b53631a

Materialize 'middle-of-trace' semantics for TraceSegment

ebd12ee

This makes explicit the condition laid out in the comments

Remove redundant conversion to UTF-8

14d10b6

lloeki commented Apr 12, 2022

View reviewed changes

lloeki force-pushed the add-trace-set-tag branch from cadffcc to 942570e Compare April 14, 2022 10:16

lloeki added 3 commits April 14, 2022 13:22

Ensure TraceSegment attribute values can come from tags

40c36af

Use TraceSegment instance variables instead of hashes

f4ff9df

Also makes writers private since TraceSegment is supposed to be immutable from the outside.

Fix overlooked reference to previous tag instance variable

f92df7e

lloeki force-pushed the add-trace-set-tag branch from b74615e to f92df7e Compare April 14, 2022 14:40

Address linter issues

8b8fc7b

delner reviewed Apr 14, 2022

View reviewed changes

lib/datadog/tracing/trace_segment.rb Outdated Show resolved Hide resolved

lib/datadog/tracing/trace_segment.rb Outdated Show resolved Hide resolved

lloeki added 3 commits April 14, 2022 17:40

Attempt symplifying initializer

3f63726

Simplify TraceSegment initializer

b70c401

The previous attempt had the issue of always falling back to the RHS of or-assignments when the values are `nil`.

Keep known tags in TraceSegment

1ee09fe

Even though they are not used anymore after initialization, they are also harmless since they're publicly invisible.

delner assigned lloeki Apr 14, 2022

delner added core Involves Datadog core libraries feature Involves a product feature labels Apr 14, 2022

delner added this to the 1.0.0.beta2 milestone Apr 14, 2022

delner approved these changes Apr 14, 2022

View reviewed changes

delner merged commit 4022d99 into master Apr 14, 2022

delner deleted the add-trace-set-tag branch April 14, 2022 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setting a tag at the trace-level #1959

Add setting a tag at the trace-level #1959

lloeki commented Apr 4, 2022

delner left a comment

lloeki Apr 12, 2022

lloeki Apr 12, 2022

lloeki Apr 12, 2022

lloeki Apr 12, 2022

delner left a comment

delner left a comment

		@@ -165,6 +165,10 @@ def tag_sampling_priority!

		private

		def partial?

Add setting a tag at the trace-level #1959

Add setting a tag at the trace-level #1959

Conversation

lloeki commented Apr 4, 2022

delner left a comment

Choose a reason for hiding this comment

lloeki Apr 12, 2022

Choose a reason for hiding this comment

lloeki Apr 12, 2022

Choose a reason for hiding this comment

lloeki Apr 12, 2022

Choose a reason for hiding this comment

lloeki Apr 12, 2022

Choose a reason for hiding this comment

delner left a comment

Choose a reason for hiding this comment

delner left a comment

Choose a reason for hiding this comment