Unify base and view #2

agnessnowplow · 2023-09-12T10:50:38Z

I think it would be worth to take a quick look at the current progress and get some feedback before I continue to make changes on sessions and users as the main logic of unification happens in base and views already so any changes I would have to correct further up as well.

I have added a dummy one line mobile data in the integration tests so that we can see if it actually runs as expected. The integration tests are currently only running to see if they compile and only until derived.views. For Snowflake/Bigquery/Databricks I checked that the mobile line is returned with the extra context fields but it needs further investigation as I progress, although the original target is to only support Snowflake with the first release. For Postgres/Redshift the base macro needed a bit of tweaking to avoid duplication coming from using the same session context for both the user and session identifiers (update: it works now).

What I want to agree on mainly is the logic of unification and which contexts to use. Upon discussion with Matus it turned out that we have started moving towards removing the atomic hardcoded fields, which will be a gradual progress, some users can already use other contexts (e.g. browser context) to replace certain fields etc. I think if we try and make the fields more 'conceptual' then when this happens we can just add a coalesce and keep the integrity. However, to avoid having a 150+ wide tables, the context fields are planned to only be optional, based on what the user selects which is where it may get tricky to get the right balance of unification. I also updated the lists of fields to extract to a more recent schema version, I think this needs double checking still.

Yml files, docs, intro etc are incomplete as of now, please ignore them (as well as optional modules).

To do (things to consider later on in a future PR, depending on time before the release or after):

adding the android referrer context
adding an optional model for cross platform tracking
making sure optional modules: cwv/consent work
porting over mobile optional module: app_errors (updating if needed)
replace engagement calculation for mobile (once the tracking side is finished within this quarter)
add proper integration test (a simplified version at that)
consider removing parts from yml /dbt tests
check if we need to remove the exclude fields macro or move it to utils
how does it handle anonymous tracking?

rlh1994

I think this is absolutely fantastic work, I've got a couple (21, lol) of comments which are mostly focused around setting up best possible for the future, or to decrease code duplication even further than you have.

Seriously, not having a model for every single warehouse is HUGE, and the 2-model approach to first standardising and then coalescing makes it really clean and clear where we manage the unification. Doing the field extraction and unification upfront also is so great to be able to pull a load of complexity and if blocks out from downstream, amazing!

dbt_project.yml

models/base/scratch/redshift_postgres/snowplow_unified_base_events_this_run.sql

macros/exclude_columns.sql

models/base/scratch/default/snowplow_unified_events_this_run.sql

models/views/scratch/default/snowplow_unified_views_this_run.sql

rlh1994 · 2023-09-12T15:21:02Z

models/views/scratch/default/snowplow_unified_views_this_run.sql

+    {% if var('snowplow__enable_mobile') %}
+      ev.session__previous_session_id,
+    {% endif %}
+
+    -- user id fields
+    ev.user_id,
+    ev.user_identifier,
+    ev.domain_userid,
+    {% if var('snowplow__enable_mobile') %}
+      ev.session__user_id as device_user_id,
+    {% endif %}


So I think it might be worth us having a discussion as a team about column ordering, as this could really help simplify the code and increase readability if we group all the web-only and mobile-only fields together. Probably have this sooner rather than later?

Okay I kept reading and saw you mostly already did this, but I still think it would be good to have a style guide around this and decide how strict on that we want to be

Also, if nothing else, you can always group them in the CTE and then do a final order at the end...

I have simplified the views by blockifying fields, I think it made it less error prone and more readable but you tell me.

models/views/scratch/snowplow_unified_pv_engaged_time.sql

rlh1994 · 2023-09-12T15:25:43Z

models/views/scratch/snowplow_unified_pv_engaged_time.sql

I think there were discussions of tracking active time directly in mobile at some point @matus-tomlein? I guess at that point we could just add it into here if it exists, use the calculation otherwise?

Yes, that's on our roadmap for this quarter to come up with a solution for engagement tracking on mobile. Sorry I don't fully understand the comment – is there any other calculation that we currently do on mobile? I can only see the one for Web based on page pings, just wondering if I'm missing something.

No that's fine, that's what I mean is that we don't currently have an approach for mobile, but once that's ready we could add that to this model and it would just slot in downstream

models/base/scratch/redshift_postgres/snowplow_unified_base_events_this_run.sql

models/base/scratch/redshift_postgres/snowplow_unified_events_this_run.sql

models/base/scratch/redshift_postgres/snowplow_unified_base_events_this_run.sql

rlh1994 · 2023-09-13T07:23:58Z

models/views/scratch/postgres/snowplow_unified_views_this_run.sql

