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.

Sign up for GitHub

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

add metrics coroutine scope #6255

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

add metrics coroutine scope #6255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
200 changes: 112 additions & 88 deletions components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import org.mozilla.experiments.nimbus.internal.NimbusException
import org.mozilla.experiments.nimbus.internal.RecordedContext
import java.io.File
import java.io.IOException
import kotlin.system.measureTimeMillis

private const val EXPERIMENT_COLLECTION_NAME = "nimbus-mobile-experiments"
private const val NIMBUS_DATA_DIR: String = "nimbus_data"
Expand Down Expand Up @@ -81,55 +82,65 @@ open class Nimbus(

private val updateScope: CoroutineScope? = delegate.updateScope

private val metricsScope: CoroutineScope = delegate.metricsScope

private val errorReporter = delegate.errorReporter

private val logger = delegate.logger

private val metricsHandler = object : MetricsHandler {
override fun recordEnrollmentStatuses(enrollmentStatusExtras: List<EnrollmentStatusExtraDef>) {
for (extra in enrollmentStatusExtras) {
NimbusEvents.enrollmentStatus.record(
NimbusEvents.EnrollmentStatusExtra(
branch = extra.branch,
slug = extra.slug,
status = extra.status,
reason = extra.reason,
errorString = extra.errorString,
conflictSlug = extra.conflictSlug,
),
)
metricsScope.launch {
for (extra in enrollmentStatusExtras) {
NimbusEvents.enrollmentStatus.record(
NimbusEvents.EnrollmentStatusExtra(
branch = extra.branch,
slug = extra.slug,
status = extra.status,
reason = extra.reason,
errorString = extra.errorString,
conflictSlug = extra.conflictSlug,
),
)
}
}
}

override fun recordFeatureActivation(event: FeatureExposureExtraDef) {
NimbusEvents.activation.record(
NimbusEvents.ActivationExtra(
experiment = event.slug,
branch = event.branch,
featureId = event.featureId,
),
)
metricsScope.launch {
NimbusEvents.activation.record(
NimbusEvents.ActivationExtra(
experiment = event.slug,
branch = event.branch,
featureId = event.featureId,
),
)
}
}

override fun recordFeatureExposure(event: FeatureExposureExtraDef) {
NimbusEvents.exposure.record(
NimbusEvents.ExposureExtra(
experiment = event.slug,
branch = event.branch,
featureId = event.featureId,
),
)
metricsScope.launch {
NimbusEvents.exposure.record(
NimbusEvents.ExposureExtra(
experiment = event.slug,
branch = event.branch,
featureId = event.featureId,
),
)
}
}

override fun recordMalformedFeatureConfig(event: MalformedFeatureConfigExtraDef) {
NimbusEvents.malformedFeature.record(
NimbusEvents.MalformedFeatureExtra(
experiment = event.slug,
branch = event.branch,
featureId = event.featureId,
partId = event.part,
),
)
metricsScope.launch {
NimbusEvents.malformedFeature.record(
NimbusEvents.MalformedFeatureExtra(
experiment = event.slug,
branch = event.branch,
featureId = event.featureId,
partId = event.part,
),
)
}
}
}

Expand Down Expand Up @@ -195,11 +206,13 @@ open class Nimbus(
try {
nimbusClient.getFeatureConfigVariables(featureId)?.let { JSONObject(it) }
} catch (e: NimbusException.DatabaseNotReady) {
NimbusHealth.cacheNotReadyForFeature.record(
NimbusHealth.CacheNotReadyForFeatureExtra(
featureId = featureId,
),
)
metricsScope.launch {
NimbusHealth.cacheNotReadyForFeature.record(
NimbusHealth.CacheNotReadyForFeatureExtra(
featureId = featureId,
),
)
}
null
} catch (e: Throwable) {
reportError("getFeatureConfigVariablesJson", e)
Expand Down Expand Up @@ -275,9 +288,13 @@ open class Nimbus(
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal fun fetchExperimentsOnThisThread() = withCatchAll("fetchExperiments") {
try {
NimbusHealth.fetchExperimentsTime.measure {
val time = measureTimeMillis {
nimbusClient.fetchExperiments()
}
logger("Measured ${time}ms to fetch experiments")
metricsScope.launch {
NimbusHealth.fetchExperimentsTime.accumulateSingleSample(time)
}
updateObserver {
it.onExperimentsFetched()
}
Expand Down Expand Up @@ -308,10 +325,15 @@ open class Nimbus(
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal fun applyPendingExperimentsOnThisThread() = withCatchAll("applyPendingExperiments") {
try {
val events = NimbusHealth.applyPendingExperimentsTime.measure {
nimbusClient.applyPendingExperiments()
var events: List<EnrollmentChangeEvent>?
val time = measureTimeMillis {
events = nimbusClient.applyPendingExperiments()
}
logger("Measured ${time}ms to apply pending experiments")
metricsScope.launch {
NimbusHealth.applyPendingExperimentsTime.accumulateSingleSample(time)
}
recordExperimentTelemetryEvents(events)
recordExperimentTelemetryEvents(events!!)
Copy link
Member

Choose a reason for hiding this comment

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

I'd avoid the bang bang (!!) here, on the off chance that events was null this would throw an exception.
If you can try using the safe unwrap operators and patterns, I think it would avoid this altogether.

Suggested change
recordExperimentTelemetryEvents(events!!)
events?.let {
recordExperimentTelemetryEvents(it)
}

// Get the experiments to record in telemetry
postEnrolmentCalculation()
} catch (e: NimbusException.InvalidExperimentFormat) {
Expand Down Expand Up @@ -485,52 +507,54 @@ open class Nimbus(

@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal fun recordExperimentTelemetryEvents(enrollmentChangeEvents: List<EnrollmentChangeEvent>) {
enrollmentChangeEvents.forEach { event ->
when (event.change) {
EnrollmentChangeEventType.ENROLLMENT -> {
NimbusEvents.enrollment.record(
NimbusEvents.EnrollmentExtra(
experiment = event.experimentSlug,
branch = event.branchSlug,
),
)
}

EnrollmentChangeEventType.DISQUALIFICATION -> {
NimbusEvents.disqualification.record(
NimbusEvents.DisqualificationExtra(
experiment = event.experimentSlug,
branch = event.branchSlug,
),
)
}

EnrollmentChangeEventType.UNENROLLMENT -> {
NimbusEvents.unenrollment.record(
NimbusEvents.UnenrollmentExtra(
experiment = event.experimentSlug,
branch = event.branchSlug,
),
)
}

EnrollmentChangeEventType.ENROLL_FAILED -> {
NimbusEvents.enrollFailed.record(
NimbusEvents.EnrollFailedExtra(
experiment = event.experimentSlug,
branch = event.branchSlug,
reason = event.reason,
),
)
}

EnrollmentChangeEventType.UNENROLL_FAILED -> {
NimbusEvents.unenrollFailed.record(
NimbusEvents.UnenrollFailedExtra(
experiment = event.experimentSlug,
reason = event.reason,
),
)
metricsScope.launch {
enrollmentChangeEvents.forEach { event ->
when (event.change) {
EnrollmentChangeEventType.ENROLLMENT -> {
NimbusEvents.enrollment.record(
NimbusEvents.EnrollmentExtra(
experiment = event.experimentSlug,
branch = event.branchSlug,
),
)
}

EnrollmentChangeEventType.DISQUALIFICATION -> {
NimbusEvents.disqualification.record(
NimbusEvents.DisqualificationExtra(
experiment = event.experimentSlug,
branch = event.branchSlug,
),
)
}

EnrollmentChangeEventType.UNENROLLMENT -> {
NimbusEvents.unenrollment.record(
NimbusEvents.UnenrollmentExtra(
experiment = event.experimentSlug,
branch = event.branchSlug,
),
)
}

EnrollmentChangeEventType.ENROLL_FAILED -> {
NimbusEvents.enrollFailed.record(
NimbusEvents.EnrollFailedExtra(
experiment = event.experimentSlug,
branch = event.branchSlug,
reason = event.reason,
),
)
}

EnrollmentChangeEventType.UNENROLL_FAILED -> {
NimbusEvents.unenrollFailed.record(
NimbusEvents.UnenrollFailedExtra(
experiment = event.experimentSlug,
reason = event.reason,
),
)
}
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/NimbusDelegate.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ class NimbusDelegate(
* was called upon.
*/
val updateScope: CoroutineScope? = MainScope(),
/**
* This is the coroutine scope that the SDK uses to record calls through Glean
*/
val metricsScope: CoroutineScope,
val errorReporter: ErrorReporter,
val logger: LoggerFunction,
) {
Expand All @@ -44,6 +48,7 @@ class NimbusDelegate(
fun default() = NimbusDelegate(
dbScope = createCoroutineScope(),
fetchScope = createCoroutineScope(),
metricsScope = createCoroutineScope(),
errorReporter = { msg: String, e: Throwable -> Log.e(logTag, msg, e) },
logger = { Log.i(logTag, it) },
)
Expand Down