Add option to inherit service name from the parent span #3685

marcotc · 2024-06-05T19:37:40Z

What does this PR do?

This PR is the first step towards aligning the Ruby tracer with all other APM languages in inheriting the parent span's service name by default.

Making this change is a breaking change, so this is introduced as an option which default to "disabled".

This option should be made "enabled" by default in the next major version.

How to test the change?

Changing the setting c.tracing.inherit_parent_service to true allows you to see the new behaviour, with the span's service inheriting the value from the active parent span.

marcotc · 2024-06-06T22:17:28Z

lib/datadog/tracing/tracer.rb

@@ -341,12 +347,13 @@ def bind_trace_events!(trace_op)
        events = trace_op.send(:events)

        events.span_before_start.subscribe do |event_span_op, event_trace_op|
-          event_trace_op.service ||= @default_service


A small improvement here: we used to always set @default_service as the fallback service for a the newly created TraceOperation. By explicity setting the value of event_trace_op.service here, the TraceOperation can no longer use the root span service name as its default value, which is the intended behavior:

dd-trace-rb/lib/datadog/tracing/trace_operation.rb

Lines 171 to 173 in b3c5a3d

def service

@service || (root_span && root_span.service)

end

This PR no longer explicitly sets the service field in the active TraceOperation, letting event_trace_op.service handle the fallback normally, as it was originally intended.

marcotc · 2024-06-06T22:19:37Z

lib/datadog/tracing/tracer.rb

-          event_span_op.service ||= @default_service
          sample_trace(event_trace_op) if event_span_op && event_span_op.parent_id == 0
        end

        events.span_finished.subscribe do |event_span, event_trace_op|
+          # Fallback in case the service was never set
+          event_span.service ||= @default_service


I moved the fallback event_span.service ||= @default_service from the span creation event to span completion evnt, because semantically a nil span service name has the meaning of "use whatever the default behaviour is for span service naming". But before, we were not allowing this to happen because we are setting the service name explicitly too early, in the span_before_start event.

codecov-commenter · 2024-06-06T22:26:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.10%. Comparing base (5a95611) to head (c2aea03).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3685   +/-   ##
=======================================
  Coverage   98.10%   98.10%           
=======================================
  Files        1225     1225           
  Lines       72846    72927   +81     
  Branches     3489     3490    +1     
=======================================
+ Hits        71463    71547   +84     
+ Misses       1383     1380    -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cswatt

approved for docs

TonyCTHsu · 2024-06-10T11:24:17Z

spec/datadog/tracing/tracer_spec.rb


-          expect(spans).to have(3).items
+        context 'when inherit_parent_service is false' do
+          it 'propagates parent span and uses default service name' do


Be explicit with conditions and useful when flipping the defaults

let(:tracer_options) { super().merge(inherit_parent_service: false) }

TonyCTHsu · 2024-06-10T11:27:05Z

spec/datadog/tracing/tracer_spec.rb

@@ -259,29 +273,55 @@
      end

      context 'when nesting spans' do
-        it 'propagates parent span and uses default service name' do
+        subject!(:nested_spans) do


Can we remove ! here?

This brings confusion to me with the ordering of execution, such as "Would this be executed before the let in the nested context"?

TonyCTHsu · 2024-06-10T11:31:26Z

docs/GettingStarted.md

