-
Notifications
You must be signed in to change notification settings - Fork 375
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.
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
Remove default value from peer.service
tag
#3846
base: master
Are you sure you want to change the base?
Conversation
BenchmarksBenchmark execution time: 2024-08-15 20:54:26 Comparing candidate commit f449211 in PR branch Found 4 performance improvements and 0 performance regressions! Performance is the same for 19 metrics, 2 unstable metrics. scenario:profiler - Major GC runs (profiling disabled)
scenario:profiler - Major GC runs (profiling enabled)
scenario:profiler - profiler gc
scenario:tracing - trace.to_digest
|
1209861
to
ccffd07
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3846 +/- ##
==========================================
- Coverage 97.84% 97.84% -0.01%
==========================================
Files 1264 1264
Lines 75726 75745 +19
Branches 3729 3730 +1
==========================================
+ Hits 74094 74109 +15
- Misses 1632 1636 +4 ☔ View full report in Codecov by Sentry. |
@@ -123,6 +123,17 @@ def self.included(base) | |||
o.default({}) | |||
end | |||
|
|||
# Enables population the `peer.service` tag. | |||
# When disabled, other peer service related configurations have no effect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to ask for this (as I'm not sure), but isn't it peer.service
also supposed to be calculated for v1
schema (is that a thing in Ruby I don't actually see it in the repo?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the question, maybe I don't know exactly what you mean by v1
schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, in .NET: https://github.com/DataDog/dd-trace-dotnet/blob/45a66abc870d1d45ce0778909162203e55dcfa2a/tracer/src/Datadog.Trace/Configuration/ConfigurationKeys.cs#L428
Maybe it isn't a thing for Ruby?
https://github.com/search?q=org%3ADataDog%20DD_TRACE_SPAN_ATTRIBUTE_SCHEMA&type=code
Ruby only has one use of it and it appears to be a fake usage to get system-tests
working, so maybe this is just something that wasn't needed for Ruby?
# setting DD_TRACE_SPAN_ATTRIBUTE_SCHEMA as the APM Test Agent relies on this ENV to run service naming assertions |
config = Datadog.configuration.tracing.contrib | ||
|
||
# If `peer_service_defaults` is disabled, we only read peer service from an explicitly set `peer.service` tag | ||
sources = Datadog::Tracing::Contrib::SpanAttributeSchema::REFLEXIVE_SOURCES unless config.peer_service_defaults |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is sources
set from two sources and then overwritten with one of the source?
Today, the default value of the span tag
peer.service
does not provide the correct semantics to populated the new Inferred Service dependency experience.The
peer.service
tag should only be used as an explicit user override, which is supported today by APM Ruby.This PR removes the default value of
peer.service
, while keeping any explicitly values set by configuration (e.g.c.tracing.instrument :pg, peer_service: 'mydb'
).If you need to keep the default values of
peer.service
, you can set the environment variableDD_TRACE_PEER_SERVICE_DEFAULTS_ENABLED=true
to restore the behavior to before this change.