Replace snowplow_web with a base that can be compatible with mobile events (close #45) #44

matus-tomlein · 2023-07-05T12:19:34Z

Issure #45

Description & motivation

This PR takes out the snowplow_web package as the base and uses a custom one taken from the ecommerce package. The motivation for this is to be able to process mobile events as well.

We do not yet support mobile events in this PR. We can only generate mobile events from the new media schemas which are not yet supported by the package, so I will add support for mobile later, when adding support for the new media schemas. I just wanted to prepare for this in advance.

Along the way, I refactored the base_events_this_run models to reuse some logic across warehouses using macros.

Checklist

I have verified that these changes work locally
I have updated the README.md (if applicable)
I have added tests & descriptions to my models (and macros if applicable)
I have raised a documentation PR if applicable (Link here if required)

rlh1994

Just skimmed a few things for normal gotchas, can you also make sure to update the changelog as part of this PR please?

dbt_project.yml

docs/markdown/snowplow_media_player_base_docs.md

docs/markdown/snowplow_media_player_common_cols.md

models/base/manifest/snowplow_media_player_base_quarantined_sessions.sql

models/base/manifest/snowplow_media_player_base_sessions_lifecycle_manifest.sql

models/base/scratch/bigquery/snowplow_media_player_base_events_this_run.sql

rlh1994 · 2023-07-05T12:41:56Z

integration_tests/data/expected/redshift/snowplow_media_player_media_stats_expected.csv

@@ -1,4 +1,4 @@
 "media_id","media_label","duration","media_type","media_player_type","play_time_min","avg_play_time_min","first_play","last_play","plays","valid_plays","complete_plays","impressions","avg_playback_rate","play_rate","completion_rate_by_plays","avg_percent_played","avg_retention_rate","last_base_tstamp","_10_percent_reached","_25_percent_reached","_50_percent_reached","_75_percent_reached","_100_percent_reached"
 html-dbt,dbt Coalesce 2021 - Data modeling at Scale,1887.0,video,org.whatwg-media_element,39.333333333333336,9.833333333333332,2022-01-18 11:56:27.815,2022-01-18 12:11:12.690,4,2,1,4,1.0,1.0,0.25,0.3126656067832538,0.25,2022-01-20 19:13:21.293,1,1,1,1,2
 yt-dbt-coalesce-2021,dbt Coalesce 2021 - Data modeling at Scale,1887.0,video,com.youtube-youtube,55.06666666666667,1.4882882882882882,2022-01-18 21:23:57.381,2022-01-20 19:13:21.293,37,5,0,39,1.0270270270270272,0.9487179487179487,0.0,0.04732236210773571,0.002702702702702702,2022-01-20 19:13:21.293,2,2,1,0,4
-yt-dbt-coalesce-2022,dbt Coalesce 2022 - Data modeling at Scale 2,1889.0,video,com.youtube-youtube,,,2999-01-01 00:00:00.000,2000-01-01 00:00:00.000,0,0,0,1,2.0,,,,,2022-01-20 19:17:13.125,0,0,0,0,0
+yt-dbt-coalesce-2022,dbt Coalesce 2022 - Data modeling at Scale 2,1889.0,video,com.youtube-youtube,,,,,0,0,0,1,2.0,,,,,2022-01-20 19:17:13.125,0,0,0,0,0


Why has the expected value changed here? If this is a replacement for the existing method should it not result in the same output?

This is a bit of a mystery to me, I don't understand why the expected output was different for the different warehouses. Now all the warehouses have the same expected data so I removed the distinction between expected outputs by warehouse.

It must have been a side effect of using the snowplow_web as the base before compare to the custom base now. But I don't know what specifically could have caused it.

The different output is usually down to just slight differences between the processing approaches by warehouse (particularly when it comes to rounding or formatting). The loss of these values feels like something we should dive into and understand though, I suspect they are default values from somewhere (either a min/max default based on the warehouse, or we provided some defaults).

I'd be wary to release with these removed as many people may be filtering on these already and rely on them.

We have solved this mystery on Slack, it was a problem with the least and greatest functions giving different output (null or the max/min value) depending on whether null was present in the incremental run. I have added a nullif around the least and greatest functions to always return null in case there are no matched events.

rlh1994

Looks really good, mostly just a few nitpicky things, and some possible options to simplify/unify some stuff. Let me know if anything I've said is unclear because it might only make sense to me!

docs/markdown/snowplow_media_player_model_docs.md

macros/allow_refresh.sql

macros/snowplow_media_player_base_events_this_run/playback_quality_col.sql

macros/snowplow_media_player_base_events_this_run/media_id_col.sql

models/base/scratch/snowplow_media_player_base_new_event_limits.sql

models/base/scratch/snowflake/snowplow_media_player_base_events_this_run.sql

models/base/scratch/default/snowplow_media_player_base_events_this_run.sql

models/base/scratch/bigquery/snowplow_media_player_base_events_this_run.sql

models/base/scratch/databricks/snowplow_media_player_base_events_this_run.sql

macros/snowplow_media_player_base_events_this_run/source_url_col.sql

rlh1994

Pretty much there!

dbt_project.yml

models/media_stats/snowplow_media_player_media_stats.sql

dbt_project.yml

matus-tomlein · 2023-07-13T12:22:40Z

Thanks @rlh1994, these were super useful comments!

rlh1994

LGTM!

rlh1994 · 2023-07-18T10:41:20Z

Just make sure to squash all the commit please

…vents (close #45) PR #44

rlh1994 · 2023-08-18T14:53:13Z

Should be good to merge this @matus-tomlein

…vents (close #45) PR #44

matus-tomlein force-pushed the issue/change_base branch from dce41d8 to 5641277 Compare July 5, 2023 12:35

rlh1994 suggested changes Jul 5, 2023

View reviewed changes

rlh1994 changed the base branch from main to release/snowplow-media-player/0.6.0 July 5, 2023 12:38

rlh1994 reviewed Jul 5, 2023

View reviewed changes

matus-tomlein changed the title ~~Replace snowplow_web with a base that is compatible with mobile events [WIP]~~ Replace snowplow_web with a base that can be compatible with mobile events Jul 6, 2023

matus-tomlein marked this pull request as ready for review July 6, 2023 07:37

matus-tomlein requested a review from georgewoodhead as a code owner July 6, 2023 07:37

matus-tomlein requested review from rlh1994 and agnessnowplow July 6, 2023 07:37

matus-tomlein changed the title ~~Replace snowplow_web with a base that can be compatible with mobile events~~ Replace snowplow_web with a base that can be compatible with mobile events (close #45) Jul 6, 2023

rlh1994 suggested changes Jul 7, 2023

View reviewed changes

rlh1994 reviewed Jul 13, 2023

View reviewed changes

macros/snowplow_media_player_base_events_this_run/source_url_col.sql Outdated Show resolved Hide resolved

rlh1994 reviewed Jul 13, 2023

View reviewed changes

dbt_project.yml Outdated Show resolved Hide resolved

models/media_stats/snowplow_media_player_media_stats.sql Outdated Show resolved Hide resolved

dbt_project.yml Outdated Show resolved Hide resolved

matus-tomlein requested a review from rlh1994 July 13, 2023 12:22

rlh1994 approved these changes Jul 18, 2023

View reviewed changes

matus-tomlein added a commit that referenced this pull request Aug 16, 2023

Replace snowplow_web with a base that can be compatible with mobile e…

c35de0a

…vents (close #45) PR #44

matus-tomlein force-pushed the issue/change_base branch from c9a08c7 to c35de0a Compare August 16, 2023 09:02

Replace snowplow_web with a base that can be compatible with mobile e…

c854310

…vents (close #45) PR #44

matus-tomlein force-pushed the issue/change_base branch from c35de0a to c854310 Compare August 18, 2023 15:10

matus-tomlein merged commit 6865627 into release/snowplow-media-player/0.6.0 Aug 18, 2023
4 checks passed

matus-tomlein deleted the issue/change_base branch August 18, 2023 16:27

matus-tomlein added a commit that referenced this pull request Sep 20, 2023

Replace snowplow_web with a base that can be compatible with mobile e…

15fe474

…vents (close #45) PR #44

matus-tomlein added a commit that referenced this pull request Sep 20, 2023

Replace snowplow_web with a base that can be compatible with mobile e…

3f9b91c

…vents (close #45) PR #44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace snowplow_web with a base that can be compatible with mobile events (close #45) #44

Replace snowplow_web with a base that can be compatible with mobile events (close #45) #44

matus-tomlein commented Jul 5, 2023 •

edited

Loading

rlh1994 left a comment

rlh1994 Jul 5, 2023

matus-tomlein Jul 6, 2023

rlh1994 Jul 6, 2023

matus-tomlein Jul 13, 2023

rlh1994 left a comment

rlh1994 left a comment

matus-tomlein commented Jul 13, 2023

rlh1994 left a comment

rlh1994 commented Jul 18, 2023

rlh1994 commented Aug 18, 2023

Replace snowplow_web with a base that can be compatible with mobile events (close #45) #44

Replace snowplow_web with a base that can be compatible with mobile events (close #45) #44

Conversation

matus-tomlein commented Jul 5, 2023 • edited Loading

Description & motivation

Checklist

rlh1994 left a comment

Choose a reason for hiding this comment

rlh1994 Jul 5, 2023

Choose a reason for hiding this comment

matus-tomlein Jul 6, 2023

Choose a reason for hiding this comment

rlh1994 Jul 6, 2023

Choose a reason for hiding this comment

matus-tomlein Jul 13, 2023

Choose a reason for hiding this comment

rlh1994 left a comment

Choose a reason for hiding this comment

rlh1994 left a comment

Choose a reason for hiding this comment

matus-tomlein commented Jul 13, 2023

rlh1994 left a comment

Choose a reason for hiding this comment

rlh1994 commented Jul 18, 2023

rlh1994 commented Aug 18, 2023

matus-tomlein commented Jul 5, 2023 •

edited

Loading