fix(protocol): Properly document which timestamps are accepted #1152

untitaker · 2021-12-09T17:24:00Z

Also remove use of a FromStr impl where chrono's documentation says
nothing about the format that is being parsed.

It is currently unclear to me whether the timestamp format we accept in prod is even all of RFC 3339.

Also remove use of a `FromStr` impl where chrono's documentation says nothing about the format that is being parsed.

jjbayer · 2021-12-10T07:10:01Z

relay-general/src/protocol/types.rs

@@ -976,9 +976,9 @@ impl FromValue for Timestamp {
    fn from_value(value: Annotated<Value>) -> Annotated<Self> {
        let rv = match value {
            Annotated(Some(Value::String(value)), mut meta) => {
-                let parsed = match value.parse::<NaiveDateTime>() {
+                let parsed = match NaiveDateTime::parse_from_str(&value, "%Y-%m-%dT%H:%M:%S%.f") {


Suggested change

let parsed = match NaiveDateTime::parse_from_str(&value, "%Y-%m-%dT%H:%M:%S%.f") {

// Use the same format as `NaiveDateTime::FromStr`: https://docs.rs/chrono/0.4.19/chrono/naive/struct.NaiveDateTime.html#impl-FromStr

let parsed = match NaiveDateTime::parse_from_str(&value, "%Y-%m-%dT%H:%M:%S%.f") {

This is lenient against spaces, but it's not obvious from the documentation or the format string. Can you please add a test case for something like "2015- 09-18T23:56:04"?

@jan-auer is this still relevant? are you saying FromStr is lenient as well?

jjbayer · 2021-12-10T07:17:02Z

relay-general/src/protocol/types.rs

                    Ok(dt) => Ok(DateTime::from_utc(dt, Utc)),
-                    Err(_) => value.parse(),
+                    Err(_) => DateTime::parse_from_rfc3339(&value).map(|dt| dt.with_timezone(&Utc)),


Is this a stricter parser, or is it exactly the same?

It's a more lenient parser afaict, but chrono literally does not document what FromStr actually accepts. So, not sure. Will sit on this PR for a bit.

Co-authored-by: Joris Bayer <joris.bayer@sentry.io>

jan-auer

I'm +1 on the documentation changes.

However, I'm conflicted whether the explicit date time format is better than using the built-in FromStr, which is both documented and doesn't require parsing a format string over and over again. I would suggest that if we're touching the implementation, we should rather look into parsing full ISO 8601 in addition to RFC 3339 and then add sufficient tests.

jan-auer · 2021-12-14T06:59:04Z

relay-general/src/protocol/types.rs

                    Ok(dt) => Ok(DateTime::from_utc(dt, Utc)),
+
+                    // XXX: This actually accepts more than RFC 3339. SDKs are strongly discouraged
+                    // from exercising that freedom. We should only support RFC3339.


The implementation suggestion for SDKs is in a very hidden spot. If we keep current behavior, I would suggest removing the comment here and moving it to a doc comment or develop docs.

The "real" docs already tell the SDKs to use RFC 3339, this code comment is intended to explain why the actual relay code accepts more.

jan-auer · 2021-12-14T07:04:00Z

relay-general/src/protocol/types.rs

@@ -976,9 +976,9 @@ impl FromValue for Timestamp {
    fn from_value(value: Annotated<Value>) -> Annotated<Self> {
        let rv = match value {
            Annotated(Some(Value::String(value)), mut meta) => {
-                let parsed = match value.parse::<NaiveDateTime>() {
+                let parsed = match NaiveDateTime::parse_from_str(&value, "%Y-%m-%dT%H:%M:%S%.f") {


This is lenient against spaces, but it's not obvious from the documentation or the format string. Can you please add a test case for something like "2015- 09-18T23:56:04"?

jan-auer · 2021-12-14T07:04:34Z

relay-general/src/protocol/event.rs

@@ -304,14 +304,23 @@ pub struct Event {
    /// value representing the number of seconds that have elapsed since the [Unix
    /// epoch](https://en.wikipedia.org/wiki/Unix_time).
    ///
+    /// Timezone is assumed to be UTC if unclear.


Suggested change

/// Timezone is assumed to be UTC if unclear.

/// Timezone is assumed to be UTC if missing.

jan-auer · 2021-12-14T07:05:11Z

relay-general/src/protocol/event.rs

    /// Sub-microsecond precision is not preserved with numeric values due to precision
    /// limitations with floats (at least in our systems). With that caveat in mind, just send
    /// whatever is easiest to produce.
    ///
-    /// All timestamps in the event protocol are formatted this way.
+    /// All timestamps in the event protocol, be it errors, transactions or the spans within a


I would revert this sentence, it reads strange and doesn't add any insight.

untitaker · 2021-12-14T14:53:45Z

@jan-auer I have reverted most changes and left it at adding code comments

fix(protocol): Properly document what timestamps are accepted

30e8c19

Also remove use of a `FromStr` impl where chrono's documentation says nothing about the format that is being parsed.

untitaker requested a review from a team December 9, 2021 17:24

untitaker changed the title ~~fix(protocol): Properly document what timestamps are accepted~~ fix(protocol): Properly document which timestamps are accepted Dec 9, 2021

untitaker force-pushed the fix/timestamps branch from b10f51a to 30e8c19 Compare December 9, 2021 18:38

jjbayer reviewed Dec 10, 2021

View reviewed changes

untitaker and others added 4 commits December 10, 2021 12:58

Update relay-general/src/protocol/types.rs

2026ab8

Co-authored-by: Joris Bayer <joris.bayer@sentry.io>

update snapshot

1edecd6

changelog

f379f52

revert impl

1c24b30

jjbayer approved these changes Dec 13, 2021

View reviewed changes

jan-auer requested changes Dec 14, 2021

View reviewed changes

apply review comments

91a9891

update snapshot

74ee542

jan-auer approved these changes Dec 15, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into fix/timestamps

3eb8ec7

untitaker merged commit 429918a into master Dec 15, 2021

untitaker deleted the fix/timestamps branch December 15, 2021 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(protocol): Properly document which timestamps are accepted #1152

fix(protocol): Properly document which timestamps are accepted #1152

untitaker commented Dec 9, 2021 •

edited

Loading

jjbayer Dec 10, 2021

jan-auer Dec 14, 2021

untitaker Dec 14, 2021

jjbayer Dec 10, 2021

untitaker Dec 10, 2021

jan-auer left a comment

jan-auer Dec 14, 2021 •

edited

Loading

untitaker Dec 14, 2021

jan-auer Dec 14, 2021

jan-auer Dec 14, 2021

jan-auer Dec 14, 2021

untitaker commented Dec 14, 2021

	let parsed = match NaiveDateTime::parse_from_str(&value, "%Y-%m-%dT%H:%M:%S%.f") {
	// Use the same format as `NaiveDateTime::FromStr`: https://docs.rs/chrono/0.4.19/chrono/naive/struct.NaiveDateTime.html#impl-FromStr
	let parsed = match NaiveDateTime::parse_from_str(&value, "%Y-%m-%dT%H:%M:%S%.f") {

	/// Timezone is assumed to be UTC if unclear.
	/// Timezone is assumed to be UTC if missing.

fix(protocol): Properly document which timestamps are accepted #1152

fix(protocol): Properly document which timestamps are accepted #1152

Conversation

untitaker commented Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-auer left a comment

Choose a reason for hiding this comment

jan-auer Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Dec 14, 2021

untitaker commented Dec 9, 2021 •

edited

Loading

jan-auer Dec 14, 2021 •

edited

Loading