Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/GettingStarted.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

| `tracing.instrument(<integration-name>, <options...>)` | | | | Activates instrumentation for a specific library. See [Integration instrumentation](#integration-instrumentation) for more details. |
| `tracing.log_injection` | `DD_LOGS_INJECTION` | `Bool` | `true` | Injects [Trace Correlation](#trace-correlation) information into Rails logs if present. Supports the default logger (`ActiveSupport::TaggedLogging`), `lograge`, and `semantic_logger`. |
| `tracing.partial_flush.enabled` | | `Bool` | `false` | Enables or disables partial flushing. Partial flushing submits completed portions of a trace to the agent. Used when tracing instruments long running tasks (e.g. jobs) with many spans. |
Expand Down
1 change: 1 addition & 0 deletions lib/datadog/tracing/component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ def build_tracer(settings, agent_settings, logger:)
span_sampler: build_span_sampler(settings),
writer: writer,
tags: build_tracer_tags(settings),
inherit_parent_service: settings.tracing.inherit_parent_service
)
end

Expand Down
15 changes: 15 additions & 0 deletions lib/datadog/tracing/configuration/settings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,21 @@ def self.extended(base)
o.setter { |header_tags, _| Configuration::HTTP::HeaderTags.new(header_tags) }
end

# Whether spans inherit their parent's service name, when span `service` is not explicitly set.
# If `true`, the spans will use the service name from its parent span.
# If `false`, the spans will use the global service name (`c.service`).
#
# DEV3.0: All other tracer languages inherit the parent's service by default.
# DEV3.0: This option should be set to `true` by default in the next major version,
# DEV3.0: with a removal deprecation warning, then removed in 4.0.
#
# @default `false`
# @return [Boolean]
option :inherit_parent_service do |o|
o.default false
o.type :bool
end

# Enable 128 bit trace id generation.
#
# @default `DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED` environment variable, otherwise `true`
Expand Down
17 changes: 13 additions & 4 deletions lib/datadog/tracing/span_operation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ def initialize(
tags: nil,
trace_id: nil,
type: nil,
links: nil
links: nil,
inherit_parent_service: false
)
# Ensure dynamically created strings are UTF-8 encoded.
#
Expand Down Expand Up @@ -90,6 +91,8 @@ def initialize(
# Subscribe :on_error event
@events.on_error.wrap_default(&on_error) if on_error.is_a?(Proc)

@inherit_parent_service = inherit_parent_service

# Start the span with start time, if given.
start(start_time) if start_time
end
Expand Down Expand Up @@ -428,7 +431,8 @@ def message
# it has been finished.
attr_reader \
:events,
:span
:span,
:inherit_parent_service

# Stored only for `service_entry` calculation.
# Use `parent_id` for the effective parent span id.
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@delner delner Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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?

service = self.service || (@parent&.service if @inherit_parent_service)

service_entry = @parent.nil? || (service && @parent.service != service)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Span.new(
@name,
duration: duration,
Expand All @@ -448,13 +457,13 @@ def build_span
metrics: Core::Utils::SafeDup.frozen_or_dup(metrics),
parent_id: @parent_id,
resource: @resource,
service: @service,
service: service,
start_time: @start_time,
status: @status,
type: @type,
trace_id: @trace_id,
links: @links,
service_entry: parent.nil? || (service && parent.service != service)
service_entry: service_entry
)
end

Expand Down
11 changes: 8 additions & 3 deletions lib/datadog/tracing/trace_operation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,19 +70,22 @@ def initialize(
sampled: nil,
sampling_priority: nil,
service: nil,
default_service: nil,
profiling_enabled: nil,
tags: nil,
metrics: nil,
trace_state: nil,
trace_state_unknown_fields: nil,
remote_parent: false
remote_parent: false,
inherit_parent_service: false
)
# Attributes
@id = id || Tracing::Utils::TraceId.next_id
@max_length = max_length || DEFAULT_MAX_LENGTH
@parent_span_id = parent_span_id
@sampled = sampled.nil? ? true : sampled
@remote_parent = remote_parent
@inherit_parent_service = inherit_parent_service

# Tags
@agent_sample_rate = agent_sample_rate
Expand All @@ -95,6 +98,7 @@ def initialize(
@sample_rate = sample_rate
@sampling_priority = sampling_priority
@service = service
@default_service = default_service
@profiling_enabled = profiling_enabled
@trace_state = trace_state
@trace_state_unknown_fields = trace_state_unknown_fields
Expand Down Expand Up @@ -169,7 +173,7 @@ def resource_override?
end

def service
@service || (root_span && root_span.service)
@service || (root_span && root_span.service) || @default_service
end

def measure(
Expand Down Expand Up @@ -249,7 +253,8 @@ def build_span(
start_time: start_time,
tags: tags,
trace_id: trace_id,
type: type
type: type,
inherit_parent_service: @inherit_parent_service
)
rescue StandardError => e
Datadog.logger.debug { "Failed to build new span: #{e}" }
Expand Down
13 changes: 10 additions & 3 deletions lib/datadog/tracing/tracer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ def initialize(
),
span_sampler: Sampling::Span::Sampler.new,
tags: {},
writer: Writer.new
writer: Writer.new,
inherit_parent_service: false
)
@trace_flush = trace_flush
@default_service = default_service
Expand All @@ -68,6 +69,7 @@ def initialize(
@span_sampler = span_sampler
@tags = tags
@writer = writer
@inherit_parent_service = inherit_parent_service
end

# Return a {Datadog::Tracing::SpanOperation span_op} and {Datadog::Tracing::TraceOperation trace_op}
Expand Down Expand Up @@ -327,12 +329,16 @@ def build_trace(digest = nil)
trace_state: digest.trace_state,
trace_state_unknown_fields: digest.trace_state_unknown_fields,
remote_parent: digest.span_remote,
default_service: @default_service,
inherit_parent_service: @inherit_parent_service,
)
else
TraceOperation.new(
hostname: hostname,
profiling_enabled: profiling_enabled,
remote_parent: false,
default_service: @default_service,
inherit_parent_service: @inherit_parent_service,
)
end
end
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Comment on lines -345 to +355
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


sample_span(event_trace_op, event_span)
flush_trace(event_trace_op)
end
Expand Down
2 changes: 2 additions & 0 deletions spec/datadog/core/configuration/components_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@
end
end
let(:span_sampler) { be_a(Datadog::Tracing::Sampling::Span::Sampler) }
let(:inherit_parent_service) { false }
let(:default_options) do
{
default_service: settings.service,
Expand All @@ -434,6 +435,7 @@
sampler: sampler,
span_sampler: span_sampler,
writer: writer,
inherit_parent_service: inherit_parent_service,
}
end

Expand Down
31 changes: 31 additions & 0 deletions spec/datadog/tracing/configuration/settings_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,37 @@ def propagation_style_inject
end
end

describe '#inherit_parent_service' do
subject(:inherit_parent_service) { settings.tracing.inherit_parent_service }

it { is_expected.to be false }

context 'when inherit_parent_service' do
before { settings.tracing.inherit_parent_service = value }

context 'is set to true' do
let(:value) { true }

it { is_expected.to be true }
end

context 'is set to false' do
let(:value) { false }

it { is_expected.to be false }
end
end
end

describe '#inherit_parent_service=' do
it 'updates the #inherit_parent_service setting' do
expect { settings.tracing.inherit_parent_service = true }
.to change { settings.tracing.inherit_parent_service }
.from(false)
.to(true)
end
end

describe '#header_tags' do
subject(:header_tags) { settings.tracing.header_tags }

Expand Down
48 changes: 44 additions & 4 deletions spec/datadog/tracing/span_operation_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@
end

context 'identifying service_entry_span' do
context 'when service of root and child are `nil`' do
context 'when service of parent and child are `nil`' do
it do
root_span_op = described_class.new('root')
child_span_op = described_class.new('child_1')
Expand All @@ -482,7 +482,7 @@
end
end

context 'when service of root and child are identical' do
context 'when service of parent and child are identical' do
it do
root_span_op = described_class.new('root', service: 'root_service')
child_span_op = described_class.new('child_1', service: root_span_op.service)
Expand All @@ -502,7 +502,7 @@
end
end

context 'when service of root and child are different' do
context 'when service of parent and child are different' do
it do
root_span_op = described_class.new('root')
child_span_op = described_class.new('child_1', service: 'child_service')
Expand All @@ -522,7 +522,7 @@
end
end

context 'when service of root and child are different, overriden within the measure block' do
context 'when service of parent and child are different, overriden within the measure block' do
it do
root_span_op = described_class.new('root')
child_span_op = described_class.new('child_1')
Expand Down Expand Up @@ -806,6 +806,46 @@
expect(span_op.finish).to be(original_span)
end
end

context 'with a parent span' do
let(:parent_span) { described_class.new('parent', service: 'parent_service') }

before do
span_op.send(:parent=, parent_span)
end

context 'with no service set' do
it 'span has no service' do
expect(finish.service).to eq(nil)
end
end

context 'with a service set' do
let(:parent_service) { 'parent_service' }

context 'without inherit_parent_service set' do
it 'span does not inherit the service' do
expect(finish.service).to eq(nil)
end
end

context 'with inherit_parent_service set' do
let(:options) { { inherit_parent_service: true } }

it 'span inherits the service' do
expect(finish.service).to eq('parent_service')
end
end

context 'and child span service set' do
let(:options) { { service: 'child_service' } }

it 'span does not inherit the service' do
expect(finish.service).to eq('child_service')
end
end
end
end
end

describe '#finished?' do
Expand Down
Loading
Loading