Added metrics for collectibles #21280
Conversation
vkjr
requested review from
shivekkhurana,
briansztamfater and
smohamedjavid
| @@ -45,3 +45,7 @@ | |||
| #(rf/dispatch [:profile.login/login-with-biometric-if-available | |||
| (get-in db [:profile/login :key-uid])])) | |||
| :shell? true}]]]}))) | |||
|
|
|||
| ;; Events do nothing but they will be intercepted and tracked | |||
| (rf/reg-event-fx :centralized-metrics/navigated-to-collectibles-tab (fn [])) | |||
There was a problem hiding this comment.
We have a choice - create events under specific namespace, like wallet/navigated-to-collectibles-tab, or under centralized-metrics. I selected the second one for the sake of code readability. Developer will immediately see that event exists for metrics.
There was a problem hiding this comment.
Hey, we should not be introducing new events. We can just hook in the :navigate-to event that already exists.
The idea with interceptors is to make the metrics layer invisible. We don't emit metric specific events, we just read other events, and track some to compute metrics.
Jenkins BuildsClick to see older builds (25)
|
vkjr
force-pushed
the
collectible-metrics
branch
from
0466ced
to
b93b5b9
Compare
| @@ -45,3 +45,7 @@ | |||
| #(rf/dispatch [:profile.login/login-with-biometric-if-available | |||
| (get-in db [:profile/login :key-uid])])) | |||
| :shell? true}]]]}))) | |||
|
|
|||
| ;; Events do nothing but they will be intercepted and tracked | |||
| (rf/reg-event-fx :centralized-metrics/navigated-to-collectibles-tab (fn [])) | |||
There was a problem hiding this comment.
Hey, we should not be introducing new events. We can just hook in the :navigate-to event that already exists.
The idea with interceptors is to make the metrics layer invisible. We don't emit metric specific events, we just read other events, and track some to compute metrics.
|
@shivekkhurana, switching to collectibles tab doesn't produce events because it happens within page and without re-frame usage |
|
I fear that if we have explicit metrics capturing events, then the code will become littered with these. In this particular case, one solution can be to lift the local state to Re-Frame. What do you think about it ? Update: |
vkjr
force-pushed
the
collectible-metrics
branch
3 times, most recently
from
2af004c
to
15bd27d
Compare
|
@shivekkhurana , metrics were rewritten to remove empty metrics-specific events. Navigation to collectible tab now uses re-frame subscription instead of local state to support this. |
| @@ -35,12 +35,16 @@ | |||
| :on-press #(rf/dispatch [:show-bottom-sheet {:content new-account}]) | |||
| :type :add-account}) | |||
|
|
|||
| (def first-tab-id :assets) | |||
There was a problem hiding this comment.
Did you lift state to re-frame ?
There was a problem hiding this comment.
yes
this is an initial state value for the case when there is no value in re-frame db. As soon as user selects any tab re-frame value will be used.
Similar approach used on account page
There was a problem hiding this comment.
@vkjr, I thought we had a place for setting up initial state. For the wallet it would be wallet/db.cljs. If we do that, then we can remove the or around the sub :wallet/home-tab.
|
I'm okay with this PR, just want someone else to review this too. I have one more question: Did you check that the event is being saved in mixpanel? |
|
@shivekkhurana, yes I checked, events appear in mixpanel |
vkjr
force-pushed
the
collectible-metrics
branch
from
15bd27d
to
774a80f
Compare
|
|
||
| (defn track-view-id-event | ||
| [view-id] | ||
| (when (contains? view-ids-to-track view-id) | ||
| (navigation-event (name view-id)))) | ||
|
|
||
| (defn collectilbes-fetched-event |
There was a problem hiding this comment.
Small typo: collectilbes
| [db] | ||
| (let [accounts (get-in db [:wallet :accounts]) | ||
| amount-on-all-accounts (reduce (fn [collectibles-amount account] | ||
| (+ collectibles-amount (:current-collectible-idx account))) |
There was a problem hiding this comment.
:current-collectible-idx is the index for the current collectible request and it varies. It doesn't necessarily reflect the total amount of collectibles for an account.
E.g., if we fetch collectibles by batches of 5, then this value will be 5, then when the user scrolls up to the end of the collectible listing, it'll be 10 and we'll fetch the following 5 collectibles, and so on. So this value will be:
5 -> 10 -> 15 -> 20 -> ...
I just wanted to clarify that this number will be small at the beginning and it changes, just in case this is not the intended purpose.
There was a problem hiding this comment.
Yes, it is intended. It doesn't reflect the total amount but reflects our current knowledge about account and I think that should be ok. As far as I know our api doesn't provide total amount of collectibles before we fetched them.
There was a problem hiding this comment.
I guess in this case the value of sending the "total amount" metric will be lost, since we won't know whether the users have x amount of collectibles, we'll just get the first fetch amount unless they scroll till the end. Is it even worth sending it? What will we learn from this metric?
| :wallet/select-account-tab | ||
| (when (= second-parameter :collectibles) | ||
| (navigated-to-collectibles-tab-event :account)) |
There was a problem hiding this comment.
Is this when a sign that this case should become a cond?
What could second-parameter be besides :collectibles? If possible, it'd be great to rename it, second-parameter provides zero context, it's almost the same as name it parameter 😞
There was a problem hiding this comment.
case here checks the event-name parameter, while second-parameter can be any because it is the second parameter of re-frame event that we are intercepting (agree that new name needed). For different case clauses second-parameter can be different or absent. So I'm not sure cond will work here because condition will look like (and (= event-name :wallet/select-account-tab) (= second-parameter :collectibles). Wdyt?
There was a problem hiding this comment.
If possible, it'd be great to rename it, second-parameter provides zero context, it's almost the same as name it parameter
@ulisesmac, the name second-parameter was chosen intentionally because it signals that we don't know what it is, it could be anything and that it is only the second (could be a third and so on). We can rename it to something else of course, but it will always be generic because the value varies per event. Please, see my other comment for the function tracked-event where I give an alternative to how we could design the code.
| [init-loaded? set-init-loaded] (rn/use-state false) | ||
| {:keys [formatted-balance]} (rf/sub [:wallet/aggregated-token-values-and-balance]) | ||
| theme (quo.theme/use-theme)] | ||
| (let [selected-tab (or (rf/sub [:wallet/home-tab]) first-tab-id) |
There was a problem hiding this comment.
The expression:
(or (rf/sub[...]) value-if-not-found)
Seems not necessary, since this is a new sub and it'll be cleaner if all the logic is wrapped within the sub.
We have a def for first-tab-id, instead of putting it in the view, we can define it near the subscription, so the sub directly returns.
Wdyt?
| (defn tracked-event | ||
| [[event-name second-parameter]] | ||
| [[event-name second-parameter] db] |
There was a problem hiding this comment.
I'm not sure this is the right way of extending this API, it is becoming more restrictive instead of more flexible.
IMO, we could create a two-arity version instead, since with this change, we call it only once as:
(tracked-event '[...] db)
And multiple times as:
(tracked-event '[...] {})
And... actually, this second-parameter value is also not passed to the function call sometimes 🤔
I think the signature could have been the following since the beginning:
(defn tracked-event
[event-name & {:keys[...] :as opts}]
...)CC: @ilmotta
There was a problem hiding this comment.
Thanks for tagging me @ulisesmac. Below I share a few roughly sketched ideas, not blocking the PR from my PoV. Text is long, hard to explain, but I hope it makes some sense.
I think it's not the best that the tracking code now has access to the entire app-db and knows the inner details of its structure. Better I would say we should give the least amount of data to this layer and keep it plain simple, also because it's not well QAed.
If we need to compute something from the app-db that needs to be tracked, and which is decoupled from the metrics layer we could:
- Define a root key in the app-db, for example,
:centralized-metrics/event-data, which will store any arbitrary data that's safe to be tracked and which is serializable to JSON. - The event that wants to be tracked can
assocto this root key. In this case, in event:wallet/flush-collectibles-fetchedit wouldassocthe total number of collectibles. - The interceptor
centralized-metrics-interceptorwill then extract the value of:centralized-metrics/event-dataand pass it totracking/tracked-event.
With this solution, the centralized metrics layer will not know how to compute anything, but will still be able to get computed track data and/or arguments passed to any re-frame event. This would scale to any other event in re-frame and we would still keep this layer simple/dumb.
About the signature of the function tracked-event, an alternative is to implement tracked-event with the case macro, but the event argument (first arg to tracked-event) would be passed to downstream functions, and then we would only destructure the re-frame event in each specific function signature, thus solving the naming problem of second-parameter or how to best destructure the event because each function will destructure and name the arguments knowing what they really are.
In this example, there's no generic name like second-parameter and we don't need to worry about the full signature of the re-frame event:
(defn- metrics-enabled-event
[[_ enabled?]]
(key-value-event "events.metrics-enabled" :enabled enabled?))
(defn tracked-event
[[event-name :as event] event-data]
(case event-name
:profile/get-profiles-overview-success
(user-journey-event app-started-event)
:centralized-metrics/toggle-centralized-metrics
(metrics-enabled-event event)
:set-view-id
(track-view-id-event event)
(:wallet/select-account-tab :wallet/select-home-tab)
(navigated-to-collectibles-tab-event event)
;; => NOTE: Here `event-data` was computed outside the centralized metrics layer and was extracted by the global interceptor when calling tracked-event
:wallet/flush-collectibles-fetched
(collectilbes-fetched-event event-data)
nil))There was a problem hiding this comment.
Thanks for tagging me @ulisesmac. Below I share a few roughly sketched ideas, not blocking the PR from my PoV. Text is long, hard to explain, but I hope it makes some sense.
I think it's not the best that the tracking code now has access to the entire app-db and knows the inner details of its structure. Better I would say we should give the least amount of data to this layer and keep it plain simple, also because it's not well QAed.
If we need to compute something from the app-db that needs to be tracked, and which is decoupled from the metrics layer we could:
- Define a root key in the app-db, for example,
:centralized-metrics/event-data, which will store any arbitrary data that's safe to be tracked and which is serializable to JSON.- The event that wants to be tracked can
assocto this root key. In this case, in event:wallet/flush-collectibles-fetchedit wouldassocthe total number of collectibles.- The interceptor
centralized-metrics-interceptorwill then extract the value of:centralized-metrics/event-dataand pass it totracking/tracked-event.With this solution, the centralized metrics layer will not know how to compute anything, but will still be able to get computed track data and/or arguments passed to any re-frame event. This would scale to any other event in re-frame and we would still keep this layer simple/dumb.
About the signature of the function
tracked-event, an alternative is to implementtracked-eventwith thecasemacro, but theeventargument (first arg totracked-event) would be passed to downstream functions, and then we would only destructure the re-frame event in each specific function signature, thus solving the naming problem ofsecond-parameteror how to best destructure the event because each function will destructure and name the arguments knowing what they really are.In this example, there's no generic name like
second-parameterand we don't need to worry about the full signature of the re-frame event:(defn- metrics-enabled-event [[_ enabled?]] (key-value-event "events.metrics-enabled" :enabled enabled?)) (defn tracked-event [[event-name :as event] event-data] (case event-name :profile/get-profiles-overview-success (user-journey-event app-started-event) :centralized-metrics/toggle-centralized-metrics (metrics-enabled-event event) :set-view-id (track-view-id-event event) (:wallet/select-account-tab :wallet/select-home-tab) (navigated-to-collectibles-tab-event event) ;; => NOTE: Here `event-data` was computed outside the centralized metrics layer and was extracted by the global interceptor when calling tracked-event :wallet/flush-collectibles-fetched (collectilbes-fetched-event event-data) nil))
Thanks for sharing your thoughts @ilmotta
I agree with you about not passing db to this event. I'm still not sure about growing app-db horizontally instead of nested, but that's a conversation for another moment.
There was a problem hiding this comment.
For this solution I proposed to work the app-db key would need to be separate from everything else because that would be the only way to create a single abstraction that works for every re-frame event we might want to track. For instance, if we store the computation for MixPanel events nested in other parts of the app-db, we will be in trouble because the interceptor would need to know how to reach the app-db leaf for various events, then it will complicate the interceptor because it wouldn't know anymore how to get data generically.
Maybe I'm missing something, but this solution wouldn't grow the app-db horizontally, just one extra key for any and all events that need to send computed data to MixPanel.
There was a problem hiding this comment.
@ilmotta, I have one more thought. Personally I would be happy to avoid both - referencing db from metrics namespace and having a separate key in re-frame db, because it should be not only set but also cleaned after every event to make sure that intercepted event didn't get wrong data from previous event. And some events just don't need it.
The way that seems more clean for me - is having dedicated event every time when we need some additional data to be computed from db and intercepting that event.
For example in current case when :wallet/flush-collectibles-fetched event fired it can dispatch one more event - :metrics/track-user-has-collectibles? which receives parameter :has-collectibles? as an argument and this event we can intercept from metrics layer.
So flush-collectibles will become:
(defn flush-collectibles
[{:keys [db]}]
(let [collectibles-per-account (get-in db [:wallet :ui :collectibles :fetched])
accounts (get-in db [:wallet :accounts])
has-collectibles? (some #(pos? (:current-collectible-idx %)) (vals accounts))]
{:db (-> db
(update-in [:wallet :ui :collectibles] dissoc :pending-requests :fetched)
(update-in [:wallet :accounts] move-collectibles-to-accounts collectibles-per-account))
:fx [:dispatch
[:metrics/track-user-has-collectibles? has-collectibles?]]}))We discussed with @shivekkhurana that he would prefer not to pollute our events code with metrics-related calculations but if we anyway need some calculations to be done, it better be done not in the metrics namespace as you suggested, and i think it is better to be in a form of separate event. It name would provide a very clear explanation of what is going on here, more high-level than plain assoc-in.
Wdyt?
There was a problem hiding this comment.
because it should be not only set but also cleaned after every event
Good point @vkjr about clean-up. We can use the interceptor itself to clean up because the context we receive has the :db effect and we can grab the value of :centralized-metrics/event-data, use it to generate the MixPanel event and then return a new context with the :db effect dissoc'ing :centralized-metrics/event-data data key. The result is that nothing will leak to another event and, actually, the app-db won't be modified to persist the computed event data. No subscription recomputations should happen if we do this correctly.
I also agree with @shivekkhurana that we shouldn't pollute normal code with metrics code and that it's highly desirable to keep the metrics layer dumb.
I think the idea of using separate events can work well 👍🏼, but I would personally use that only when necessary because of the overhead and extra verbosity. For example, in the example you provided, a simple assoc gets the job done and it's decent for readability. Also considering the clean-up will be automatic by the interceptor.
Including event data computation in the tracked re-frame event is not bad actually, there's an advantage, the computation can be unit tested with the rest of the handler, which is actually quite important because if we start to track incorrect computations it will really complicate analysis in MixPanel and I doubt QAs will have the time to manually double-check metrics are generated correctly. Actually unfair to ask them to do that.
(defn flush-collectibles
[{:keys [db]}]
(let [collectibles-per-account (get-in db [:wallet :ui :collectibles :fetched])
accounts (get-in db [:wallet :accounts])
has-collectibles? (some #(pos? (:current-collectible-idx %)) (vals accounts))]
{:db (-> db
(update-in [:wallet :ui :collectibles] dissoc :pending-requests :fetched)
(update-in [:wallet :accounts] move-collectibles-to-accounts collectibles-per-account)
(assoc :centralized-metrics/event-data has-collectibles?))}))
|
@ulisesmac, thanks a lot for the review, all your comments are highly relevant! as always ;) |
| @@ -35,12 +35,16 @@ | |||
| :on-press #(rf/dispatch [:show-bottom-sheet {:content new-account}]) | |||
| :type :add-account}) | |||
|
|
|||
| (def first-tab-id :assets) | |||
There was a problem hiding this comment.
@vkjr, I thought we had a place for setting up initial state. For the wallet it would be wallet/db.cljs. If we do that, then we can remove the or around the sub :wallet/home-tab.
| :wallet/select-account-tab | ||
| (when (= second-parameter :collectibles) | ||
| (navigated-to-collectibles-tab-event :account)) |
There was a problem hiding this comment.
If possible, it'd be great to rename it, second-parameter provides zero context, it's almost the same as name it parameter
@ulisesmac, the name second-parameter was chosen intentionally because it signals that we don't know what it is, it could be anything and that it is only the second (could be a third and so on). We can rename it to something else of course, but it will always be generic because the value varies per event. Please, see my other comment for the function tracked-event where I give an alternative to how we could design the code.
| (navigated-to-collectibles-tab-event :home)) | ||
|
|
||
| :wallet/flush-collectibles-fetched | ||
| (collectilbes-fetched-event db) |
There was a problem hiding this comment.
@vkjr @shivekkhurana, why do we need to track the total number of collectibles from users? Which business answers we intend to answer from collecting this? I'm asking this just so we consider if we can collect less data that can be attributed to any individual, but which would still be valuable to learn.
There was a problem hiding this comment.
Tracking this parameter was solely my initiative and as a developer I felt comfortable to track as much info as possible :) But agree that if we intend to track less data then this is a wrong approach.
But I think it would be valuable to know if user has any collectibles or not. We can pass a boolean parameter to event, like :has-collectibles?. @shivekkhurana, @ilmotta, wdyt?
There was a problem hiding this comment.
I agree with you @vkjr, knowing if users have collectibles is useful and it's a good middle ground in terms of privacy because knowing exactly how many collectibles users have seem not super necessary.
There was a problem hiding this comment.
The number of collectibles tell us if people are using the NFT features. If there are no collectibles for the majority of the users, then this screen becomes less important.
| (defn tracked-event | ||
| [[event-name second-parameter]] | ||
| [[event-name second-parameter] db] |
There was a problem hiding this comment.
Thanks for tagging me @ulisesmac. Below I share a few roughly sketched ideas, not blocking the PR from my PoV. Text is long, hard to explain, but I hope it makes some sense.
I think it's not the best that the tracking code now has access to the entire app-db and knows the inner details of its structure. Better I would say we should give the least amount of data to this layer and keep it plain simple, also because it's not well QAed.
If we need to compute something from the app-db that needs to be tracked, and which is decoupled from the metrics layer we could:
- Define a root key in the app-db, for example,
:centralized-metrics/event-data, which will store any arbitrary data that's safe to be tracked and which is serializable to JSON. - The event that wants to be tracked can
assocto this root key. In this case, in event:wallet/flush-collectibles-fetchedit wouldassocthe total number of collectibles. - The interceptor
centralized-metrics-interceptorwill then extract the value of:centralized-metrics/event-dataand pass it totracking/tracked-event.
With this solution, the centralized metrics layer will not know how to compute anything, but will still be able to get computed track data and/or arguments passed to any re-frame event. This would scale to any other event in re-frame and we would still keep this layer simple/dumb.
About the signature of the function tracked-event, an alternative is to implement tracked-event with the case macro, but the event argument (first arg to tracked-event) would be passed to downstream functions, and then we would only destructure the re-frame event in each specific function signature, thus solving the naming problem of second-parameter or how to best destructure the event because each function will destructure and name the arguments knowing what they really are.
In this example, there's no generic name like second-parameter and we don't need to worry about the full signature of the re-frame event:
(defn- metrics-enabled-event
[[_ enabled?]]
(key-value-event "events.metrics-enabled" :enabled enabled?))
(defn tracked-event
[[event-name :as event] event-data]
(case event-name
:profile/get-profiles-overview-success
(user-journey-event app-started-event)
:centralized-metrics/toggle-centralized-metrics
(metrics-enabled-event event)
:set-view-id
(track-view-id-event event)
(:wallet/select-account-tab :wallet/select-home-tab)
(navigated-to-collectibles-tab-event event)
;; => NOTE: Here `event-data` was computed outside the centralized metrics layer and was extracted by the global interceptor when calling tracked-event
:wallet/flush-collectibles-fetched
(collectilbes-fetched-event event-data)
nil))
|
@ilmotta, I like the approach you suggested, thanks! |
vkjr
force-pushed
the
collectible-metrics
branch
2 times, most recently
from
7d7f46b
to
8357b42
Compare
|
@ilmotta, @ulisesmac, code updated, please take a look
|
vkjr
force-pushed
the
collectible-metrics
branch
from
2925057
to
1d05775
Compare
There was a problem hiding this comment.
@ilmotta, @ulisesmac, code updated, please take a look
- event stores calculated data in dedicated re-frame db key
- metrics take data from this key and cleanup after (p.s we use db from :effects now, because :coeffects don't have changes inroduced by event)
tracking-eventrenamed tometrics-eventfor clarity and takes only one arg - map with keys to solve problem when keys aren't always required
Nice improvements in this PR! I left a few other suggestions @vkjr, please see what makes sense to you.
FYI @seanstrom your PR #21328 and @vkjr's PR are incompatible, so I'm adding you as a reviewer for the sake of visibility.
| (log/debug "tracking event" metrics-event) | ||
| (when (push-event? rf-db) | ||
| (native-module/add-centralized-metric metrics-event))) | ||
| (interceptor/assoc-effect context :db (dissoc rf-db :centralized-metrics/event-data)))) |
There was a problem hiding this comment.
Interestingly, there's interceptor/update-effect.
|
|
||
| (defn navigated-to-collectibles-tab-event | ||
| [location] | ||
| (key-value-event "navigated-to-collectibles-tab" :location location)) |
There was a problem hiding this comment.
Reducing the number of events can be advantageous for simplifying managing the reports in MixPanel.
I think the original implementation of navigation-event is too limiting because it hardcodes the viewId, but from the perspective of analytics maintainers (not just devs), events like opening tabs, modals, bottom sheets are also navigation events.
The diff below is untested, but is simple enough. Sorry if I'm being too detailed, but the thing with metrics is that we can't evolve the models easily after events are published to MixPanel. This is just a suggestion based on my experience with these tools, but your solution also works.
diff --git a/src/status_im/contexts/centralized_metrics/tracking.cljs b/src/status_im/contexts/centralized_metrics/tracking.cljs
index 41e2b4a002..57e1086865 100644
--- a/src/status_im/contexts/centralized_metrics/tracking.cljs
+++ b/src/status_im/contexts/centralized_metrics/tracking.cljs
@@ -4,20 +4,20 @@
[react-native.platform :as platform]))
(defn key-value-event
- [event-name val-key value]
+ [event-name event-value]
{:metric
{:eventName event-name
:platform platform/os
:appVersion build/app-short-version
- :eventValue {val-key value}}})
+ :eventValue event-value}})
(defn user-journey-event
[action]
- (key-value-event "user-journey" :action action))
+ (key-value-event "user-journey" {:action action}))
(defn navigation-event
- [view-id]
- (key-value-event "navigation" :viewId view-id))
+ [m]
+ (key-value-event "navigation" m))
(def ^:const app-started-event "app-started")
@@ -51,15 +51,15 @@
(defn track-view-id-event
[view-id]
(when (contains? view-ids-to-track view-id)
- (navigation-event (name view-id))))
+ (navigation-event {:viewId (name view-id)})))
(defn collectibles-fetched-event
[has-collectibles?]
- (key-value-event "collectibles-fetched" :has-collectibles has-collectibles?))
+ (key-value-event "collectibles-fetched" {:has-collectibles has-collectibles?}))
(defn navigated-to-collectibles-tab-event
[location]
- (key-value-event "navigated-to-collectibles-tab" :location location))
+ (navigation-event {:context "wallet.collectibles" :location location}))
(defn metrics-event
[{:keys [rf-event metrics-data]}]
@@ -69,7 +69,7 @@
(user-journey-event app-started-event)
:centralized-metrics/toggle-centralized-metrics
- (key-value-event "events.metrics-enabled" :enabled rf-event-parameter)
+ (key-value-event "events.metrics-enabled" {:enabled rf-event-parameter})
:set-view-id
(track-view-id-event rf-event-parameter)
diff --git a/src/status_im/contexts/centralized_metrics/tracking_test.cljs b/src/status_im/contexts/centralized_metrics/tracking_test.cljs
index 35c197a64b..849083d559 100644
--- a/src/status_im/contexts/centralized_metrics/tracking_test.cljs
+++ b/src/status_im/contexts/centralized_metrics/tracking_test.cljs
@@ -18,7 +18,7 @@
:platform platform-os
:appVersion app-version
:eventValue {val-key value}}}]
- (is (= expected (tracking/key-value-event event-name val-key value))))))
+ (is (= expected (tracking/key-value-event event-name {val-key value}))))))
(deftest user-journey-event-test
(testing "creates correct user journey event"
@@ -38,7 +38,7 @@
:platform platform-os
:appVersion app-version
:eventValue {:viewId view-id}}}]
- (is (= expected (tracking/navigation-event view-id))))))
+ (is (= expected (tracking/navigation-event {:viewId view-id}))))))
(deftest track-view-id-event-test
(testing "returns correct navigation event for view-id"
There was a problem hiding this comment.
for key-value-event func i prefer @clauxx suggestion, as for the task i'm working i need to send more data and we can do it without breaking other usage
(defn key-value-event
[event-name & args]
(if (odd? (count args))
(throw (ex-info "Expected even number of args"
{:args args
:n-args (count args)}))
{:metric
{:eventName event-name
:platform platform/os
:appVersion build/app-short-version
:eventValue (apply hash-map args)}}))There was a problem hiding this comment.
@mohsen-ghafouri What are the benefits of using a var args signature to mimic a map and build it manually with hash-map if we can pass a map structure directly as in the diff above? Also there would be no need to check the args are pairs because the compiler would guarantee that. throw we should almost never use because it breaks the experience for the user.
There was a problem hiding this comment.
Just for avoid breaking other usages, but as we only used here yeah we can apply changes what we want.
There was a problem hiding this comment.
Your suggestion make sense, I applied in my PR
| (key-value-event "navigated-to-collectibles-tab" :location location)) | ||
|
|
||
| (defn metrics-event | ||
| [{:keys [rf-event metrics-data]}] |
There was a problem hiding this comment.
Good idea of renaming to rf-* to bring clarity to the two types of events 👍🏼
| (let [collectibles-per-account (get-in db [:wallet :ui :collectibles :fetched]) | ||
| updated-accounts (move-collectibles-to-accounts (get-in db [:wallet :accounts]) | ||
| collectibles-per-account) | ||
| has-collectibles? (some (fn [account] |
There was a problem hiding this comment.
A slight improvement is to skip these computations unless centralized-metrics/push-event? is true. To avoid dependencies between event namespaces, the function push-event? could be moved to a new file src/status_im/contexts/centralized_metrics/data_store.cljs (because these data store namespaces are kind of supposed to be required from anywhere).
| metrics-event-data (get rf-db :centralized-metrics/event-data)] | ||
| (when-let [metrics-event (tracking/metrics-event {:rf-event rf-event | ||
| :metrics-data metrics-event-data})] | ||
| (log/debug "tracking event" metrics-event) |
There was a problem hiding this comment.
FYI: The call to add-centralized-metric will eventually trigger processMetrics() in status-go, which will log the metrics when they are processed (approximately every 10s). Thus, this log call is kind of redundant. https://github.com/status-im/status-go/blob/5500fa04b53a32296715d14ee66303d48e723323/centralizedmetrics/metrics.go#L119
|
Wdyt if instead of adding extra data in (rf/reg-event-fx :centralized-metrics/track-event
(fn [{:keys [db]} [event-name metric-data]]
... |
Which places we don't have access to the
What would be the body of this event handler? I ask that because there's nothing to do in the event if |
|
@ilmotta, I initially made a quick assumption about the trigger metric from the view, but after further discussion, it seems I can achieve my goals by creating -success and -fail events for tracking. Here’s a draft PR: #21361. I used Volodymyr’s approach to pass extra data, so for now, I think we don’t need any specific event for tracking. |
| {:db (-> db | ||
| (update-in [:wallet :ui :collectibles] dissoc :pending-requests :fetched) | ||
| (update-in [:wallet :accounts] move-collectibles-to-accounts collectibles-per-account))})) | ||
| (assoc-in [:wallet :accounts] updated-accounts) | ||
| (assoc :centralized-metrics/event-data has-collectibles?))})) |
There was a problem hiding this comment.
It feels odd to me that we're assoc-ing additional data for metrics in the db here to pass extra data to the interceptor. Since we're already coupling the event itself with the metrics, i'd consider to directly send the metric with a separate event and not use the interceptor at all in such cases. I think it will be cleaner, less indirect and easier to understand what metrics are sent e.g.
{:db (-> ...)
:fx [[:dispatch [:centralized-metrics/send-metric "flush-collectibles" {:has-collectibles? has-collectibles?}]]]}I think the interceptor is good for cases that are more generic e.g. navigation, but given that we might have more complex requirements for metrics, which might need extra computation/data (like we have here) that is unavailable in the event args, maybe we should consider alternative approaches. Also, the way it's done here mixes the data required for the event and the data needed for metrics, which IMO should be kept apart.
There was a problem hiding this comment.
It feels odd to me that we're assoc-ing additional data for metrics in the db here to pass extra data to the interceptor.
It is odd indeed, there's more than one way to solve the problem. And coming back to the problem, I initially started suggesting a different solution from the original in the PR because the context/centralized_metrics/* namespaces shouldn't know how to compute anything from other domains, therefore, the data should come already cooked and mostly ready to be sent to MixPanel.
Dispatching a separate event as you suggest can work well, I agree 👍🏼 What should the handler body look like? I'm imagining the body would use a new re-frame effect to execute the effectful call to native-module/add-centralized-metric (basically what we do in the global interceptor).
less indirect and easier to understand what metrics
It's less magical, but it's more indirect technically because it will go through the whole re-frame loop and have more overhead (although marginal).
Also, the way it's done here mixes the data required for the event and the data needed for metrics
I don't see how that's different than the alternative you suggested. The computed data is still present in the same event handler. One solution assocs to a completely separate root key, so it's not mixed with the event data. The alternative you provided sends the data to a separate event, but the computation still happens in the originating event. Both solutions couple in the correct place IMO.
I don't have any strong opinion tbh @clauxx, my only more strong opinion was that we shouldn't compute business data in the centralized_metrics/ namespaces and your solution also solves that 👍🏼
Would be helpful for @vkjr if you could provide the full implementation suggestion so that we can get this PR merged soon. Maybe pair with him since you have the idea in your head?
There was a problem hiding this comment.
@ilmotta afaik @vkjr is off till the end of the week, but we discussed with @mohsen-ghafouri, and he will make a draft PR with a more specific implementation of what I explained, but for dapps metrics, and we can discuss it there.
There was a problem hiding this comment.
Sounds great, thanks @clauxx and @mohsen-ghafouri
|
Hi everyone!
I’ve tried to summarise all the main ideas we had along the way because re-reading all discussions in all the PRs will be tedious for someone who will decide to dive here :) So the main discussion happens around a way how data should be passed to metrics. Originally @shivekkhurana was against introducing metric-specific events and proposed to use only existing events and their data. But looks like some events still need a piece of data that calculated solely for metrics. Area where everyone has no doubts - metrics namespace shouldn’t do any data preparation on itself and only pass the existing data further. Regarding the way of passing computed data to metrics layer:
Also currently we have 3 PRs:
@seanstrom's PR #21328
@mohsen-ghafouri's PR #21379
As far as I understood we lean toward having a single metric event that carries data. P.S. |
|
Hey @vkjr Next time, I’ll create a separate commit for the core part so others can easily pick it up and continue their work. I will try to finalize is as soon as I fix my local setup |
fixes #21279
Summary
Added tracking for events:
Review notes
Platforms
Areas that maybe impacted
Functional
status: ready