Skip to content

Fix base service logic and make it future proof#5510

Open
amarziali wants to merge 6 commits intomasterfrom
andrea.marziali/base_service
Open

Fix base service logic and make it future proof#5510
amarziali wants to merge 6 commits intomasterfrom
andrea.marziali/base_service

Conversation

@amarziali
Copy link
Copy Markdown

What does this PR do?

Moves _dd.base_service tag logic from 25 individual integration files into SpanOperation#finish, making it apply universally to every span in the system.

Motivation:

The _dd.base_service tag was previously set by copy-pasted boilerplate in each integration. This meant it was absent for spans created via the manual tracing API (e.g. Datadog::Tracing.trace('op', service: 'custom')), if the service name was manually changed, and any new integration had to remember to add it manually. Centralizing in SpanOperation#finish closes the gap, removes the duplication, and makes future integrations correct by default.

Change log entry

Additional Notes:

How to test the change?

@amarziali amarziali requested review from a team as code owners March 26, 2026 11:29
@amarziali amarziali requested a review from mabdinur March 26, 2026 11:29
@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Mar 26, 2026
@github-actions
Copy link
Copy Markdown

👋 Hey @DataDog/ruby-guild, please fill "Change log entry" section in the pull request description.

If changes need to be present in CHANGELOG.md you can state it this way

**Change log entry**

Yes. A brief summary to be placed into the CHANGELOG.md

(possible answers Yes/Yep/Yeah)

Or you can opt out like that

**Change log entry**

None.

(possible answers No/Nope/None)

Visited at: 2026-03-26 11:29:31 UTC

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 26, 2026

Typing analysis

Note: Ignored files are excluded from the next sections.

steep:ignore comments

This PR introduces 2 steep:ignore comments, and clears 2 steep:ignore comments.

steep:ignore comments (+2-2)Introduced:
lib/datadog/tracing/span_operation.rb:328
lib/datadog/tracing/span_operation.rb:400
Cleared:
lib/datadog/tracing/span_operation.rb:325
lib/datadog/tracing/span_operation.rb:397

Untyped methods

This PR introduces 5 untyped methods, and clears 5 untyped methods. It increases the percentage of typed methods from 61.69% to 61.71% (+0.02%).

Untyped methods (+5-5)Introduced:
sig/datadog/tracing/span_operation.rbs:162
└── def build_span: () -> untyped
sig/datadog/tracing/span_operation.rbs:164
└── def parent=: (untyped parent) -> untyped
sig/datadog/tracing/span_operation.rbs:166
└── def duration_marker: () -> untyped
sig/datadog/tracing/span_operation.rbs:168
└── def start_time_nano: () -> untyped
sig/datadog/tracing/span_operation.rbs:170
└── def duration_nano: () -> untyped
Cleared:
sig/datadog/tracing/span_operation.rbs:160
└── def build_span: () -> untyped
sig/datadog/tracing/span_operation.rbs:162
└── def parent=: (untyped parent) -> untyped
sig/datadog/tracing/span_operation.rbs:164
└── def duration_marker: () -> untyped
sig/datadog/tracing/span_operation.rbs:166
└── def start_time_nano: () -> untyped
sig/datadog/tracing/span_operation.rbs:168
└── def duration_nano: () -> untyped

If you believe a method or an attribute is rightfully untyped or partially typed, you can add # untyped:accept on the line before the definition to remove it from the stats.

@amarziali amarziali marked this pull request as draft March 26, 2026 11:45
@pr-commenter
Copy link
Copy Markdown

pr-commenter bot commented Mar 26, 2026

Benchmarks

Benchmark execution time: 2026-03-26 11:59:04

Comparing candidate commit 4af83bd in PR branch andrea.marziali/base_service with baseline commit 5889737 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 0 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

@amarziali amarziali force-pushed the andrea.marziali/base_service branch from 67c5108 to 61e0e33 Compare March 26, 2026 12:54
@amarziali amarziali marked this pull request as ready for review March 26, 2026 13:10
@datadog-prod-us1-6
Copy link
Copy Markdown

datadog-prod-us1-6 bot commented Mar 26, 2026

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 95.34% (+0.16%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 79ce50f | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback!

Copy link
Copy Markdown
Member

@raphaelgavache raphaelgavache left a comment

Choose a reason for hiding this comment

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

nice catch, this match other tracer behavior and it's critical to never miss this tag

stop(end_time)

# Set _dd.base_service when service overrides the global service
set_base_service_tag
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correct me if I am wrong. The tracing data models do not harbor the knowledge intrinsically, and service is an attribute provided to trace/span models by instrumentations or configurations.

Hence, the logic should be pushed higher and this file should not contains changes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For cross product correlation between tracing and other products (profiling/apm telemetry etc) we need this common base service tag to be enforced on all spans and their related stats

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, and the original implementation (see #3018) had two main limitations: it was not fully implemented (e.g., it only covered integrations and not other span APIs), and it was not future-proof (there was no mechanism to automatically enforce it for new integrations).

@TonyCTHsu While it clearly needs to be addressed, suggestions on the most appropriate place to implement this logic are welcome.

In any case, it should be computed before spans are potentially sent to stats (if client-side stats are in scope for Ruby), which is why I placed it here rather than at serialization time for tracing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think where it is today is probably ok, because it's ideal to have a correct span payload when we call build_span in the next line, since the immutable span built in build_span can be used for inspection in the processing pipeline, before the span is sent to the transport.

Comment on lines +66 to +67
# Set equal to the global service when contrib span.service is overriden
TAG_BASE_SERVICE = '_dd.base_service'
TAG_BASE_SERVICE = Tracing::Metadata::Ext::TAG_BASE_SERVICE
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This constant is not public, you can delete it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done in 79ce50f

Comment on lines +34 to +36
TAG_APM_ENABLED = '_dd.apm.enabled'

# Set to the global service name when a span's service is overridden
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add a brief comment with the reason why we're setting this tag?

If there's a link for what this tag enpowers (e.g. Inferred services
I'm not actually sure if inferred services is the precise feature), that's super great to add here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes indeed I added more details on 79ce50f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integrations Involves tracing integrations tracing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants