Replace time.Time.String() with time.Time.Format() #1017

MaryamTaj · 2024-02-17T16:30:34Z

Resolves #987
Earlier, we used time.Time.String() to marshal time into a string. According to the go time.Time documentation however, time.Time.String() is only suitable for debugging purposes. This PR replaces time.Time.String with time.Time.Format() with RFC3339 to provide a stable serialized representation of time.

embano1 · 2024-02-18T06:54:13Z

.github/workflows/draft-release.yaml

@@ -6,6 +6,7 @@ on:
    - 'protocol/**' # Push events to matching protocol/foo/bar/v*, i.e. protocol/foo/bar/v1.0, protocol/foo/bar/v20.15.10
    - 'observability/**' # Push events to matching observability/foo/bar/v*
    - 'sql/**' # Push events to matching sql/v*
+    - 'binding/**' # Push events to matching binding/foo/bar/v*


nit: should be separate PR

embano1 · 2024-02-18T06:54:20Z

README.md

@@ -27,7 +27,7 @@ _Note:_ Supported go version: 1.14+
 Add the module as dependency using go mod:

 ```shell
-go get github.com/cloudevents/sdk-go/v2@v2.4.1
+go get github.com/cloudevents/sdk-go/v2@v2.5.0


nit: should be separate PR (incl. the other ones)

embano1 · 2024-02-18T06:54:52Z

binding/format/protobuf/v2/protobuf.go

+/*
+ Copyright 2021 The CloudEvents Authors
+ SPDX-License-Identifier: Apache-2.0
+*/
+


nit: should be separate PR

embano1 · 2024-02-18T06:58:19Z

v2/event/event_marshal.go

-			stream.WriteString(eventContext.Time.String())
+			stream.WriteString(eventContext.Time.Format(time.RFC3339))


I ran an example w/ Go 1.22 (same w/ Go 1.21) here: https://go.dev/play/p/NcMTqPu75fe

And I get:

2009-11-10 23:00:00 +0000 UTC m=+0.000000001 2009-11-10T23:00:00Z

Which could be considered a breaking change. WDYT?

cc/ @duglin

I'm ok with the breaking change. I'm more bothered that:
1 - it wasn't spec compliant before and no one noticed
2 - not a single testcase needed to be changed

I agree with splitting the upgrade stuff into a separate PR. But can we also get at least one testcase added that checks the format of the time attribute and that it adheres to the CE spec (RFC3339) ?

I'll get to work on it! Thank you so much for the feedback!

Cleaned up the PR. Currently working on test cases:)

embano1 · 2024-03-22T08:03:31Z

@MaryamTaj converted to draft, seems you're still working on this. Once ready for review, please just mark as "Ready for review" and ping us.

duglin · 2024-07-16T12:11:18Z

@MaryamTaj any update on this?

MaryamTaj · 2024-07-16T13:25:47Z

@duglin Hi Doug! I was running into some issues earlier, where all the tests pass locally but not on GitHub. I am going to try a new approach, and I'll let you know how it goes

duglin · 2024-07-16T13:30:39Z

ok thanks for the update.

duglin · 2024-09-26T13:47:21Z

@MaryamTaj any update on this?

MaryamTaj · 2024-09-29T21:52:36Z

@duglin I think I'm running into a wall. I tried to resolve as many errors as I could.
Below, I have outlined the errors I am still running into, and what I have tried so far. This way, anyone else opening this PR does not have to start from scratch. I hope that will help, and I am genuinely sorry for not being able to resolve the issue!
https://docs.google.com/document/d/1xfac9mAtsvB8RvqKN-Fz4F96W-VfdtivWGZitTi_QZg/edit?usp=sharing

duglin · 2024-10-23T16:49:00Z

@MaryamTaj I push a commit - see what you think.
Also, I think you need to sign your commit(s). Might want to squash 'em.
Did you add a testcase that checks the actual syntax of the "time" field?

duglin · 2024-10-31T12:23:30Z

ping @MaryamTaj

MaryamTaj · 2024-10-31T13:21:39Z

@duglin Thank you so much for helping me out! I'm going through, and understanding how you resolved the issue. I will sign my commits to pass the DCO.

embano1 · 2024-10-31T13:36:21Z

Glad it's sorted out! Before we merge, can you please squash/clean up commits into one?

embano1 · 2024-10-31T13:37:49Z

test/integration/http/direct_v1_test.go

@@ -13,7 +13,7 @@ import (
 	"time"

 	cloudevents "github.com/cloudevents/sdk-go/v2"
-	"github.com/cloudevents/sdk-go/v2/types"
+	// "github.com/cloudevents/sdk-go/v2/types"


why comment out and not remove?

embano1 · 2024-10-31T13:39:27Z

@duglin do we consider this PR a breaking change for users? I understand our code was broken, so this would be a fix - but still could be an issue impacting downstream users?

MaryamTaj · 2024-10-31T15:07:09Z

Glad it's sorted out! Before we merge, can you please squash/clean up commits into one?

duglin · 2024-10-31T15:28:42Z

Yes it is a breaking change. Some options:
1 - merge it and see what happens :-)
2 - add a config option to allow people to turn it on if they wish, but no one will do so, or even know about it
3 - add a config option to get the current (wrong) behavior and only those who are broken will need to turn it on - if they don't want to be spec compliant going forward, but at least they have an immediate "fix" until they can "do the right thing"

I like 3.

Also, I think we need a test that checks the serialization of the timestamp IS actually the right format, not just that the sent and received values look the same. Unless I'm missing a test that already covers this.

Signed-off-by: MaryamTaj <[email protected]>

Signed-off-by: Doug Davis <[email protected]>

MaryamTaj · 2024-10-31T17:37:30Z

I signed off and squashed all the commits into one. I also went through the codebase. Would v2/types/timestamp_test.go check the serialization of the timestamp is actually the right format?

duglin · 2024-10-31T18:22:49Z

I signed off and squashed all the commits into one. I also went through the codebase. Would v2/types/timestamp_test.go check the serialization of the timestamp is actually the right format?

I'm guessing no since you didn't need to change any test in there as part of this PR. At least some test should have failed as a result of your change... hence I think we need a new test that would pass w/o your change in v2/event/event_marshal.go but would fail with it.... then you can "fix" the test to fail w/o your change, and pass with it.

embano1 · 2024-11-01T15:09:04Z

Yes, the existing tests might not detect it because if the producer is updated with the new semantics, the consumer will see automatically the new behavior. A test showing a producer with the old semantics and a consumer with the new behavior should fail with this change IIUC.

I also like option (3), but strictly speaking it requires a MAJOR version change in our semver scheme which doesn't feel justified. And my concern is that at the scale this SDK is used, consumers might not detect it right away?

duglin · 2024-11-04T15:00:41Z

I also like option (3), but strictly speaking it requires a MAJOR version change in our semver scheme which doesn't feel justified. And my concern is that at the scale this SDK is used, consumers might not detect it right away?

I agree that a major version bump doesn't feel right, especially if we want to consider this a "fix" rather than a new feature. One option is to do 2 and 3, meaning:

add a flag to control which timestamp format to use, but keep the default as today's format (option 2)
give lots of warnings in the docs, READMEs... about the issue and that the default will change
create a new release
then switch the default value of the flag in the next release (option 3)

At least then people had warnings and could migrate to the new format (turn on the flag) in prep for the final step at their own pace.

embano1 · 2024-11-04T16:20:34Z

v2/event/event_marshal.go

@@ -120,7 +121,7 @@ func WriteJson(in *Event, writer io.Writer) error {
 		if eventContext.Time != nil {
 			stream.WriteMore()
 			stream.WriteObjectField("time")
-			stream.WriteString(eventContext.Time.String())
+			stream.WriteString(eventContext.Time.Format(time.RFC3339Nano))


Why are we using Nano here, and the tests don't?

If I'm remembering correctly, it was because there were some tests that needed more precision than just "seconds".

embano1 reviewed Feb 18, 2024

View reviewed changes

MaryamTaj force-pushed the issue-987 branch 2 times, most recently from a209c30 to 232dcc4 Compare February 24, 2024 23:56

MaryamTaj force-pushed the issue-987 branch from 2ba4570 to d0a560f Compare March 21, 2024 13:26

embano1 marked this pull request as draft March 22, 2024 08:02

duglin force-pushed the issue-987 branch from 8f1eb8f to b39d268 Compare October 23, 2024 15:43

duglin force-pushed the issue-987 branch from fcc4f16 to d7b6aff Compare October 31, 2024 12:23

embano1 reviewed Oct 31, 2024

View reviewed changes

MaryamTaj closed this Oct 31, 2024

MaryamTaj reopened this Oct 31, 2024

MaryamTaj and others added 2 commits October 31, 2024 13:05

BREAKING CHANGE:Serializing time to RFC-3339

16b701e

Signed-off-by: MaryamTaj <[email protected]>

don't truncate to seconds

6d24612

Signed-off-by: Doug Davis <[email protected]>

MaryamTaj force-pushed the issue-987 branch from d7b6aff to 6d24612 Compare October 31, 2024 17:15

embano1 reviewed Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace time.Time.String() with time.Time.Format() #1017

Replace time.Time.String() with time.Time.Format() #1017

MaryamTaj commented Feb 17, 2024

embano1 Feb 18, 2024

embano1 Feb 18, 2024

embano1 Feb 18, 2024

embano1 Feb 18, 2024

duglin Feb 20, 2024

MaryamTaj Feb 20, 2024

MaryamTaj Feb 24, 2024 •

edited

Loading

embano1 commented Mar 22, 2024

duglin commented Jul 16, 2024

MaryamTaj commented Jul 16, 2024 •

edited

Loading

duglin commented Jul 16, 2024

duglin commented Sep 26, 2024

MaryamTaj commented Sep 29, 2024 •

edited

Loading

duglin commented Oct 23, 2024

duglin commented Oct 31, 2024

MaryamTaj commented Oct 31, 2024

embano1 commented Oct 31, 2024

embano1 Oct 31, 2024

embano1 commented Oct 31, 2024

MaryamTaj commented Oct 31, 2024

duglin commented Oct 31, 2024

MaryamTaj commented Oct 31, 2024

duglin commented Oct 31, 2024

embano1 commented Nov 1, 2024

duglin commented Nov 4, 2024

embano1 Nov 4, 2024

duglin Nov 4, 2024

		stream.WriteString(eventContext.Time.String())
		stream.WriteString(eventContext.Time.Format(time.RFC3339))

Replace time.Time.String() with time.Time.Format() #1017

Are you sure you want to change the base?

Replace time.Time.String() with time.Time.Format() #1017

Conversation

MaryamTaj commented Feb 17, 2024

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

Choose a reason for hiding this comment

MaryamTaj Feb 24, 2024 • edited Loading

Choose a reason for hiding this comment

embano1 commented Mar 22, 2024

duglin commented Jul 16, 2024

MaryamTaj commented Jul 16, 2024 • edited Loading

duglin commented Jul 16, 2024

duglin commented Sep 26, 2024

MaryamTaj commented Sep 29, 2024 • edited Loading

duglin commented Oct 23, 2024

duglin commented Oct 31, 2024

MaryamTaj commented Oct 31, 2024

embano1 commented Oct 31, 2024

Choose a reason for hiding this comment

embano1 commented Oct 31, 2024

MaryamTaj commented Oct 31, 2024

duglin commented Oct 31, 2024

MaryamTaj commented Oct 31, 2024

duglin commented Oct 31, 2024

embano1 commented Nov 1, 2024

duglin commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaryamTaj Feb 24, 2024 •

edited

Loading

MaryamTaj commented Jul 16, 2024 •

edited

Loading

MaryamTaj commented Sep 29, 2024 •

edited

Loading