RUMM-2171 Use core feature scope in RUM #902
Conversation
maxep
force-pushed
the
maxep/RUMM-2171/rum-feature-scope
branch
from
bd85422 to
482f482
Compare
maxep
force-pushed
the
maxep/RUMM-2171/rum-feature-scope
branch
from
174192d to
d5cbeaf
Compare
maxep
force-pushed
the
maxep/RUMM-2171/rum-feature-scope
branch
from
d5cbeaf to
27cce00
Compare
| return childScope | ||
| } | ||
| /// Returns `self` to be kept open, `nil` if it requests to close. | ||
| func scope(byPropagating command: RUMCommand, context: DatadogV1Context, writer: Writer) -> Self? { |
There was a problem hiding this comment.
I've simplified the propagation methods signatures to better reflect its behaviour.
| service: dependencies.serviceName, | ||
| service: context.service, | ||
| session: .init( | ||
| hasReplay: nil, | ||
| id: context.sessionID.toRUMDataFormat, | ||
| id: self.context.sessionID.toRUMDataFormat, | ||
| type: dependencies.ciTest != nil ? .ciTest : .user |
There was a problem hiding this comment.
nit: to help differentiating between contexts, we could name it self.rumContext and sdkContext.
On the other hand both are well decoupled and we have enough type safety on them to avoid mistakes 👍. I'm only worried that in the future, when RUM context will be a part of the SDKContext it could be misleading (or even error-prone) here. Any thoughts @maxep ?
There was a problem hiding this comment.
I agree, it's confusing. I will rename that 👍
There was a problem hiding this comment.
Actually self.context is defined by RUMContextProvider protocol. So changing the definition will have a large impact. RUMContext will be merge in Core Context anyway!
| let appStateListener: AppStateListening | ||
| let deviceInfo: RUMDevice | ||
| let osInfo: RUMOperatingSystem | ||
| let userInfoProvider: RUMUserInfoProvider | ||
| let launchTimeProvider: LaunchTimeProviderType | ||
| let connectivityInfoProvider: RUMConnectivityInfoProvider | ||
| let serviceName: String | ||
| let applicationVersion: String | ||
| let sdkVersion: String | ||
| let source: String | ||
| let firstPartyURLsFilter: FirstPartyURLsFilter | ||
| let eventBuilder: RUMEventBuilder | ||
| let eventOutput: RUMEventOutput | ||
| let rumUUIDGenerator: RUMUUIDGenerator | ||
| /// Adjusts RUM events time (device time) to server time. | ||
| let dateCorrector: DateCorrectorType | ||
| /// Integration with Crash Reporting. It updates the crash context with RUM info. | ||
| /// `nil` if Crash Reporting feature is not enabled. | ||
| let crashContextIntegration: RUMWithCrashContextIntegration? |
There was a problem hiding this comment.
Nice to see it shrink 💪. I think that ultimately we should only keep RUM-specific properties in there and everything else should be passed through SDKContext, WDYT @maxep? Some of it is even possible now - e.g. I can see sdkInitDate and MobileDevice (transitive input for RUMDevice and RUMOperatingSystem) already defined in DatadogV1Context. If we agree, I think it's best to tackle them in separate PR.
There was a problem hiding this comment.
Ah indeed, that's a good point! Also makes me realise that I forgot to remove the eventBuilder and eventOutput.
I've created a dedicated story for managing the context in a single queue, RUMM-2299, I can add that in the requirements!
| connectivity: dependencies.connectivityInfoProvider.current, | ||
| connectivity: .init(context: context), |
There was a problem hiding this comment.
Nice 👍. I think we should do the same for initializing other RUM data types from context, e.g.: RUMOperatingSystem, RUMDevice. Can be a separate PR IMO.
| @@ -593,9 +596,16 @@ public class RUMMonitor: DDRUMMonitor, RUMCommandSubscriber { | |||
| // MARK: - RUMCommandSubscriber | |||
|
|
|||
| func process(command: RUMCommand) { | |||
| guard let scope = core.v1.scope(for: RUMFeature.self) else { | |||
| return | |||
There was a problem hiding this comment.
(just checking) This return is unreachable, right? We do early-return in RUMMonitor.initialize() when RUMFeature is not registered, so it's not possible to run here.
There was a problem hiding this comment.
Yes, we do an early return in RUMMonitor.initialize() with appropriate console prints 👍
| _ = scope.process( | ||
| command: RUMStartUserActionCommand.mockAny(), | ||
| context: context, | ||
| writer: writer | ||
| ) |
There was a problem hiding this comment.
After seeing this all over, I wonder if it wouldn't make sense to anticipate future needs by introducing container type bundling context and writer in RUMScope APIs. For example:
scope.process(command:rumScopeContext:)where initially we will have:
internal struct RUMScopeContext {
let sdkContext: DatadogV1Context
let writer: Writer
}I will keep door open for bundling other aspects (if necessary) without huge refactoring pain. WDYT @maxep ?
There was a problem hiding this comment.
I would prefer to keep those 2 as arguments for now! It would be best to use a container once we have a larger list of parameters. Command processing is well contained into RUM scopes, it won't be too difficult to refactor 👍
What and why?
RUMMonitorand scope instances now build and record events usingV1FeatureScopeprovided byDatadogV1CoreProtocol.How?
The
RUMMonitornow keeps a reference to the providedcoreto retrieve the scope when aRUMCommandneeds to be processed byRUMScopes. TheRUMScopenow expect aDatadogV1Contextand aWriterto record events and apply side-effects.Review checklist
Custom CI job configuration (optional)