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

Add wip incremental model #1

Merged

Conversation

agnessnowplow
Copy link
Collaborator

Opening this PR for an early review to make sure the structure and incremental logic is agreed on before I go ahead and complete / revise it, add the integration tests and make the changes on docs. Currently it is only tested for Snowflake.

Core logic:

  • should work out of the box by using the upcoming unified package with the conversions as source and sessions used for path source (both can be changed, needs further testing if we want check examples but due to the field names I think they are pretty much "forced" to use it)
  • the paths_to_conversion incremental table collects the latest data in one go (starting from a user defined start date called snowplow_attribution_start_date) taking into account the last conversion event that was processed for incremental runs
  • users should be instructed not to change any variables, for safety so far only the incremental_manifest table exists, later we could add warnings to prevent such user errors
  • once we have the path data per conversion event we create two other downstream incremental tables the channel and campaign attributions, both of which apply all the different attribution algorithms apart from shapely (need to think about how to support it without python in a future release)
  • the report table is currently set as a user configured one with a sample output, this is to allow for custom spend data integration and other flexibility such as including the currently optional paths_to_non_conversion tables which for simplicity's sake is a drop and recompute table for now
  • I have left the path summary and channel counts tables for now but I'm not too sure they are needed for anything, we can agree with customers, we might add it as optional models if still useful, not sure

Copy link
Contributor

@rlh1994 rlh1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall structure and approach is good, nice to see how it all flows together. I haven't really had a chance to look at any actual data yet (partly an error with report table), but it all makes sense to go ahead with this approach.

I think we should have a working report table out the box, not including any spend info, but make it easyish to add that in by default. A few other bits here and there but SO much easier to understand and follow than the last one!

.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/workflows/build-and-push-docker-image.yml Outdated Show resolved Hide resolved
LICENSE Outdated Show resolved Hide resolved
packages.yml Outdated Show resolved Hide resolved
models/snowplow_attribution_channel_counts.sql Outdated Show resolved Hide resolved
macros/path_transformations/path_transformation.sql Outdated Show resolved Hide resolved
macros/path_transformations/transform_paths.sql Outdated Show resolved Hide resolved
models/snowplow_attribution_channel_attributions.sql Outdated Show resolved Hide resolved
models/snowplow_attribution_report_table.sql Outdated Show resolved Hide resolved
macros/report_table.sql Outdated Show resolved Hide resolved
macros/report_table.sql Outdated Show resolved Hide resolved
macros/conversion_clause.sql Outdated Show resolved Hide resolved
models/snowplow_attribution_overview.sql Outdated Show resolved Hide resolved
macros/overview.sql Outdated Show resolved Hide resolved
macros/overview.sql Outdated Show resolved Hide resolved
macros/overview.sql Outdated Show resolved Hide resolved
macros/overview.sql Outdated Show resolved Hide resolved
macros/overview.sql Outdated Show resolved Hide resolved
macros/paths_to_conversion.sql Show resolved Hide resolved
Copy link
Contributor

@rlh1994 rlh1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feedback :)

models/schema.yml Outdated Show resolved Hide resolved
macros/schema.yml Show resolved Hide resolved
macros/overview.sql Outdated Show resolved Hide resolved
models/snowplow_attribution_channel_attributions.sql Outdated Show resolved Hide resolved
models/snowplow_attribution_campaign_attributions.sql Outdated Show resolved Hide resolved
macros/overview.sql Outdated Show resolved Hide resolved
models/snowplow_attribution_channel_attributions.sql Outdated Show resolved Hide resolved
models/snowplow_attribution_campaign_attributions.sql Outdated Show resolved Hide resolved
models/snowplow_attribution_campaign_attributions.sql Outdated Show resolved Hide resolved
macros/paths_to_conversion.sql Outdated Show resolved Hide resolved
macros/overview.sql Outdated Show resolved Hide resolved
macros/overview.sql Outdated Show resolved Hide resolved
macros/overview.sql Outdated Show resolved Hide resolved
macros/paths_to_conversion.sql Outdated Show resolved Hide resolved
macros/paths_to_conversion.sql Show resolved Hide resolved
macros/paths_to_conversion.sql Outdated Show resolved Hide resolved
@agnessnowplow agnessnowplow marked this pull request as ready for review January 18, 2024 13:10
@agnessnowplow agnessnowplow force-pushed the feature/incremental_model branch 6 times, most recently from 86b1e1e to bd075c7 Compare January 30, 2024 16:34
Copy link
Contributor

@rlh1994 rlh1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, probably!

@agnessnowplow agnessnowplow force-pushed the feature/incremental_model branch from bd075c7 to c142932 Compare January 30, 2024 16:55
@agnessnowplow agnessnowplow merged commit ea26a14 into release/snowplow_attribution/0.1.0 Jan 31, 2024
3 checks passed
@agnessnowplow agnessnowplow deleted the feature/incremental_model branch January 31, 2024 10:05
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.

2 participants