+
+    row_number() over (partition by ev.view_id order by ev.derived_tstamp, ev.dvce_created_tstamp) as view_id_dedupe_index
+


I want other peoples opinions on this, but given there's literally 3 lines of difference between postgres and every other warehouse on a ~700 line file, I wonder if we should just wrap those lines in an if target.type... instead and only have 1 file, which would make updates or anything else much easier to manage?

I think there are many if blocks due to the various conditions so I don't think this would cause much difference when it comes to readability, if that is the concern. I have made the changes just so that we can actually see how it looks in real life and it needed 3 blocks of code like that.

My personal opinion is 3 blocks to remove duplicated 690 lines is a very worthwhile trade, but not sure what @emielver and @georgewoodhead think?

macros/unify_fields_query.sql

rlh1994 · 2023-09-13T07:33:07Z

models/views/scratch/default/snowplow_unified_views_this_run.sql

+    -- device fields
+    pve.app_id,
+    pve.platform,
+    pve.device_category,
+    {% if var('snowplow__enable_mobile_context') %}


I think we should bring these fields up more to the front, because now you are likely to want to filter web/mobile, where before they were (except maybe app_id) mostly the same...

I added them behind timestamp fields.

matus-tomlein

This is really exciting!

models/base/scratch/redshift_postgres/snowplow_unified_events_this_run.sql

custom_example/models/snowplow_web_custom_modules/users/snowplow_unified_users_custom.sql

...ules/views/page_view_channel_engagement/snowflake/snowplow_unified_pv_channel_engagement.sql

integration_tests/models/dummy_custom_module/snowplow_unified_pv_channels.sql

models/base/scratch/default/snowplow_unified_events_this_run.sql

matus-tomlein · 2023-09-13T08:05:16Z

models/base/scratch/redshift_postgres/snowplow_unified_events_this_run.sql

+    domain_userid,
+    domain_sessionidx,
+    network_userid,
+    geo_country,


Sorry for the nitpick, but would there be a way to distinguish the geo properties that are calculated based on the IP address (such as this one), from the ones based on the geolocation context (which are reported from the device)? They are quite different and there is a lot of confusion around that from users. Maybe we could use geo_ip_... and geo_dvce_... as the prefixes? This might be out of scope for this PR, so ignore me in that case.

It is a very valid point and I'm not sure how it was or wasn't distinguished in the scratch table for redshift/posttgres as I now unified them, but later on you will see that there is a separate --geo fields and -- mobile only fields with geo__ prefix (double underscore always indicates it comes from a context) perhaps I should change __geo to __geo_dvce so that the distinction is even more clear?

models/base/scratch/redshift_postgres/snowplow_unified_events_this_run.sql

macros/field_extractions/get_app_context_fields.sql

emielver · 2023-09-13T16:06:18Z

macros/unify_fields_query.sql

+          ev.domain_sessionidx,
+        {% endif %}
+        {% if var('snowplow__enable_mobile') %}
+          ev.session__session_index,


Should we flip the order of this around? I'm not sure about domain_sessionidx existing for mobile but if it does we need to take session__session_index, and session__session_index definitely doesn't exist for web so no harm done in swapping them around

