RUMM-2138 make SDK Features simple classes#928
Conversation
eeabb5c to
ca7a978
Compare
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
I've added some questions/concerns, but overall lgtm and these are non-blocking.
With this change we have quite a lot of usages of ?. operator and null checks, but I hope later it will go away (otherwise we need to think how to we check invariants of the SDK state - for example for A to work we should have B created, and if it is not the case, we should report this violation).
| /** @inheritdoc */ | ||
| override fun intercept(chain: Interceptor.Chain): Response { | ||
| if (RumFeature.isInitialized()) { | ||
| val rumFeature = (Datadog.globalSDKCore as? DatadogCore)?.rumFeature |
There was a problem hiding this comment.
How do we make sure that we won't forget parts of the code like this (getting the feature with nullable chain with casting)? Maybe add TODO item?
I think we need to add some method which will encapsulate that, or maybe such chains will go away once we finish the rewrite.
There was a problem hiding this comment.
A lot of the dependencies will be reversed.
In v1 we had something like :
class SomethingUsingFeatureA {
fun foo() {
get FeatureA.component
}
}
In v2 we'll have:
class SomethingUsingFeatureA(component: FeatureAComponent)
class FeatureA {
buildSomething(): SomethingUsingFeatureA
}
| // endregion | ||
|
|
||
| companion object { | ||
| internal var processImportance: Int = |
There was a problem hiding this comment.
Shouldn't this field be a class member? I understand why we would want to keep it in the companion (the value will always be the same for the whole process for any instance of CoreFeature), but my concern is what if we have call to this field from different threads (say each instance of CoreFeature is running on the dedicated thread).
There was a problem hiding this comment.
In the end it'll be in the SDKContext, I kept it there to minimize the impact of the current feature
| uploadAllBatches( | ||
| RumFeature.persistenceStrategy.getReader(), | ||
| RumFeature.uploader | ||
| val globalSDKCore: DatadogCore? = (Datadog.globalSDKCore as? DatadogCore) |
There was a problem hiding this comment.
minor: why do we need an explicit type here?
There was a problem hiding this comment.
Not necessary, it'll disappear eventually
| val rumApplicationId = CoreFeature.rumApplicationId | ||
| return if (!RumFeature.isInitialized()) { | ||
| val datadogCore = Datadog.globalSDKCore as? DatadogCore | ||
| val coreFeature = datadogCore?.coreFeature |
There was a problem hiding this comment.
A thought: if rumFeature (and any other feature) takes non-null coreFeature in the constructor, can we get coreFeature through rumFeature? This would eliminate checking coreFeature for null here and other places in the codebase where we access coreFeature near the specific feature.
Because otherwise they are independent code-wise (while in reality they are dependent) and for example blocks like check for if (rumFeature == null || coreFeature == null) below look strange: we cannot have a rumFeature without having a coreFeature.
There was a problem hiding this comment.
In the end yes, it'll be that way
|
|
||
| override fun onMenuItemSelected(featureId: Int, item: MenuItem): Boolean { | ||
| val resourceId = resourceIdName(item.itemId) | ||
| val resourceId = windowReference.get()?.context.resourceIdName(item.itemId) |
There was a problem hiding this comment.
can be replaced with extension function created
| timeProvider = CoreFeature.timeProvider | ||
|
|
||
| if (coreFeature == null || webViewLogsFeature == null || webViewRumFeature == null) { | ||
| return NoOpWebViewEventConsumer() |
There was a problem hiding this comment.
It'll disappear before the end of the v2 refactor
3 participants
What does this PR do?
Converts all
SdkFeatureobjects toclasses.NOTE: tests were not updated at this stage, this is a transitional state that will be followed by more refactoring.