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

Issue #340 fill null event key #341

Merged
merged 7 commits into from
Aug 26, 2024

Conversation

EvavW
Copy link
Contributor

@EvavW EvavW commented Aug 22, 2024

Description & motivation

Follows discussion in Issue #340

Changes:

  • Use array_to_string rather than concat to create event_key
  • replace session_key with client_key and session_id in event_key creation
  • Add warning for users that this could result in duplicate event_keys in cases where distinction between events depends on one or both of those values being not null.

Rationale:

Generate event_key even when session_key is null (either because of null user_psuedo_id or null ga_session_id). This will improve the usability of event_key as a tool to count unique events.

Use both client_key and session_id in key creation (instead of session_key) so that a null value in either user_pseudo_id or ga_session_id will not nullify the other. This prevents duplicate event_keys. For example:

  • In a case where user_pseudo_id is available but ga_session_id is null. This results in a null session_key. If that session_key is used to create event_key for two events for which all other inputs to key generation are identical then there will be duplicates. Using both client_key and session_id in event_key generation prevents this.
  • Of course in the case that both user_pseudo_id and ga_session_id are null and all other inputs to key generation are identical there will still be duplicates.

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 run dbt test and python -m pytest . to validate existing tests

@EvavW EvavW marked this pull request as draft August 22, 2024 19:30
@EvavW EvavW marked this pull request as ready for review August 22, 2024 19:39
@EvavW EvavW marked this pull request as draft August 22, 2024 20:12
@EvavW EvavW marked this pull request as ready for review August 22, 2024 20:39
@adamribaudo-velir adamribaudo-velir self-requested a review August 23, 2024 11:32
Copy link
Collaborator

@adamribaudo-velir adamribaudo-velir left a comment

Choose a reason for hiding this comment

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

Looks good. Well-documented as well. Hope to see you contribute more!

@EvavW
Copy link
Contributor Author

EvavW commented Aug 23, 2024

Thanks for the review @adamribaudo-velir! Do you have an idea of when this can be merged and released?

@adamribaudo-velir adamribaudo-velir merged commit 1a606a5 into Velir:main Aug 26, 2024
2 checks passed
@adamribaudo-velir
Copy link
Collaborator

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

Successfully merging this pull request may close these issues.

2 participants