It can exist on Web (it's an optional configuration now) and we plan to make it default in the v4 version of the tracker (probably). Still no harm done if the order is swapped because it has the same info as domain_sessionidx. But in the future we plan not to deprecate the domain_sessionidx and use the session context on Web too, so we would probably need to decouple the session context from the snowplow__enable_mobile flag (but that is a problem for the future).

Co-authored-by: Emiel Verkade <[email protected]>

agnessnowplow · 2023-09-14T14:38:23Z

Thanks for all the comments so far! I am now actively working on sessions (discovering more bugs etc) but keeping this PR open for a little while so that you can see the replies but will close/merge this soon.

Unify base and view

1b6a5df

agnessnowplow requested review from emielver, rlh1994, georgewoodhead and matus-tomlein September 12, 2023 10:57

Postgres changes

b0a7fdb

rlh1994 reviewed Sep 12, 2023

View reviewed changes

models/base/scratch/redshift_postgres/snowplow_unified_events_this_run.sql Outdated Show resolved Hide resolved

agnessnowplow added 7 commits September 12, 2023 17:41

Remove limit pv to session

95e6d1e

Remove enable_load_tstamp var

65e7f7c

Unify unified events this run

d316ff3

Make yauaa coalesce null

4607119

Rename web page context field macro

5fb587a

Rename page_views cluster by fields

83ac335

Add redshift to default views this run

4149042

rlh1994 reviewed Sep 13, 2023

View reviewed changes

models/base/scratch/redshift_postgres/snowplow_unified_base_events_this_run.sql Outdated Show resolved Hide resolved

rlh1994 reviewed Sep 13, 2023

View reviewed changes

macros/unify_fields_query.sql Show resolved Hide resolved

macros/unify_fields_query.sql Outdated Show resolved Hide resolved

rlh1994 reviewed Sep 13, 2023

View reviewed changes

agnessnowplow added 4 commits September 13, 2023 09:13

Remove weird renaming from redshift this run

e4a4576

Rename web in macro

0a104f4

Rename pageviewstitching

f9d66c4

Use set block for engaged time calc

932ea30

matus-tomlein reviewed Sep 13, 2023

View reviewed changes

agnessnowplow added 2 commits September 13, 2023 09:39

Rename cluster by part 2

dddf9c5

Unify base events this run

98d2365

rlh1994 reviewed Sep 13, 2023

View reviewed changes

macros/field_extractions/get_app_context_fields.sql Outdated Show resolved Hide resolved

agnessnowplow added 3 commits September 13, 2023 11:27

Unify views this run

cd73c29

Change device category logic

f78ce57

Rename name to schema

e688846

emielver reviewed Sep 13, 2023

View reviewed changes

emielver and others added 26 commits September 13, 2023 17:06

Indentation

1a6d99a

Update macros/field_extractions/get_app_context_fields.sql

83270a5

Co-authored-by: Emiel Verkade <[email protected]>

Update macros/field_extractions/get_iab_context_fields.sql

483788a

Co-authored-by: Emiel Verkade <[email protected]>

Update macros/field_extractions/get_deep_link_context_fields.sql

4bad3b5

Co-authored-by: Emiel Verkade <[email protected]>

Update macros/field_extractions/get_browser_context_fields.sql

b1aca43

Co-authored-by: Emiel Verkade <[email protected]>

Update macros/field_extractions/get_browser_context_fields.sql

d5e7349

Co-authored-by: Emiel Verkade <[email protected]>

Update macros/field_extractions/get_browser_context_fields.sql

e2456e0

Co-authored-by: Emiel Verkade <[email protected]>

Update macros/field_extractions/get_browser_context_fields.sql

e93d809

Co-authored-by: Emiel Verkade <[email protected]>

Update macros/field_extractions/get_browser_context_fields.sql

5ca4262

Co-authored-by: Emiel Verkade <[email protected]>

Update macros/field_extractions/get_app_error_context_fields.sql

962910a

Co-authored-by: Emiel Verkade <[email protected]>

Update macros/field_extractions/get_browser_context_fields.sql

0e61ed6

Co-authored-by: Emiel Verkade <[email protected]>

Update macros/field_extractions/get_browser_context_fields.sql

eed9a5a

Co-authored-by: Emiel Verkade <[email protected]>

remove comma

0238a59

Fix more indentation stuff

22409d6

Swap sessionindex around

0e25783

Unify domain userid under device identifier

aaead5a

Rename cte to views

6c6b2f8

Rename session_index and add missed device fields

3c8f071

Bug fixes

ccb028f

Point utils to release branch

c5a4ace

Simplify views for readability

9c39a70

Enable multi version support for bq contexts

94102e0

Bq fixes

c2de2be

Change nulls strings to max macro due to redshift

d3ad7d0

Add config to unified this run

0ce05ae

Change dummy to model_tstamp

ac931b3

agnessnowplow closed this Sep 18, 2023

emielver deleted the feature/unify_views branch December 1, 2023 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify base and view #2

Unify base and view #2

agnessnowplow commented Sep 12, 2023 •

edited

Loading

rlh1994 left a comment

rlh1994 Sep 12, 2023

rlh1994 Sep 12, 2023

rlh1994 Sep 12, 2023

agnessnowplow Sep 14, 2023

rlh1994 Sep 12, 2023

matus-tomlein Sep 13, 2023

rlh1994 Sep 13, 2023

rlh1994 Sep 13, 2023

agnessnowplow Sep 13, 2023

rlh1994 Sep 13, 2023

rlh1994 Sep 13, 2023

agnessnowplow Sep 13, 2023

matus-tomlein left a comment

matus-tomlein Sep 13, 2023

agnessnowplow Sep 13, 2023

emielver Sep 13, 2023

matus-tomlein Sep 14, 2023

agnessnowplow commented Sep 14, 2023


		row_number() over (partition by ev.view_id order by ev.derived_tstamp, ev.dvce_created_tstamp) as view_id_dedupe_index

Unify base and view #2

Unify base and view #2

Conversation

agnessnowplow commented Sep 12, 2023 • edited Loading

rlh1994 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matus-tomlein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnessnowplow commented Sep 14, 2023

agnessnowplow commented Sep 12, 2023 •

edited

Loading