Attributed metrics module and skeleton #6887
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
cmonfortep
force-pushed
the
feature/cristian/attributed_metrics_module
branch
from
3309dae to
54292a7
Compare
| eventRepository.getEventStats(eventName, days) | ||
| } | ||
|
|
||
| // TODO: Pending adding default attributed metrics and removing default prefix from pixel names |
There was a problem hiding this comment.
This will happen on future PRs
cmonfortep
force-pushed
the
feature/cristian/attributed_metrics_module
branch
from
54292a7 to
fe91bc1
Compare
cmonfortep
force-pushed
the
feature/cristian/attributed_metrics_module
branch
from
e6b6bc4 to
831e0ec
Compare
cmonfortep
force-pushed
the
feature/cristian/attributed_metrics_module
branch
from
831e0ec to
c8c63fb
Compare
| @@ -0,0 +1,4 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
nit: I don't think we need the baseline file
There was a problem hiding this comment.
It was needed, lint failed and I had to create the baseline.
| generateDaggerFactories = true // default is false | ||
| } | ||
| lintOptions { | ||
| baseline file("lint-baseline.xml") |
There was a problem hiding this comment.
nit: don't think we need this since there are no linting issues
|
|
||
| package com.duckduckgo.app.attributed.metrics | ||
|
|
||
| internal interface AttributedMetricsState { |
There was a problem hiding this comment.
Kinda confused by this, you also have another AttributedMetricsState but with isActive... maybe the fact that the interface name is the same? Also not sure if this used?
There was a problem hiding this comment.
It can be removed, leftover when refactoring some code. It got replaced by the other one with same name.
|
|
||
| @ContributesBinding(AppScope::class) | ||
| class RealAttributedMetricsDateUtils @Inject constructor() : AttributedMetricsDateUtils { | ||
| override fun getCurrentDate(): String = getCurrentLocalDate().format(DATE_FORMATTER) |
There was a problem hiding this comment.
Didn't we say to use ET? Can't really remember but it rings a bell.
There was a problem hiding this comment.
We don't store time, just date. So this is just storing events on the device date. You mentioned ET time in one kickoff project, but we didn't discuss that further and I haven't considered that.
Trying to reason this, I believe what you say is: a user can install the app, and in their same day, they can receive 2 different atb because it's calculated by ET time, even for the user is same day. Is that correct? I can take that offline and revisit.
There was a problem hiding this comment.
Follow-up: we migrate to ET time.
https://app.asana.com/1/137249556945/project/72649045549333/task/1211587142871285?focus=true
|
|
||
| @Dao | ||
| interface EventDao { | ||
| @Transaction |
There was a problem hiding this comment.
Why is this one a transaction? I find it weird to have one on a normal query.
cmonfortep
force-pushed
the
feature/cristian/attributed_metrics_module
branch
from
0602cd7 to
1e2a8db
Compare
Task/Issue URL: https://app.asana.com/1/137249556945/project/1202552961248957/task/1211500171306010?focus=true
Description
Scope
Steps to test this PR
While testing this PR you need to add the following in your logcat filter:
tag~:"AttributedMetrics"Feature 1
New install detected, attributed metrics activeClient status running: true...attributedMetricsFFPrivacy config downloaded, attributed metrics enabled: false(that should disable the client)Client status running: false -> isActive: true, isEnabled: false...Feature 2
App reinstall detected, attributed metrics will not be activeClient status running: false -> isActive: false, isEnabled: true...UI changes