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.

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

Display unique campaign views instead of overall views #680

Closed
wants to merge 4 commits into from

Conversation

marcinkunert
Copy link
Contributor

@marcinkunert marcinkunert commented Jan 26, 2022

I haven't tested it on a huge database, but it should do the trick. As far as I know, no other changes should be required.

Closes #676

.gitattributes Outdated Show resolved Hide resolved
@knadh
Copy link
Owner

knadh commented Jan 27, 2022

I'll test this locally on a large DB soon and then merge.

@knadh
Copy link
Owner

knadh commented Jan 31, 2022

I tested this on a DB with tens of millions of rows and it's fast. So, DISTINCT is fine. Could you please amend this PR to:

  • Remove the .gitattributes changes (it's already merged in that other PR).
  • Add DISTINCT to the get-campaign-click-counts query.

However, there's a side-effect here. The distinct query is only meaningful when Settings -> Privacy -> Individual subscriber tracking is on. If that's turned off, then subscriber_id is null in the tables and DISTINCT will always return 1 :)

@NicoHood
Copy link
Contributor

I think we should merge this PR/add this feature.

You mentioned a good point about individual subscriber tracking. I am personally on a privacy friendly side. However what is the point, if the graphics are totally useless (they are) if no individual people are tracked. Maybe it would make sense to USE or NOT TO USE the tracking at all. Is that maybe a more user friendly experience idea?

@knadh
Copy link
Owner

knadh commented Jan 31, 2022

However what is the point, if the graphics are totally useless (they are) if no individual people are tracked.

I disagree. With individual tracking off, the PR renders a flat graph. There are thousands of actual views recorded (without individual subscriber information), including duplicate views, granted, but it is still valid information. To show 0 in such cases is semantically incorrect.

image

I think the better approach is to show unique views (like this PR does) when individual views are turned on, and non-unique views when they're off. This non-unique graph, despite including an unknown number of duplicates, still conveys some useful information on trends.

image

I'll hold off on this PR and push a patch that does unique/non-unique conditionally based on the privacy setting. It'll need the existing SQL queries to have string interpolation based on the privacy setting (there's no way to do a conditional DISTINCT directly) so requires a bit more work.

@NicoHood
Copy link
Contributor

But then the UI should also reflect this and should not show any percent, like it does at the moment.

@knadh
Copy link
Owner

knadh commented Jan 31, 2022

Yep, when subscriber tracking is off and the graph's non-unique, the % dials will not render.

@marcinkunert
Copy link
Contributor Author

@knadh

I'll hold off on this PR and push a patch that does unique/non-unique conditionally based on the privacy setting. It'll need the existing SQL queries to have string interpolation based on the privacy setting (there's no way to do a conditional DISTINCT directly) so requires a bit more work.

Maybe instead of creating a string interpolation query, just fetch both of these values. I'm not sure though how this affects the performance.

SELECT campaign_id, COUNT(DISTINCT subscriber_id) AS "count", COUNT(*) AS "total_count", DATE_TRUNC((SELECT * FROM intval), created_at) AS "timestamp"

After that (depending on tracking settings) one of these counts could be returned to REST endpoint.

@knadh knadh closed this in 2614b07 Feb 1, 2022
@knadh
Copy link
Owner

knadh commented Feb 2, 2022

@marcinkunert 2614b07 addresses this. The queries are string interpolated based on the privacy setting just once and are prepared and kept on app boot. Having 4 big queries and executing them in app logic would've been complicated.

@marcinkunert
Copy link
Contributor Author

@knadh sure, looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display unique campaign views instead of overall views
3 participants