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

[snowplow-analytics] ✨ Add optional consent context #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

igneel64
Copy link

@igneel64 igneel64 commented Oct 4, 2022

Description

Add a new schema describing the AMP consent state and the values we can track.

These data can come from the usage of the capability for an integration with a CMP (Consent Management Platform). Based on the settings on the CMP and the user selections, the values in the fields shown in the schema are updated and are available for us to collect.

Fields will be described in the schema.

  • consentState
  • gdprApplies
  • consentType
  • consentOne

closes #17

Pending schema snowplow/iglu-central#1329

Copy link

@matus-tomlein matus-tomlein left a comment

Choose a reason for hiding this comment

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

LGTM (not knowing much about AMP)!

Just ran the schema through igluctl lint and it complains about 2 strings not having maximum length limit:

1. Following string properties provide no clues about maximum length (add --skip-checks stringLength to omit this check):
 - /properties/additionalConsentString	
 - /properties/consentString

Might be worth adding a limit just to avoid some huge events getting into the warehouse. Sometimes we use 4096 as max length but not sure what fits here.

Also there are no required fields in the schema – maybe that's intentional. On the other hand gdprApplies and purposeOne don't allow for null in their types. I think we should either make them required or add the null to the types. Otherwise we end up with a weird situation that they either are not present in the payloads or have a value but not null. But you'll probably get better feedback about this when you make the PR on Iglu Central.

Copy link
Collaborator

@colmsnowplow colmsnowplow left a comment

Choose a reason for hiding this comment

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

Assuming CONSENT_METADATA macro works as it looks like it works, LGTM!

@igneel64 igneel64 force-pushed the issue/17-add-consent-tracking branch from 70dc407 to adfdcbc Compare August 22, 2023 11:33
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.

Add support for tracking consent
3 participants