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 dev.amp.snowplow/amp_session/jsonschema/1-0-0 (closes #1234) #1235

Closed
wants to merge 1 commit into from

Conversation

igneel64
Copy link
Contributor

Add the amp_session schema.

  • sessionCreationTimestamp and lastSessionEventTimestamp are sending values of 'milliseconds since Epoch' as you would get the result from calling in the browser new Date().getTime(). What would we set as the expected maximum of this value ? For now I have set the maximum as maximum of seconds since Epoch * 1000

See more at snowplow-incubator/amphtml#20

@igneel64 igneel64 requested a review from AlexBenny October 12, 2022 15:48
@snowplow snowplow deleted a comment from snowplowcla Oct 12, 2022
@igneel64 igneel64 requested a review from paulboocock October 12, 2022 15:48
@paulboocock
Copy link
Contributor

You could go for Max Safe Integer 9007199254740991 which is good for another 200 or so years.

@igneel64 igneel64 force-pushed the feature/1234-add_amp_session_schema branch from 510f1e4 to bf7da4d Compare October 17, 2022 06:59
@igneel64
Copy link
Contributor Author

Added max safe integer!

Copy link
Contributor

@paulboocock paulboocock left a comment

Choose a reason for hiding this comment

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

LGTM. This can be merged into a release/r137 branch (you might need to create it) and the typically Prepare for release commit should also be added (with changelog updates and such) e.g. 2ec9d57

Once thats done, then should be able to release this. I don't imagine other schemas will be ready particularly soon so might aswell get this out now, then we can do a r138 once the other new tracking ones are ready.

@colmsnowplow colmsnowplow self-requested a review October 27, 2022 13:44
Copy link
Contributor

@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.

LGTM!

@igneel64
Copy link
Contributor Author

igneel64 commented Nov 7, 2022

Merged at #1238

@igneel64 igneel64 closed this Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants