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

Store full enrollment date on a/b/n and truncate before sending pixels #5535

Merged
merged 2 commits into from
Jan 31, 2025
Merged

Store full enrollment date on a/b/n and truncate before sending pixels #5535

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcd5b7e
Store full enrollment date on a/b/n and truncate before sending pixels
marcosholgado Jan 24, 2025
9b0bea8
Calculate conversion window based on days rather than real 24h
marcosholgado Jan 28, 2025
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
3 changes: 1 addition & 2 deletions ...es/feature-toggles-api/src/main/java/com/duckduckgo/feature/toggles/api/FeatureToggles.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import java.lang.reflect.Method
import java.lang.reflect.Proxy
import java.time.ZoneId
import java.time.ZonedDateTime
import java.time.temporal.ChronoUnit
import kotlin.random.Random
import org.apache.commons.math3.distribution.EnumeratedIntegerDistribution

Expand Down Expand Up @@ -506,7 +505,7 @@ internal class ToggleImpl constructor(
}

return getRandomCohort(cohorts)?.copy(
enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString(),
enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString(),
)
}

Expand Down
3 changes: 2 additions & 1 deletion ...eature-toggles-api/src/main/java/com/duckduckgo/feature/toggles/api/MetricsPixelPlugin.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.duckduckgo.feature.toggles.api

import java.time.ZonedDateTime
import java.time.format.DateTimeFormatter
import java.time.temporal.ChronoUnit

/**
* Experiment pixels that want to be fired should implement this plugin. The associated plugin point
Expand Down Expand Up @@ -45,7 +46,7 @@ data class MetricsPixel(
fun getPixelDefinitions(): List<PixelDefinition> {
val cohort = toggle.getRawStoredState()?.assignedCohort?.name.orEmpty()
val enrollmentDateET = toggle.getRawStoredState()?.assignedCohort?.enrollmentDateET?.let {
ZonedDateTime.parse(it).format(DateTimeFormatter.ISO_LOCAL_DATE)
ZonedDateTime.parse(it).truncatedTo(ChronoUnit.DAYS).format(DateTimeFormatter.ISO_LOCAL_DATE)
}.orEmpty()
if (cohort.isEmpty() || enrollmentDateET.isEmpty()) {
return emptyList()
Expand Down
3 changes: 2 additions & 1 deletion ...gles-impl/src/main/java/com/duckduckgo/feature/toggles/impl/RealFeatureTogglesCallback.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import com.squareup.anvil.annotations.ContributesBinding
import com.squareup.anvil.annotations.ContributesMultibinding
import java.time.ZonedDateTime
import java.time.format.DateTimeFormatter
import java.time.temporal.ChronoUnit
import javax.inject.Inject
import okio.ByteString.Companion.encode

Expand All @@ -43,7 +44,7 @@ class RealFeatureTogglesCallback @Inject constructor(
cohortName: String,
enrollmentDate: String,
) {
val parsedDate = ZonedDateTime.parse(enrollmentDate).format(DateTimeFormatter.ISO_LOCAL_DATE)
val parsedDate = ZonedDateTime.parse(enrollmentDate).truncatedTo(ChronoUnit.DAYS).format(DateTimeFormatter.ISO_LOCAL_DATE)
val params = mapOf("enrollmentDate" to parsedDate)
val pixelName = getPixelName(experimentName, cohortName)
val tag = "${pixelName}_$params".encode().md5().hex()
Expand Down
15 changes: 7 additions & 8 deletions ...in/java/com/duckduckgo/feature/toggles/impl/metrics/RetentionMetricsAtbLifecyclePlugin.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ import com.duckduckgo.app.di.AppCoroutineScope
import com.duckduckgo.app.statistics.api.AtbLifecyclePlugin
import com.duckduckgo.app.statistics.pixels.Pixel
import com.duckduckgo.di.scopes.AppScope
import com.duckduckgo.feature.toggles.api.MetricsPixel
import com.duckduckgo.feature.toggles.api.PixelDefinition
import com.duckduckgo.feature.toggles.impl.MetricsPixelStore
import com.duckduckgo.feature.toggles.impl.RetentionMetric.APP_USE
import com.duckduckgo.feature.toggles.impl.RetentionMetric.SEARCH
import com.squareup.anvil.annotations.ContributesMultibinding
import java.time.LocalDate
import java.time.ZoneId
import java.time.ZonedDateTime
import java.time.temporal.ChronoUnit
Expand All @@ -46,7 +46,7 @@ class RetentionMetricsAtbLifecyclePlugin @Inject constructor(
appCoroutineScope.launch {
searchMetricPixelsPlugin.getMetrics().forEach { metric ->
metric.getPixelDefinitions().forEach { definition ->
if (isInConversionWindow(definition)) {
if (isInConversionWindow(definition, metric)) {
store.getMetricForPixelDefinition(definition, SEARCH).takeIf { it < metric.value.toInt() }?.let {
store.increaseMetricForPixelDefinition(definition, SEARCH).takeIf { it == metric.value.toInt() }?.apply {
pixel.fire(definition.pixelName, definition.params)
Expand All @@ -62,7 +62,7 @@ class RetentionMetricsAtbLifecyclePlugin @Inject constructor(
appCoroutineScope.launch {
appUseMetricPixelsPlugin.getMetrics().forEach { metric ->
metric.getPixelDefinitions().forEach { definition ->
if (isInConversionWindow(definition)) {
if (isInConversionWindow(definition, metric)) {
store.getMetricForPixelDefinition(definition, APP_USE).takeIf { it < metric.value.toInt() }?.let {
store.increaseMetricForPixelDefinition(definition, APP_USE).takeIf { it == metric.value.toInt() }?.apply {
pixel.fire(definition.pixelName, definition.params)
Expand All @@ -74,8 +74,8 @@ class RetentionMetricsAtbLifecyclePlugin @Inject constructor(
}
}

private fun isInConversionWindow(definition: PixelDefinition): Boolean {
val enrollmentDate = definition.params["enrollmentDate"] ?: return false
private fun isInConversionWindow(definition: PixelDefinition, metric: MetricsPixel): Boolean {
val enrollmentDate = metric.toggle.getCohort()?.enrollmentDateET ?: return false
val lowerWindow = definition.params["conversionWindowDays"]?.split("-")?.first()?.toInt() ?: return false
val upperWindow = definition.params["conversionWindowDays"]?.split("-")?.last()?.toInt() ?: return false
val daysDiff = daysBetweenTodayAnd(enrollmentDate)
Expand All @@ -85,8 +85,7 @@ class RetentionMetricsAtbLifecyclePlugin @Inject constructor(

private fun daysBetweenTodayAnd(date: String): Long {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's one more copy of this function in MetricsPixelInterceptor that we should also adjust, shouldn't we?

Copy link
Contributor

@LukasPaczos LukasPaczos Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This one looks fine to stay because it takes truncated days as parameter 👍 It would be great to add a small comment to the function parameter so that someone stumbling on this in the future will immediately understand the difference between the way we parse and use the various enrollmentDate instances. Needs to get updated in the end, refs #5535 (comment).

val today = ZonedDateTime.now(ZoneId.of("America/New_York"))
val localDate = LocalDate.parse(date)
val zoneDateTime: ZonedDateTime = localDate.atStartOfDay(ZoneId.of("America/New_York"))
return ChronoUnit.DAYS.between(zoneDateTime, today)
val localDate = ZonedDateTime.parse(date)
return ChronoUnit.DAYS.between(localDate, today)
Copy link
Contributor

@LukasPaczos LukasPaczos Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This now returns different values than it used to.

Here's an example:

    @Test
    fun test() {
        val enrollmentTruncatedZDT = "2025-01-30"
        val enrollmentNotTruncatedZDT = "2025-01-30T18:05:28.548199-05:00[America/New_York]"

        // check on the next day when MORE than 24 hours since enrollment elapsed, both methods return 1 day diff
        var currentZDT = "2025-01-31T18:06:07.467047-05:00[America/New_York]"
        val old1 = oldDaysBetweenTodayAnd(enrollmentDate = enrollmentTruncatedZDT, currentZDTString = currentZDT)
        val new1 = daysBetweenTodayAnd(enrollmentDate = enrollmentNotTruncatedZDT, currentZDTString = currentZDT)
        assertEquals(old1, new1) // passes

        // check on the next day when LESS than 24 hours since enrollment elapsed, old method returns 1 while the new returns 0
        currentZDT = "2025-01-31T18:04:07.467047-05:00[America/New_York]"
        val old2 = oldDaysBetweenTodayAnd(enrollmentDate = enrollmentTruncatedZDT, currentZDTString = currentZDT)
        val new2 = daysBetweenTodayAnd(enrollmentDate = enrollmentNotTruncatedZDT, currentZDTString = currentZDT)
        assertEquals(old2, new2) // fails
    }

    private fun oldDaysBetweenTodayAnd(enrollmentDate: String, currentZDTString: String): Long {
        val today = ZonedDateTime.parse(currentZDTString)
        val localDate = LocalDate.parse(enrollmentDate)
        val zoneDateTime: ZonedDateTime = localDate.atStartOfDay(ZoneId.of("America/New_York"))
        return ChronoUnit.DAYS.between(zoneDateTime, today)
    }

    private fun daysBetweenTodayAnd(enrollmentDate: String, currentZDTString: String): Long {
        val today = ZonedDateTime.parse(currentZDTString)
        val localDate = ZonedDateTime.parse(enrollmentDate)
        return ChronoUnit.DAYS.between(localDate, today)
    }

If the aim of the framework was only to check for calendar day changes without accounting for hours, then the new iteration would regress by being too precise.

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 really need to be more precise, should be 24h diff rather than calendar days so the new one seems to work as expected compared to the old.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 In this case, #5535 (comment) would need to get updated in the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rolling back on this thought, it shouldn't be 24h so I'm changing this bit and leaving the other one as it is.

}
}
13 changes: 5 additions & 8 deletions .../java/com/duckduckgo/feature/toggles/codegen/ContributesRemoteFeatureCodeGeneratorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3321,14 +3321,11 @@ class ContributesRemoteFeatureCodeGeneratorTest {

// call isEnabled(cohort) to force cohort assignment
testFeature.fooFeature().isEnabled(CONTROL)
assertEquals(
Cohort(
name = "control",
weight = 1,
enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString(),
),
testFeature.fooFeature().getRawStoredState()!!.assignedCohort,
)

val date = testFeature.fooFeature().getRawStoredState()!!.assignedCohort?.enrollmentDateET
val parsedDate = ZonedDateTime.parse(date).truncatedTo(ChronoUnit.DAYS)
val now = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS)
assertEquals(now, parsedDate)
}

@Test
Expand Down
19 changes: 9 additions & 10 deletions ...gles-impl/src/test/java/com/duckduckgo/feature/toggles/impl/MetricPixelInterceptorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import com.duckduckgo.feature.toggles.codegen.TestTriggerFeature
import java.time.ZoneId
import java.time.ZonedDateTime
import java.time.format.DateTimeFormatter
import java.time.temporal.ChronoUnit
import org.junit.Assert.*
import org.junit.Before
import org.junit.Rule
Expand Down Expand Up @@ -43,7 +42,7 @@ class MetricPixelInterceptorTest {

@Test
fun `test metric exist but is not in conversion window drops pixel`() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
val enrollmentDateParsedET: String = ZonedDateTime.parse(enrollmentDateET).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

testFeature.experimentFooFeature().setRawStoredState(
Expand All @@ -69,7 +68,7 @@ class MetricPixelInterceptorTest {

@Test
fun `test metric exist and is in conversion window sends pixel`() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
val enrollmentDateParsedET: String = ZonedDateTime.parse(enrollmentDateET).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

testFeature.experimentFooFeature().setRawStoredState(
Expand All @@ -95,7 +94,7 @@ class MetricPixelInterceptorTest {

@Test
fun `test metric allows conversion window of one day`() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
val enrollmentDateParsedET: String = ZonedDateTime.parse(enrollmentDateET).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

testFeature.experimentFooFeature().setRawStoredState(
Expand All @@ -121,7 +120,7 @@ class MetricPixelInterceptorTest {

@Test
fun `test metric exist and is in conversion window from previous days sends pixel`() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).minusDays(4).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).minusDays(4).toString()
val enrollmentDateParsedET: String = ZonedDateTime.parse(enrollmentDateET).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

testFeature.experimentFooFeature().setRawStoredState(
Expand All @@ -147,7 +146,7 @@ class MetricPixelInterceptorTest {

@Test
fun `test pixel metric cannot be sent twice`() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
val enrollmentDateParsedET: String = ZonedDateTime.parse(enrollmentDateET).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

testFeature.experimentFooFeature().setRawStoredState(
Expand Down Expand Up @@ -177,7 +176,7 @@ class MetricPixelInterceptorTest {

@Test
fun `test metric doesnt exist drops pixels`() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
val enrollmentDateParsedET: String = ZonedDateTime.parse(enrollmentDateET).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

testFeature.experimentFooFeature().setRawStoredState(
Expand All @@ -203,7 +202,7 @@ class MetricPixelInterceptorTest {

@Test
fun `test value doesnt exist drops pixels`() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
val enrollmentDateParsedET: String = ZonedDateTime.parse(enrollmentDateET).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

testFeature.experimentFooFeature().setRawStoredState(
Expand All @@ -229,7 +228,7 @@ class MetricPixelInterceptorTest {

@Test
fun `test cohort name doesnt exist drops pixels`() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
val enrollmentDateParsedET: String = ZonedDateTime.parse(enrollmentDateET).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

testFeature.experimentFooFeature().setRawStoredState(
Expand All @@ -255,7 +254,7 @@ class MetricPixelInterceptorTest {

@Test
fun `test experiment name doesnt exist drops pixels`() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
val enrollmentDateParsedET: String = ZonedDateTime.parse(enrollmentDateET).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

testFeature.experimentFooFeature().setRawStoredState(
Expand Down
2 changes: 1 addition & 1 deletion ...-impl/src/test/java/com/duckduckgo/feature/toggles/impl/RealFeatureTogglesCallbackTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class RealFeatureTogglesCallbackTest {
callback.onCohortAssigned(
experimentName = "experimentName",
cohortName = "cohortName",
enrollmentDate = "2024-10-15T00:00-04:00[America/New_York]",
enrollmentDate = "2024-10-15T08:50:17.467-05:00[America/New_York]",
)
verify(pixel).fire(pixelName = pixelName, parameters = params, type = Unique(tag))
}
Expand Down
17 changes: 8 additions & 9 deletions ...ava/com/duckduckgo/feature/toggles/impl/metrics/RetentionMetricsAtbLifecyclePluginTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import com.duckduckgo.feature.toggles.impl.RetentionMetric
import java.time.ZoneId
import java.time.ZonedDateTime
import java.time.format.DateTimeFormatter
import java.time.temporal.ChronoUnit
import kotlinx.coroutines.test.runTest
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
Expand Down Expand Up @@ -90,7 +89,7 @@ class RetentionMetricsAtbLifecyclePluginTest {

@Test
fun `when search atb refreshed and matches metric and conversion window, pixel sent to all active experiments`() = runTest {
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
val parsedDate: String = ZonedDateTime.parse(today).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

setCohorts(today)
Expand All @@ -107,7 +106,7 @@ class RetentionMetricsAtbLifecyclePluginTest {

@Test
fun `when app use atb refreshed and no metric match conversion window, do not send pixels`() = runTest {
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
setCohorts(today)

atbLifecyclePlugin.onAppRetentionAtbRefreshed("", "")
Expand All @@ -116,7 +115,7 @@ class RetentionMetricsAtbLifecyclePluginTest {

@Test
fun `when search atb refreshed, fire all pixels which metric matches the number of searches done and conversion window`() = runTest {
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).minusDays(6).truncatedTo(ChronoUnit.DAYS).toString()
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).minusDays(6).toString()
val parsedDate: String = ZonedDateTime.parse(today).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

setCohorts(today)
Expand All @@ -140,7 +139,7 @@ class RetentionMetricsAtbLifecyclePluginTest {

@Test
fun `when app use atb refreshed, fire all pixels which metric matches the number of app use and conversion window`() = runTest {
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).minusDays(6).truncatedTo(ChronoUnit.DAYS).toString()
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).minusDays(6).toString()
val parsedDate: String = ZonedDateTime.parse(today).format(DateTimeFormatter.ISO_LOCAL_DATE).toString()

setCohorts(today)
Expand All @@ -164,7 +163,7 @@ class RetentionMetricsAtbLifecyclePluginTest {

@Test
fun `when search atb refreshed, only fire pixels with active experiments`() = runTest {
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
testFeature.experimentFooFeature().setRawStoredState(
State(
remoteEnableState = false,
Expand All @@ -188,7 +187,7 @@ class RetentionMetricsAtbLifecyclePluginTest {

@Test
fun `when search atb refreshed, only fire pixels from experiments with cohorts assigned`() = runTest {
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
testFeature.experimentFooFeature().setRawStoredState(
State(
remoteEnableState = true,
Expand All @@ -212,7 +211,7 @@ class RetentionMetricsAtbLifecyclePluginTest {

@Test
fun `when app use refreshed, only fire pixels with active experiments`() = runTest {
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).minusDays(1).truncatedTo(ChronoUnit.DAYS).toString()
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).minusDays(1).toString()

testFeature.experimentFooFeature().setRawStoredState(
State(
Expand All @@ -237,7 +236,7 @@ class RetentionMetricsAtbLifecyclePluginTest {

@Test
fun `when app use refreshed, only fire pixels from experiments with cohorts assigned`() = runTest {
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).minusDays(1).truncatedTo(ChronoUnit.DAYS).toString()
val today = ZonedDateTime.now(ZoneId.of("America/New_York")).minusDays(1).toString()

testFeature.experimentFooFeature().setRawStoredState(
State(
Expand Down
Loading