Skip to content
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-2347: Provide core SDK context #988

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

0xnm
Copy link
Contributor

@0xnm 0xnm commented Jul 20, 2022

What does this PR do?

This change brings a complete DatadogContext which includes some core SDK information, like service name, env, time information, device information, etc.

Having DatadogContext allows to replace userInfoProvider, timeProvider, networkInfoProvider, deviceInfoProvider with it (currently replacement is done mostly in RUM, not everywhere).

The data inside DatadogContext is from the time the context was requested, there are no computed properties.

NB: this change doesn't bring any thread-safety invariants or non-blocking API, those will be addresses later.

Reference in iOS repository DataDog/dd-sdk-ios#916.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@xgouchet xgouchet added the size-huge This PR is huge sized label Jul 20, 2022
@0xnm 0xnm force-pushed the nogorodnikov/rumm-2347/provide-core-sdk-context branch from 8fcf7c2 to da2dd64 Compare July 20, 2022 11:30
@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2022

Codecov Report

Merging #988 (da2dd64) into feature/sdkv2 (7e3b57e) will increase coverage by 0.12%.
The diff coverage is 91.55%.

@@                Coverage Diff                @@
##           feature/sdkv2     #988      +/-   ##
=================================================
+ Coverage          82.79%   82.91%   +0.12%     
=================================================
  Files                295      295              
  Lines               9714     9823     +109     
  Branches            1590     1600      +10     
=================================================
+ Hits                8042     8144     +102     
- Misses              1190     1195       +5     
- Partials             482      484       +2     
Impacted Files Coverage Δ
...src/main/kotlin/com/datadog/android/DatadogSite.kt 100.00% <ø> (ø)
.../android/core/internal/data/upload/UploadWorker.kt 83.33% <0.00%> (ø)
...core/internal/system/DefaultAndroidInfoProvider.kt 84.44% <ø> (ø)
...id/core/internal/system/NoOpAndroidInfoProvider.kt 100.00% <ø> (ø)
...n/com/datadog/android/v2/api/context/DeviceType.kt 100.00% <ø> (ø)
.../main/kotlin/com/datadog/android/rum/RumMonitor.kt 77.08% <66.67%> (-0.92%) ⬇️
...android/v2/core/internal/DatadogContextProvider.kt 81.03% <81.03%> (ø)
.../android/rum/internal/domain/scope/RumViewScope.kt 96.88% <87.32%> (-0.04%) ⬇️
...ndroid/rum/internal/domain/scope/RumActionScope.kt 98.44% <94.12%> (-0.01%) ⬇️
...roid/rum/internal/domain/scope/RumResourceScope.kt 97.91% <94.29%> (-0.01%) ⬇️
... and 23 more

@0xnm 0xnm marked this pull request as ready for review July 20, 2022 11:57
@0xnm 0xnm requested a review from a team as a code owner July 20, 2022 11:57
/**
* Device type.
*/
public enum class DeviceType {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it public by default ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, public can be removed here

@@ -9,5 +9,9 @@ package com.datadog.android.v2.core.internal
import com.datadog.android.v2.api.context.DatadogContext

internal interface ContextProvider {
val context: DatadogContext?
// TODO RUMM-0000 getting context may be quite heavy, should it be something non-blocking here?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean like an async call ? Maybe we can provide both ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are currently not sure if we will expose core context provider as a part of public API. For internal needs we are ok with blocking call for now, considering the way we get context fields and what fields are added to the context, so keeping things simple for now.

)
}

private fun NetworkInfoV1.Connectivity.asV2(): NetworkInfo.Connectivity {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why doing the conversion and not have one single NetworkInfo for both v1 and v2 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. The thing is that we indeed have some duplication for models between V1 and V2: UserInfo as well, for example. Eventually it will be only one, duplication is only for the V1 to V2 transition period, to have smaller changes footprint in PRs.

@0xnm 0xnm merged commit 6a8f162 into feature/sdkv2 Jul 21, 2022
@0xnm 0xnm deleted the nogorodnikov/rumm-2347/provide-core-sdk-context branch July 21, 2022 10:28
@xgouchet xgouchet added this to the 1.16.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-huge This PR is huge sized
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants