-
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-696 Stop sending tracing Span
for RUM Resources
#314
RUMM-696 Stop sending tracing Span
for RUM Resources
#314
Conversation
… Tracer registered
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.
looks good, we can add one more test case though (i couldn't find if the case was already there)
Sources/Datadog/URLSessionAutoInstrumentation/Interception/TaskInterception.swift
Outdated
Show resolved
Hide resolved
Sources/Datadog/URLSessionAutoInstrumentation/Interception/URLSessionInterceptor.swift
Outdated
Show resolved
Hide resolved
XCTAssertNil(interceptedThirdPartyRequest.allHTTPHeaderFields?[TracingHTTPHeaders.traceIDField]) | ||
XCTAssertNil(interceptedThirdPartyRequest.allHTTPHeaderFields?[TracingHTTPHeaders.parentSpanIDField]) | ||
XCTAssertNil(interceptedThirdPartyRequest.allHTTPHeaderFields?[TracingHTTPHeaders.originField]) | ||
XCTAssertNil(interceptedInternalRequest.allHTTPHeaderFields?[TracingHTTPHeaders.traceIDField]) | ||
XCTAssertNil(interceptedInternalRequest.allHTTPHeaderFields?[TracingHTTPHeaders.parentSpanIDField]) | ||
XCTAssertNil(interceptedInternalRequest.allHTTPHeaderFields?[TracingHTTPHeaders.originField]) |
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 guess we expect request.allHTTPHeaderFields == nil/unchanged
in those 2 cases:
XCTAssertNil(interceptedThirdPartyRequest.allHTTPHeaderFields?)
XCTAssertNil(interceptedInternalRequest.allHTTPHeaderFields?)
?
and the next lines might already cover them:
XCTAssertEqual(thirdPartyRequest, interceptedThirdPartyRequest)
XCTAssertEqual(internalRequest, interceptedInternalRequest)
if HTTPHeaderFields
are changed, those assertions fail; don't they?
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.
Yes, inspecting .allHTTPHeaderFields
is redundant if we assert the full request equality later 👍. However, due to request equality bug in iOS 14 SDK: https://openradar.appspot.com/radar?id=4988276943355904 it seemed safer to do it before. To be more verbose on this, I added extra helper: assertRequestsEqual(_:_:)
which implements the workaround. Redundant checks are now removed 👌.
interceptor.additionalHeadersForFirstPartyRequests, | ||
[TracingHTTPHeaders.originField: TracingHTTPHeaders.rumOriginValue], | ||
"Additional `x-datadog-origin: rum` header should be injected when both Tracing and RUM instrumentations are enabled." | ||
) | ||
} | ||
|
||
// MARK: - URLRequest Interception |
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.
we can add already traced request (original request with tracing headers) test case here
i guess we expect to have the original headers after the interception, correct?
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.
Sorry, I don't understand 🙂. In quoted lines we do not test requests, but only internal state of the interceptor
(interceptor.additionalHeadersForFirstPartyRequests
).
There's no such case, where interceptor.modify(request:)
is called for a request, which was previously intercepted. This contract is guaranteed in swizzler tests.
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.
LGTM
What and why?
🚚 This PR changes the logic of auto-instrumented, network
Span
generation depending of which feature (RUM or Tracing) is enabled. This is because recently our RUM backend started generating spans for RUM Resources on behalf of the SDK.New behaviour, assuming that
set(firstPartyHosts:)
is set andDDURLSessionDelegate()
is installed:Global.sharedTracer
Global.rum
Span
+ inject tracing headerstrace_id
andspan_id
+ inject tracing headers + injectx-datadog-origin: rum
headerAlso, appropriate warnings are printed on different misconfiguration, e.g. when Tracing auto-instrumentation is enabled, and a first party request is sent while
Global.sharedTracer
is not registered.How?
Continuing what was started in #262, the logic of
interceptor
was simplified. Now, instead of two distinct handlers (for RUM and Tracing), it takes only one interface:implemented by
URLSessionRUMResourceHandler
andURLSessionTracingHandler
.Interceptor:
SpanContext
and customx-datadog-origin: rum
header into/from 1st party requests,Handler:
URLSessionRUMResourceHandler
- send RUM Resources and associateRUMSpanContext
if set for theinterception
,URLSessionTracingHandler
- sendSpan
only for interceptions flagged asinterception.isFirstPartyRequest
.Review checklist