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.

Sign up for GitHub

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

Jump to bottom

Derived session properties #54

Closed
willbryant opened this issue Aug 24, 2022 · 2 comments · Fixed by #58
Closed

Derived session properties #54

willbryant opened this issue Aug 24, 2022 · 2 comments · Fixed by #58

Comments

@willbryant
Copy link
Contributor

willbryant commented Aug 24, 2022 •
edited
Loading

derived_user_properties is excellent and exactly what I needed for some of my properties that once set don't really change.

But we also have some event params that will most likely be different for each session, and putting them in derived_user_properties means we overwrite the old values for old sessions, which is not right for our data. For example, our shipping time estimates are per-session.

So I'd like to add derived_session_properties.

Do you think it's worth waiting for #46 to go in before working on this? That looks pretty major so some risk of a merge conflict, but also looks a bit stalled?
@dgitis
Copy link
Collaborator

dgitis commented Aug 24, 2022

Hi @willbryant, I'm responsible for #46. It makes some big changes that we should discuss. I've been very busy lately with a lot of things but I will start to have time to push this to where it can be merged next week. I would expect a timeline of about 2-3 weeks before we can merge it.

Alternately, you can create a new branch from #46 instead of main and build off of that and then be sure to merge after we've resolved #46.

willbryant added a commit to willbryant/dbt-ga4 that referenced this issue Aug 24, 2022
@willbryant
Support derived_session_properties (Velir#54)
d10a823 
stg_ga4__derived_session_properties.sql is simply stg_ga4__derived_user_properties.sql with:
* derived_user_properties replaced by derived_session_properties
* user_property_name replaced by session_property_name
* up replaced by sp
@willbryant
Copy link
Contributor Author

Righto, thanks @dgitis.

Since #46 is a few weeks off, I took a closer look and realised that you weren't changing the derived user properties staging model, and it turns out I don't actually need to change any conflicting lines in the session tables, so I believe my PR will merge OK with yours. I'll submit it now.

@willbryant willbryant mentioned this issue Aug 24, 2022
Merged
4 tasks
adamribaudo-velir pushed a commit that referenced this issue Aug 26, 2022
@willbryant
Support derived_session_properties (#54) (#58)
75b09a9 
stg_ga4__derived_session_properties.sql is simply stg_ga4__derived_user_properties.sql with:
* derived_user_properties replaced by derived_session_properties
* user_property_name replaced by session_property_name
* up replaced by sp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support derived_session_properties (#54) willbryant/dbt-ga4
2 participants
@willbryant @dgitis