-
Notifications
You must be signed in to change notification settings - Fork 135
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
RUMM-2924 Create DatadogTrace
API for 2.x
#1325
Conversation
Datadog ReportBranch report: ✅ |
196b93c
to
17f12c3
Compare
DatadogTrace
API for 2.x DatadogTrace
API for 2.x
/// tracer = DDTracer.initialize(...) | ||
/// | ||
public class DatadogTracer: OTTracer { | ||
internal class DatadogTracer: OTTracer { |
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.
Leaving it not renamed, because I feel we lack solid convention for such things. I can rename it to anything (<whatever>Tracer
) but I wish we can make up some no-brainer pattern to follow - likely on next iOS Dev Chat.
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.
Perfect 👌
I've left one non-blocking question but it's good to go IMO!
// MARK: - Nested Types | ||
|
||
/// Configuration of automatic network requests tracing. | ||
public struct URLSessionTracking { |
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 wonder if shouldn't combine this one and RUM.Configuration.URLSessionTracking
into a single config struct defined in DatadogInternal
🤔
Not a change request but I would like your opinion on this?
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 was thinking about this 👌, but IMO this wouldn't be correct nor user-friendly.
First, there is a slight but key difference in both:
// DatadogRUM:
public struct URLSessionTracking {
public var firstPartyHostsTracing: FirstPartyHostsTracing? // <-- ❗️ optional
public var resourceAttributesProvider: RUM.ResourceAttributesProvider?
}
// DatadogTrace:
public struct URLSessionTracking {
public var firstPartyHostsTracing: FirstPartyHostsTracing // <-- ❗️ non-optional
}
Not only RUM takes resourceAttributesProvider
, but also optionality of firstPartyHostsTracing
is different. This comes from the fact that:
- If
URLSession
is instrumented by RUM, it tracks all requests as RUM resource. Additionally it will inject tracing headers to first-party requests, hence we ask for their definition optionaly. - If
URLSession
is instrumented by Trace, it doesn't track all requests (ref.: RUMM-696 Stop sending tracingSpan
for RUM Resources #314). It only intercepts first-party requests and injects tracing headers. Hence, it doesn't make sense to haveURLSessionTracking
without definition of hosts.
Next, because of that difference ☝️ , it could be hard to come up with a single definition of URLSessionTracking
that works for both products and includes product-agnostic explanation in API comment.
Last, this ☝️ is the difference we have today. It is easy to imagine this drifting even more away (e.g. distinguishing the default sampleRate
value in FirstPartyHostsTracing
enum of both products or adding more header types, only in RUM). Consider the fact that RUM and APM are separate DD products with distinct infrastructure which might translate into separate constraints.
For all that reasons, I think it is safer to duplicate this definition than assume wrong abstraction. To help users with adopting this, I made sure that both URLSessionTracking
and FirstPartyHostsTracing
APIs stay close to each other in both products.
How does it all sound @maxep ?
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.
Also, keep in mind that there is no practical use case of "sharing config definition" by the user. RUM offers a superset of distributed tracing - if you enable automatic RUM resources with urlSessionTracking
, no real point of enabling APM network tracing as RUM will do distributed tracing for you.
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.
It all makes sense, thanks for the clarification 🙏
What and why?
📦 This PR extracts
DatadogTrace
public API for2.0.0
.The new API for tracing will consist of 3 main interfaces:
@maxep please note that I'm not introducing
TracerProtocol
like we do for RUM monitor and Logger. Unlike these two, our tracer conforms to external Open Tracing standard and hence IMO we should using existingOTTracer
interface. Even though OT is deprecated and it is just a conventional conformance, long term we will rather switch to OTel and not our custom interface.How?
Only mandatory updates and following convention:
Datadog
;Trace.Configuration
inDatadogTrace
;DDTraceConfiguration
inDatadogObjc
;DatadogTests
where we still carry onTracer
tests;pbxproj
Distributed Tracing API
Because
URLSession
instrumentation can be enabled in both RUM and Trace configuration, I kept the same convention in its setup (aligned with #1321).By default, no
URLSession
instrumentation is installed (distributed tracing is disabled):To enable distributed tracing with
x-datadog-*
headers:To enable distributed tracing with custom headers:
❗ Note, that network instrumentation in tracing only works for first party requests. Requests sent to 3rd party URLs won't be traced: no span and no headers injection. Nothing new, just leaving it for clarity.
Review checklist
Custom CI job configuration (optional)