-
Notifications
You must be signed in to change notification settings - Fork 113
Add/ga4 schemas #1387
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/ga4 schemas #1387
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schemas look good. My only comment is about monetary values.
A field with "type": "number"
will get loaded into the warehouse as a double. If you add "multipleOf": 0.01
then it gets loaded as a decimal, which might be more convenient for the end user working with the data. (Although I am not an expert in how analysts work with this data).
See for example the following schemas which specify a multipleOf
for monetary values:
- schemas/com.google.analytics.ecommerce/item/jsonschema/1-0-0
- schemas/com.google.analytics.ecommerce/transaction/jsonschema/1-0-0
- schemas/com.google.analytics.enhanced-ecommerce/actionFieldObject/jsonschema/1-0-0
- schemas/com.google.analytics.enhanced-ecommerce/impressionFieldObject/jsonschema/1-0-0
- schemas/com.google.analytics.enhanced-ecommerce/productFieldObject/jsonschema/1-0-0
- schemas/com.google.analytics.measurement-protocol/item/jsonschema/1-0-0
- schemas/com.google.analytics.measurement-protocol/product/jsonschema/1-0-0
- schemas/com.google.analytics.measurement-protocol/product_impression/jsonschema/1-0-0
- schemas/com.google.analytics.measurement-protocol/transaction/jsonschema/1-0-0
- schemas/com.snowplowanalytics.snowplow.ecommerce/cart/jsonschema/1-0-0
- schemas/com.snowplowanalytics.snowplow.ecommerce/product/jsonschema/1-0-0
- schemas/com.snowplowanalytics.snowplow.ecommerce/refund/jsonschema/1-0-0
- schemas/com.snowplowanalytics.snowplow.ecommerce/transaction/jsonschema/1-0-0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look good to me, can't find any discrepancies with the GA4 events.
@istreeter this makes sense, the other schemas are a little bit incorrect here (we need multipleOf 0.001) for consistency with three decimal place currencies. This will still create a decimal with the correct scale right? If so I'm happy to update. |
I confirm multipleOf 0.001 will still create a decimal field. So that sounds like a good solution. |
Thanks for confirming @istreeter. I've added @greg-el Do you mind giving this another once over and then we can hopefully get it merged? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The multipleOf
changes look good. LGTM assuming the changes are split out into the relevant commits.
…ema/1-0-0 (close #1381)
52bca49
to
ddf153f
Compare
I've splitted out mike's changes to the relevant commits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"properties": { | ||
"score": { | ||
"description": "The score to post.", | ||
"type": ["number", "null"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we okay to keep it nullable even if it's a required field?
(for ref: https://developers.google.com/analytics/devguides/collection/protocol/ga4/reference/events#post_score)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, null is fine here.
"maxLength": 500 | ||
} | ||
}, | ||
"required": ["virtual_currency_name", "value"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Is it ok to keep them nullable even if required?
Ref: https://developers.google.com/analytics/devguides/collection/protocol/ga4/reference/events#spend_virtual_currency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah no issue, this just means you can send null in here if you want to.
"maxLength": 500 | ||
} | ||
}, | ||
"required": ["achievement_id"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required but nullable. Is it ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR cherry-picks #1245 and splits adding the schemas into commits.
The events are documented here: https://developers.google.com/analytics/devguides/collection/ga4/reference/events?client_type=gtag