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
Prev Previous commit
Calculate conversion window based on days rather than real 24h
  • Loading branch information
@marcosholgado
marcosholgado committed Jan 28, 2025
commit 9b0bea8dff587207ded76e3c6dbb94b1197e5377
3 changes: 1 addition & 2 deletions app/src/test/java/com/duckduckgo/app/brokensite/api/BrokenSiteSubmitterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ import com.duckduckgo.privacyprotectionspopup.api.PrivacyProtectionsPopupExperim
import com.squareup.moshi.Moshi
import java.time.ZoneId
import java.time.ZonedDateTime
import java.time.temporal.ChronoUnit
import java.util.*
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.test.TestScope
Expand Down Expand Up @@ -623,7 +622,7 @@ class BrokenSiteSubmitterTest {
}

private fun assignToExperiment() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
testBlockListFeature.tdsNextExperimentTest().setRawStoredState(
State(
remoteEnableState = true,
Expand Down
3 changes: 1 addition & 2 deletions app/src/test/java/com/duckduckgo/app/browser/refreshpixels/RefreshPixelSenderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import com.duckduckgo.feature.toggles.impl.RealFeatureTogglesInventory
import com.squareup.moshi.Moshi
import java.time.ZoneId
import java.time.ZonedDateTime
import java.time.temporal.ChronoUnit
import junit.framework.TestCase.assertNull
import junit.framework.TestCase.assertTrue
import kotlinx.coroutines.test.runTest
Expand Down Expand Up @@ -307,7 +306,7 @@ class RefreshPixelSenderTest {
}

private fun assignToExperiment() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
testBlockListFeature.tdsNextExperimentTest().setRawStoredState(
State(
remoteEnableState = true,
Expand Down
3 changes: 1 addition & 2 deletions ...st/java/com/duckduckgo/app/trackerdetection/blocklist/BlockListPrivacyTogglePluginTest.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.privacy.dashboard.api.PrivacyToggleOrigin.MENU
import com.squareup.moshi.Moshi
import java.time.ZoneId
import java.time.ZonedDateTime
import java.time.temporal.ChronoUnit
import kotlinx.coroutines.test.runTest
import org.junit.Before
import org.junit.Rule
Expand Down Expand Up @@ -91,7 +90,7 @@ class BlockListPrivacyTogglePluginTest {
}

private fun assignToExperiment() {
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).truncatedTo(ChronoUnit.DAYS).toString()
val enrollmentDateET = ZonedDateTime.now(ZoneId.of("America/New_York")).toString()
testBlockListFeature.tdsNextExperimentTest().setRawStoredState(
State(
remoteEnableState = true,
Expand Down
15 changes: 8 additions & 7 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, metric)) {
if (isInConversionWindow(definition)) {
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, metric)) {
if (isInConversionWindow(definition)) {
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, metric: MetricsPixel): Boolean {
val enrollmentDate = metric.toggle.getCohort()?.enrollmentDateET ?: return false
private fun isInConversionWindow(definition: PixelDefinition): Boolean {
val enrollmentDate = definition.params["enrollmentDate"] ?: 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,7 +85,8 @@ 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 = ZonedDateTime.parse(date)
return ChronoUnit.DAYS.between(localDate, today)
val localDate = LocalDate.parse(date)
val zoneDateTime: ZonedDateTime = localDate.atStartOfDay(ZoneId.of("America/New_York"))
return ChronoUnit.DAYS.between(zoneDateTime, today)
}
}
Loading