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

Add feature activation events #5908

Merged
merged 7 commits into from Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -9,6 +9,7 @@
- Removed `enrollment_id` from being generated or recorded anywhere in the Nimbus SDK ([#5899](https://github.com/mozilla/application-services/pull/5899)).
- This was originally thought to be of use, but after running the system for sometime, we have found that this isn't needed.
- In the spirit of reducing unique identifiers in telemetry and in the spirit of Lean Data, we have removed `enrollment_id` (and the code that generates it).
- Added the feature `activation` event ([#5908](https://github.com/mozilla/application-services/pull/5908)).

## Places

Expand Down
Expand Up @@ -35,6 +35,8 @@ import org.mozilla.experiments.nimbus.internal.EnrolledExperiment
import org.mozilla.experiments.nimbus.internal.EnrollmentChangeEvent
import org.mozilla.experiments.nimbus.internal.EnrollmentChangeEventType
import org.mozilla.experiments.nimbus.internal.EnrollmentStatusExtraDef
import org.mozilla.experiments.nimbus.internal.FeatureExposureExtraDef
import org.mozilla.experiments.nimbus.internal.MalformedFeatureConfigExtraDef
import org.mozilla.experiments.nimbus.internal.MetricsHandler
import org.mozilla.experiments.nimbus.internal.NimbusClient
import org.mozilla.experiments.nimbus.internal.NimbusClientInterface
Expand Down Expand Up @@ -96,6 +98,37 @@ open class Nimbus(
)
}
}

override fun recordFeatureActivation(event: FeatureExposureExtraDef) {
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,
),
)
}

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

private val nimbusClient: NimbusClientInterface
Expand Down Expand Up @@ -507,61 +540,16 @@ open class Nimbus(
// If the experiment slug is known, then use that to look up the enrollment.
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
@AnyThread
internal fun recordExposureOnThisThread(featureId: String, experimentSlug: String? = null) {
if (experimentSlug.isNullOrEmpty()) {
recordExposureFromFeature(featureId)
} else {
recordExposureFromExperiment(featureId, experimentSlug)
}
}

private fun recordExposureFromFeature(featureId: String) = withCatchAll("recordExposureFromFeature") {
// First, we get the enrolled feature, if there is one, for this id.
val enrollment = nimbusClient.getEnrollmentByFeature(featureId) ?: return@withCatchAll
// If branch is null, this is a rollout, and we're not interested in recording
// exposure for rollouts.
val branch = enrollment.branch ?: return@withCatchAll
// Finally, if we do have an experiment for the given featureId, we will record the
// exposure event in Glean. This is to protect against accidentally recording an event
// for an experiment without an active enrollment.
NimbusEvents.exposure.record(
NimbusEvents.ExposureExtra(
experiment = enrollment.slug,
branch = branch,
featureId = featureId,
),
)
}

private fun recordExposureFromExperiment(featureId: String, slug: String) = withCatchAll("recordExposureFromExperiment") {
nimbusClient.getExperimentBranch(slug)?.let { branch ->
NimbusEvents.exposure.record(
NimbusEvents.ExposureExtra(
experiment = slug,
branch = branch,
featureId = featureId,
),
)
}
internal fun recordExposureOnThisThread(featureId: String, experimentSlug: String? = null) = withCatchAll("recordFeatureExposure") {
nimbusClient.recordFeatureExposure(featureId, experimentSlug)
}

// The malformed feature event is recorded by app developers, if the configuration is
// _semantically_ invalid or malformed.
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
@AnyThread
internal fun recordMalformedConfigurationOnThisThread(featureId: String, partId: String) = withCatchAll("recordMalformedConfiguration") {
// First, we get the enrolled feature, if there is one, for this id.
val enrollment = nimbusClient.getEnrollmentByFeature(featureId)
// If the enrollment is null, then that's the most serious: we've shipped invalid FML,
// If the branch is null, then this is also serious: we've shipped an invalid rollout.
NimbusEvents.malformedFeature.record(
NimbusEvents.MalformedFeatureExtra(
experiment = enrollment?.slug,
branch = enrollment?.branch,
featureId = featureId,
partId = partId,
),
)
nimbusClient.recordMalformedFeatureConfig(featureId, partId)
}

@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
Expand Down
Expand Up @@ -63,10 +63,12 @@ class NimbusTests {
errorReporter = { message, e -> Log.e("NimbusTest", message, e) },
)

private val nimbus = Nimbus(
private val nimbus = createNimbus()

private fun createNimbus(coenrollingFeatureIds: List<String> = listOf()) = Nimbus(
context = context,
appInfo = appInfo,
coenrollingFeatureIds = listOf(),
coenrollingFeatureIds = coenrollingFeatureIds,
server = null,
deviceInfo = deviceInfo,
observer = null,
Expand Down Expand Up @@ -189,6 +191,69 @@ class NimbusTests {
)
}

@Ignore // until the activation event is enabled by default.
@Test
fun `getFeatureVariables records activation telemetry`() {
// Load the experiment in nimbus so and optIn so that it will be active. This is necessary
// because recordExposure checks for active experiments before recording.
nimbus.setUpTestExperiments(packageName, appInfo)

// Assert that there are no events to start with
assertNull(
"There must not be any pre-existing events",
NimbusEvents.activation.testGetValue(),
)

nimbus.getVariables("missing_feature", recordExposureEvent = false)
// Missing feature does not call activation event
assertNull(
"There must not be any events for missing_features",
NimbusEvents.activation.testGetValue(),
)

nimbus.getVariables("about_welcome", recordExposureEvent = false)
// Existing feature does call activation event
val events = NimbusEvents.activation.testGetValue()
assertNotNull(
"There must be an activation event for a feature under experiment",
events,
)
assertEquals("Exactly one activation event", events!!.size, 1)

val extras = events.first().extra!!
assertEquals("test-branch", extras["branch"])
assertEquals("test-experiment", extras["experiment"])
assertEquals("about_welcome", extras["feature_id"])
}

@Test
fun `getFeatureVariables for coenrolling feature does not record activation telemetry`() {
// Load the experiment in nimbus so and optIn so that it will be active. This is necessary
// because recordExposure checks for active experiments before recording.
val nimbus = createNimbus(coenrollingFeatureIds = listOf("about_welcome"))
nimbus.setUpTestExperiments(packageName, appInfo)

// Assert that there are no events to start with
assertNull(
"There must not be any pre-existing events",
NimbusEvents.activation.testGetValue(),
)

nimbus.getVariables("missing_feature", recordExposureEvent = false)
// Missing feature does not call activation event
assertNull(
"There must not be any events for missing_features",
NimbusEvents.activation.testGetValue(),
)

nimbus.getVariables("about_welcome", recordExposureEvent = false)
// Coenrolling feature does not call activation event
assertNull(
"There must not be any events for coenrolling feature",
NimbusEvents.activation.testGetValue(),
)
}

@Test
fun `recordExposure from feature records telemetry`() {
// Load the experiment in nimbus so and optIn so that it will be active. This is necessary
Expand Down
17 changes: 16 additions & 1 deletion components/nimbus/examples/experiment.rs
Expand Up @@ -12,7 +12,10 @@ fn main() -> Result<()> {
use clap::{App, Arg, SubCommand};
use env_logger::Env;
use nimbus::{
metrics::{EnrollmentStatusExtraDef, MetricsHandler},
metrics::{
EnrollmentStatusExtraDef, FeatureExposureExtraDef, MalformedFeatureConfigExtraDef,
MetricsHandler,
},
AppContext, AvailableRandomizationUnits, EnrollmentStatus, NimbusClient,
NimbusTargetingHelper, RemoteSettingsConfig,
};
Expand All @@ -25,6 +28,18 @@ fn main() -> Result<()> {
fn record_enrollment_statuses(&self, _: Vec<EnrollmentStatusExtraDef>) {
// do nothing
}

fn record_feature_activation(&self, _activation_event: FeatureExposureExtraDef) {
// do nothing
}

fn record_feature_exposure(&self, _exposure_event: FeatureExposureExtraDef) {
// do nothing
}

fn record_malformed_feature_config(&self, _event: MalformedFeatureConfigExtraDef) {
// do nothing
}
}

// We set the logging level to be `warn` here, meaning that only
Expand Down
51 changes: 4 additions & 47 deletions components/nimbus/ios/Nimbus/Nimbus.swift
Expand Up @@ -111,57 +111,14 @@ extension Nimbus: FeaturesInterface {
}

public func recordExposureEvent(featureId: String, experimentSlug: String? = nil) {
if let experimentSlug = experimentSlug {
recordExposureFromExperiment(featureId: featureId, experimentSlug: experimentSlug)
} else {
recordExposureFromFeature(featureId: featureId)
}
}

func recordExposureFromFeature(featureId: String) {
// First, we get the enrolled feature, if there is one, for this id.
if let enrollment = getEnrollmentByFeature(featureId: featureId),
// If branch is nil, this is a rollout, and we're not interested in recording
// exposure for rollouts.
let branch = enrollment.branch
{
// Finally, if we do have an experiment for the given featureId, we will record the
// exposure event in Glean. This is to protect against accidentally recording an event
// for an experiment without an active enrollment.
GleanMetrics.NimbusEvents.exposure.record(GleanMetrics.NimbusEvents.ExposureExtra(
branch: branch,
experiment: enrollment.slug,
featureId: featureId
))
}
}

func recordExposureFromExperiment(featureId: String, experimentSlug: String) {
if let branch = getExperimentBranch(experimentId: experimentSlug) {
GleanMetrics.NimbusEvents.exposure.record(GleanMetrics.NimbusEvents.ExposureExtra(
branch: branch,
experiment: experimentSlug,
featureId: featureId
))
catchAll {
nimbusClient.recordFeatureExposure(featureId: featureId, slug: experimentSlug)
}
}

public func recordMalformedConfiguration(featureId: String, with partId: String) {
// First, we get the enrolled feature, if there is one, for this id.
let enrollment = getEnrollmentByFeature(featureId: featureId)
// If the enrollment is nil, then that's the most serious: we've shipped invalid FML,
// If the branch is nil, then this is also serious: we've shipped an invalid rollout.
GleanMetrics.NimbusEvents.malformedFeature.record(GleanMetrics.NimbusEvents.MalformedFeatureExtra(
branch: enrollment?.branch,
experiment: enrollment?.slug,
featureId: featureId,
partId: partId
))
}

func getEnrollmentByFeature(featureId: String) -> EnrolledFeature? {
return catchAll {
try nimbusClient.getEnrollmentByFeature(featureId: featureId)
catchAll {
nimbusClient.recordMalformedFeatureConfig(featureId: featureId, partId: partId)
}
}

Expand Down
28 changes: 28 additions & 0 deletions components/nimbus/ios/Nimbus/NimbusCreate.swift
Expand Up @@ -33,6 +33,34 @@ class GleanMetricsHandler: MetricsHandler {
))
}
}

func recordFeatureActivation(event: FeatureExposureExtraDef) {
GleanMetrics.NimbusEvents.activation
.record(GleanMetrics.NimbusEvents.ActivationExtra(
branch: event.branch,
experiment: event.slug,
featureId: event.featureId
))
}

func recordFeatureExposure(event: FeatureExposureExtraDef) {
GleanMetrics.NimbusEvents.exposure
.record(GleanMetrics.NimbusEvents.ExposureExtra(
branch: event.branch,
experiment: event.slug,
featureId: event.featureId
))
}

func recordMalformedFeatureConfig(event: MalformedFeatureConfigExtraDef) {
GleanMetrics.NimbusEvents.malformedFeature
.record(GleanMetrics.NimbusEvents.MalformedFeatureExtra(
branch: event.branch,
experiment: event.slug,
featureId: event.featureId,
partId: event.part
))
}
}

public extension Nimbus {
Expand Down
27 changes: 27 additions & 0 deletions components/nimbus/metrics.yaml
Expand Up @@ -140,6 +140,33 @@ nimbus_events:
- tlong@mozilla.com
- telemetry-team@mozilla.com
expires: never
activation:
type: event
description: >
Recorded when a feature is configured with an experimental
configuration for the first time in this session.
extra_keys:
experiment:
type: string
description: The slug/unique identifier of the experiment
branch:
type: string
description: The branch slug/identifier that was randomly chosen
feature_id:
type: string
description: The identifier of the feature that is recording an exposure
bugs:
- https://mozilla-hub.atlassian.net/browse/EXP-3950
data_reviews:
- https://github.com/mozilla/application-services/pull/5908#pullrequestreview-1718840482
data_sensitivity:
- technical
notification_emails:
- jhugman@mozilla.com
- project-nimbus@mozilla.com
expires: never
Copy link
Member

Choose a reason for hiding this comment

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

To prevent a similar issue to the enrollment_status metric overload, can we initially add this as disabled and enable it via glean server knobs in nightly?

Suggested change
expires: never
expires: never
disabled: true

# Disabled by default. This needs a server-knobs rollout to ensure the volume is not overwhelming.
disabled: true
exposure:
type: event
description: >
Expand Down