Financial-Times / o-tracking

Origami Tracking component
http://registry.origami.ft.com/components/o-tracking
6 stars 8 forks source link

Do not send multiple page-view events if the new event is the exact same as the previously sent event #298

Closed JakeChampion closed 4 years ago

JakeChampion commented 4 years ago

Fixes https://github.com/Financial-Times/o-tracking/issues/296

origamiserviceuser commented 4 years ago

o-tracking bundle size difference from 2.0.10 to d2988e5fba32a750635158a71c5d7825f8477095 js: 5.28kb increase (1.61kb/gzip)

chrisbrownuk commented 4 years ago

@JakeChampion Please could you clarify if this will only remove events where the context.id, and everything else, are the same. It looks like that's the case to me. The issue #296 had lots of events in rapid succession but they had different context.id and context.root_id values.

JakeChampion commented 4 years ago

@JakeChampion Please could you clarify if this will only remove events where the context.id, and everything else, are the same. It looks like that's the case to me.

Yes, that is the case.

The issue #296 had lots of events in rapid succession but they had different context.id and context.root_id values.

Hmm, that would require a bit more work to solve in o-tracking, @chrisbrownuk do you think we should also solve that situation in o-tracking?

chrisbrownuk commented 4 years ago

Thanks for clarifying @JakeChampion .

We do need to find a way to stop the issue happening. It would be better to understand the cause so that we can put a specific fix in place, but this has been an occasional but recurring problem for years now and we still haven't worked out for sure why it happens.

We will need to be very careful in removing page view events in o-tracking. Removing ajacent events that only differ by context.id and context.root_id would remove valid manual page refreshes. So we would definitely need to consider the difference in timestamp and set a suitable threshold between adjacent page view events. (Also possibly consider the number generated, but that'll be of little use in real time, since it's too late once they've been sent - I guess we could at least cap the number.)

JakeChampion commented 4 years ago

We will need to be very careful in removing page view events in o-tracking. Removing ajacent events that only differ by context.id and context.root_id would remove valid manual page refreshes.

If the page is refreshed, then the local variable in o-tracking which keeps track of the previously sent page event would also be refreshed 👍

chrisbrownuk commented 4 years ago

If the page is refreshed, then the local variable in o-tracking which keeps track of the previously sent page event would also be refreshed 👍

Ah yes. True.

jaya-ananthram-ft commented 4 years ago

Hi @chrisbrownuk @JakeChampion , Just for my understanding, this is the root cause of the issue https://financialtimes.slack.com/archives/C1WF81SR3/p1585756523025700?thread_ts=1585737775.022200&cid=C1WF81SR3, right?

JakeChampion commented 4 years ago

Hi @chrisbrownuk @JakeChampion , Just for my understanding, this is the root cause of the issue financialtimes.slack.com/archives/C1WF81SR3/p1585756523025700?thread_ts=1585737775.022200&cid=C1WF81SR3, right?

This is not the root cause no, we still don't know the root cause but this will stop the events from making it all the way into Spoor

JakeChampion commented 4 years ago

@chrisbrownuk and @jaya-ananthram-ft, I have added some more code to ignore duplicate page-view events that only differ by the context.id and context.root_id properties 👍

origamiserviceuser commented 4 years ago

o-tracking bundle size difference from 2.0.10 to 08b3d496b4bc45193cff02e309af81ad640d00aa js: 5.4kb increase (1.64kb/gzip)

chrisbrownuk commented 4 years ago

@JakeChampion Great. Looks good.