@@ -1935,6 +1935,7 @@ For example, if `tracing.sampling.default_rate` is configured by [Remote Configu
 | `tracing.propagation_style`                            | `DD_TRACE_PROPAGATION_STYLE`                            | `Array`                               | `nil`                        | Distributed tracing propagation formats to extract and inject. See [Distributed Tracing](#distributed-tracing) for more details.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                       |
 | `tracing.enabled`                                      | `DD_TRACE_ENABLED`                                      | `Bool`                                | `true`                       | Enables or disables tracing. If set to `false` instrumentation will still run, but no traces are sent to the trace agent.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              |
 | `tracing.header_tags`                                  | `DD_TRACE_HEADER_TAGS`                                  | `Array`                               | `nil`                        | Record HTTP headers as span tags. See [Applying header tags to root spans][header tags] for more information.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                          |
+| `tracing.inherit_parent_service`                       |                                                         | `Bool`                                | `false`                      | Inherit the parent span's service name when creating a new child span. |


Should we be more explicit about this option is not ready for production yet?

And it might leads to breaking changes for user existing graph with various instrumentations

TonyCTHsu · 2024-06-10T11:42:36Z

lib/datadog/tracing/span_operation.rb

@@ -439,6 +443,11 @@ def message
      # mutation by reference; when this span is returned,
      # we don't want this SpanOperation to modify it further.
      def build_span
+        # Use parent service if no service is set and @inherit_parent_service is set


I am generally supportive for moving toward this direction. What I found a larger problem is the fact that service is a mutable field.

Things get tricky when parent span changes its service after child spans are finished. For instance:

Datadog::Tracing.trace('parent', service: 'service-parent') do |span_op, _| Datadog::Tracing.trace('child1') {} span_op.service = "BalaBoom!" # Change the service name after the child span is finished end

I was not sure what the behaviour is like for instrumentation depending on ActiveSupport::Notifications, which I remember the service field is set in the after callback.

Any thoughts about restricting the mutability of the service field?

I agree with Tony; mutability is a problem. Generally speaking, I think moving towards an immutable world simplifies things quite a bit, the only question is how, and what kinds of trade-offs. Have to think about this more.

Secondary question: how does this feature fit in with the "one service name" change we originally planned for 2.0? If we end up introducing that, is this obsolete?

delner · 2024-06-10T20:01:03Z

lib/datadog/tracing/span_operation.rb

+        # Use parent service if no service is set and @inherit_parent_service is set
+        service = self.service || (@parent&.service if @inherit_parent_service)
+
+        service_entry = @parent.nil? || (service && @parent.service != service)


What is @parent? It concerns me that SpanOperation has a reference to parent, because of the deep coupling/nesting... more references means less garbage collection means more memory bloat. It also makes it harder to move data structures like this off the critical path to a background process, if there was an optimization opportunity to do so.

I know my original intention was to eliminate @parent altogether. But I can't remember if we couldn't and if I just had to minimize its usage. I'm not hot on the idea of expanding its usage...

p-datadog

The lib part of the diff LGTM

Signed-off-by: Marco Costa <[email protected]>

github-actions bot added the tracing label Jun 5, 2024

marcotc self-assigned this Jun 5, 2024

marcotc force-pushed the inherit-parent-service branch 3 times, most recently from f45244f to fe9c4a8 Compare June 6, 2024 22:12

marcotc commented Jun 6, 2024

View reviewed changes

marcotc marked this pull request as ready for review June 6, 2024 22:22

marcotc requested review from a team as code owners June 6, 2024 22:22

cswatt approved these changes Jun 7, 2024

View reviewed changes

TonyCTHsu reviewed Jun 10, 2024

View reviewed changes

delner reviewed Jun 10, 2024

View reviewed changes

p-datadog approved these changes Jun 10, 2024

View reviewed changes

Add option to inherit service name from the parent span

c2aea03

Signed-off-by: Marco Costa <[email protected]>

marcotc force-pushed the inherit-parent-service branch from fe9c4a8 to c2aea03 Compare June 14, 2024 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to inherit service name from the parent span #3685

Add option to inherit service name from the parent span #3685

marcotc commented Jun 5, 2024 •

edited

Loading

marcotc Jun 6, 2024

marcotc Jun 6, 2024

codecov-commenter commented Jun 6, 2024 •

edited

Loading

cswatt left a comment

TonyCTHsu Jun 10, 2024

TonyCTHsu Jun 10, 2024

TonyCTHsu Jun 10, 2024

TonyCTHsu Jun 10, 2024

delner Jun 10, 2024 •

edited

Loading

delner Jun 10, 2024

delner Jun 10, 2024

p-datadog left a comment

Add option to inherit service name from the parent span #3685

Are you sure you want to change the base?

Add option to inherit service name from the parent span #3685

Conversation

marcotc commented Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 6, 2024 • edited Loading

Codecov Report

cswatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-datadog left a comment

Choose a reason for hiding this comment

marcotc commented Jun 5, 2024 •

edited

Loading

codecov-commenter commented Jun 6, 2024 •

edited

Loading

delner Jun 10, 2024 •

edited

Loading