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 option to use refer in channel classification if mkt is null #42

Merged

Conversation

rlh1994
Copy link
Contributor

@rlh1994 rlh1994 commented Apr 17, 2024

Description

FOR DISCUSSION

This PR adds a var to enable the use of refr fields in place of mkt fields when they are null in the default channel group query. This is off by default to make this a non-breaking change (and it is non-GA type behaviour. This has been mentioned a few times and done a few times by customers (see here).

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Checklist

  • ❗️ I have verified that these changes work on Redshift
  • 💣 Is your change a breaking change?
  • 📖 I have updated the CHANGELOG.md

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

Added to documentation?

  • 📓 internal package docs (ymls, macros, readme, if applicable)
  • 📕 I have raised a Snowplow documentation PR if applicable (Link here if required)
  • 🙅 no documentation needed

@rlh1994 rlh1994 requested a review from a team as a code owner April 17, 2024 15:55
@rlh1994 rlh1994 requested a review from miike April 17, 2024 15:55
@rlh1994 rlh1994 force-pushed the feature/add-refr-alt-to-mkt branch from 0e70f91 to c64c611 Compare April 17, 2024 16:37
@miike
Copy link
Contributor

miike commented Apr 18, 2024

I think we probably just need something in the docs to the effect that this only impacts medium and source (in terms of coalescing from referrer) and not other UTM fields (campaign, content etc). I'm not sure if people expect / want those fields coalesced as well.

@rlh1994
Copy link
Contributor Author

rlh1994 commented Apr 18, 2024

I think we probably just need something in the docs to the effect that this only impacts medium and source

Yeah we'll add details to the var for this and probably flesh out the docs on this more generally. From what I could tell refr doesn't have a campaign equivalent, and we don't use any other fields (e.g. content)

@rlh1994 rlh1994 force-pushed the feature/add-refr-alt-to-mkt branch from c64c611 to 9e621fb Compare May 9, 2024 09:26
@rlh1994 rlh1994 force-pushed the feature/add-refr-alt-to-mkt branch from 9e621fb to d0dde97 Compare May 9, 2024 09:28
Copy link
Collaborator

@agnessnowplow agnessnowplow left a comment

Choose a reason for hiding this comment

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

LGTM apart from the change log but that is a release pr topic anyway.

@rlh1994 rlh1994 merged commit 99d801f into release/snowplow-unified/0.4.1 May 9, 2024
4 checks passed
@rlh1994 rlh1994 deleted the feature/add-refr-alt-to-mkt branch May 9, 2024 14:17
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.

3 participants