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.

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.

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.

Did you lift state to re-frame ?

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

@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.

Small typo: collectilbes

: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.

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.

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?

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 😞

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?

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.

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?

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

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:

1. 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.
2. The event that wants to be tracked can assoc to this root key. In this case, in event :wallet/flush-collectibles-fetched it would assoc the total number of collectibles.
3. The interceptor centralized-metrics-interceptor will then extract the value of :centralized-metrics/event-data and pass it to tracking/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))

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:

1. 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.
2. The event that wants to be tracked can assoc to this root key. In this case, in event :wallet/flush-collectibles-fetched it would assoc the total number of collectibles.
3. The interceptor centralized-metrics-interceptor will then extract the value of :centralized-metrics/event-data and pass it to tracking/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))

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.

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.

@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?

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?))}))

@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.

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.

@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.

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?

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.

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.

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:

1. 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.
2. The event that wants to be tracked can assoc to this root key. In this case, in event :wallet/flush-collectibles-fetched it would assoc the total number of collectibles.
3. The interceptor centralized-metrics-interceptor will then extract the value of :centralized-metrics/event-data and pass it to tracking/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))