feat: Add Time configtype #1905

rhino1998 · 2024-09-23T16:36:22Z

Summary

This adds a new Time type to be used in plugin specs that can represent either fixed times or relative times. A future PR should extend the duration parsing to handle days, etc.

kind: source
spec:
  name: "orb"
  registry: "grpc"
  path: "localhost:7777"
  version: "v1.0.0"
  tables:
    ["*"]
  destinations:
    - "sqlite"
  spec:
    api_key: "${CQ_ORB_API_KEY}"
    timeframe_start: 10 days ago
    timeframe_end: "2024-09-25T03:52:40.14157935-04:00"

Here timeframe_start and timeframe_end are both configtype.Time types. Using mixed relative/fixed timestamps for timeframes isn't really useful here as the relative one will be relative to the run-time of the sync but in this example having

timeframe_start: 10 days ago
timeframe_end: now

or even omitting timeframe_end entirely is more useful

timeframe_start: 10 days ago

The plugin is free to define default like so

type Spec struct {
	// TimeframeStart is the beginning of the timeframe in which to fetch cost data.
	TimeframeStart configtype.Time `json:"timeframe_start"`

	// TimeframeEnd is the beginning of the timeframe in which to fetch cost data.
	TimeframeEnd configtype.Time `json:"timeframe_end"`

	// ...
}


func (s *Spec) SetDefaults() {
	if s.TimeframeStart.IsZero() {
		s.TimeframeStart = configtype.NewRelativeTime(-1 * 365 * 24 * time.Hour)
	}

	if s.TimeframeEnd.IsZero() {
		s.TimeframeEnd = configtype.NewRelativeTime(0)
	}
}

erezrokah

Hi @rhino1998, nice 🚀 Can you give an example how this will be used in a plugin and in a spec file?

rhino1998 · 2024-09-23T17:27:30Z

Added some more context. Should configtype.Time.Time take a time.Time instead of a func() time.Time?

erezrokah

I like the idea of having this kind of type as an SDK helper.

A few comments:

If the purpose is to make it easier to configure relative times, I would use https://github.com/tj/go-naturaldate/tree/master instead of a wrapper over duration
I would hide the IsRelative and NewRelativeTime and do NewTime(t string) that accepts either an RFC3339 or a natural date string. I don't see a reason consumers need to know the internal representation, also I don't think we even need typ timeType, as you can have NewTime(t string, base time.Time) and convert the string on creation.

configtype/time.go

hermanschaaf · 2024-09-24T09:35:25Z

If the purpose is to make it easier to configure relative times, I would use https://github.com/tj/go-naturaldate/tree/master instead of a wrapper over duration

I would push back on this; the library was last updated 4 years ago, and supports much more than we need to support. I think we would be better off supporting a small, well-defined number of formats.

Also I think the purpose here is not to make it easier, it's to make it possible at all (in Cloud), in a standardized way.

erezrokah · 2024-09-24T09:50:36Z

the library was last updated 4 years ago

I agree it hasn't change in a long time, but it doesn't have any other dependencies (other than the ones used for testing), and the author is https://github.com/tj (https://x.com/tjholowaychuk), which has a bunch of other popular repositories like https://github.com/tj/commander.js and https://github.com/koajs/koa

well-defined number of formats.

Agree it does more than we need, but timeframe_start: -1000h seems more machine friendly than human friendly to me

Also I think the purpose here is not to make it easier, it's to make it possible at all (in Cloud), in a standardized way.

Can you add more context on the Cloud use case? We want the backend to specify a duration via spec generated via backend code in Go? If so is it possible to convert the duration to time before?

hermanschaaf · 2024-09-24T11:27:40Z

Can you add more context on the Cloud use case? We want the backend to specify a duration via spec generated via backend code in Go? If so is it possible to convert the duration to time before?

We want users to be able to specify a relative time, such as "30 days ago", which will be stored as part of the spec. Every time the sync runs, this should be converted to a timestamp relative to the current time. This way, we don't need to involve the backend to do string interpolation on the spec, and things remain relatively simple.

Right now, what happens in many plugins is they allow only a fixed timestamp to be specified, such as 2024-05-01T00:00:00Z, or a default value, often relative (30 days ago or now). This doesn't lend us the flexibility we desire for scheduled syncs in Cloud

hermanschaaf · 2024-09-24T11:30:36Z

Agree it does more than we need, but timeframe_start: -1000h seems more machine friendly than human friendly to me

I agree with this.

I'm also okay with using that library, but if we do, let's not support everything that the library supports, but instead limit it to a defined list of supported formats that we can control.

I would maybe say something like:

now
x seconds [ago|from now]
x minutes [ago|from now]
x hours [ago|from now]
x days [ago|from now]

would be a good enough start (we can always add more if requested)

erezrokah · 2024-09-24T11:33:43Z

I would maybe say something like:

now

x seconds [ago|from now]

x minutes [ago|from now]

x hours [ago|from now]

x days [ago|from now]

would be a good enough start (we can always add more if requested)

Sounds like a plan to me

hermanschaaf · 2024-09-24T12:09:54Z

I forgot that in addition to relative times, we'd also want to support fixed timestamps of course, probably using RFC 3339. And also some support for plain dates, e.g. 2024-05-10, for plugins that need it.

hermanschaaf · 2024-09-25T08:07:27Z

configtype/time_test.go

+		{
+			Name: "complex relative duration",
+			Err:  false,
+			Spec: `"10 months 3 days 4h20m from now"`,


I think if we're going to expose MarshalJSON then we should probably make this format 10 months 3 days 4 hours 20 minutes from now for consistency (and so we don't need to make a breaking change later to fix it)

Alternatively we could remove the ability to specify an arbitrary duration in the constructor (just remove the constructor) and only allow this to be constructed using a simple string like 10 days ago

The reason for the weird hybrid format is for backwards compat for the Duration type. It ensures that anything constructed from a plain duration will emit the same json as before this PR. It may be reasonable to separate the json serialization format and the string parsed format and make the serialization format (but still unmarshalling the nicer human format) just [-+](\d+[a-zA-Z])+ (basically time.ParseDuration format but with additional units). That frees us to have the string format be nicer and human readable

Would https://github.com/cloudquery/plugin-sdk/pull/1905/files#r1775175473 solve this?

erezrokah

Thanks for the following @rhino1998, added a few more comments

erezrokah · 2024-09-25T12:36:46Z

configtype/duration.go

 	"github.com/invopop/jsonschema"
 )

+var (


I prefer if we don't change the semantics of the existing duration type, as that could breaking existing behavior for consumers of this type

All previously valid values are still valid for the duration type. This just adds more possible valid values. Additionally all previously valid values will serialize to the same string after this PR in its current state. (unless I missed something)

This just adds more possible valid values.

Exactly, so now the JSON validation is more relaxed, so something that would have failed before can pass now.
While this could be seen as a feature, it's better if plugins opt-in to it, then document the new behavior

configtype/time.go

erezrokah · 2024-09-25T12:53:27Z

configtype/time.go

+}
+
+func (t Time) MarshalJSON() ([]byte, error) {
+	switch t.typ {


I think we can keep the original string that we got in ParseTime and return that instead, WDYT?

263750b, but unclear what the handling for an uninitialized Time or Duration should be. Currently it just marshals to ""

Also, as a side-effect Time and Duration are no longer == comparable in a useful way

263750b, but unclear what the handling for an uninitialized Time or Duration should be. Currently it just marshals to ""

I think we need to decide that and pick whatever we want to happen, we can

Error out on uninitialized

Return MarshalJSON of the zero value of time.Time

Return MarshalJSON of the zero value of time.Duration

Return null

Maybe more options

Not sure what's the best option but it needs to be consistent with UnmarshalJSON.
We can see what happens for time.Time and time.Duration zero values

Also, as a side-effect Time and Duration are no longer == comparable in a useful way

How could you compare Time and Duration before? Where do you need this comparison and how will it be used?

Duration was comparable with == (as it was just a struct) containing a time.Duration. I'm unsure if that was used anywhere but removing that is a breaking change. Adding days,months,etc to the struct maintained comparability for equal values but adding the input string field breaks that for anything besides values that had the same input string even if they're otherwise equivalent (including whitespace differences).

I guess Time wasn't usefully comparable with == as while time.Time is == comparable doing doesn't handle timezones and so is not recommended.

As for where it is needed, it isn't exactly for time, but the duration change is breaking change in the API (unclear if anything actually uses it though)

Ok so:

I don't think we should modify the existing duration config type. Relaxing the parsing is also a breaking change

Even if we did modify the existing type, we can used the parsed value of the duration for Equals

Regarding 2., that still will have broken == comparison, although again unclear if that is used externally or if that even counts as an explicit part of the API to be broken

Regarding default behavior, returning the zero value of time seems sane as it'll allow the current IsZero check to work. Although that is "0001-01-01T00:00:00Z"

https://go.dev/play/p/qI6SprLcw5p

configtype/time.go

Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>

hermanschaaf · 2024-09-30T16:21:42Z

@erezrokah @rhino1998 I lost track a bit, is there still anything unresolved on this PR?

erezrokah · 2024-09-30T17:37:23Z

@erezrokah @rhino1998 I lost track a bit, is there still anything unresolved on this PR?

Not sure https://github.com/cloudquery/plugin-sdk/pull/1905/files#r1774764467 is resolved, also #1905 (comment)

I'd rather if we don't change the existing type behavior as that implicitly change spec parsing behavior for plugins. If plugins want to support the new human friend format mixed duration/time format they should do it explicitly I think

rhino1998 · 2024-09-30T17:41:35Z

@erezrokah @rhino1998 I lost track a bit, is there still anything unresolved on this PR?

Not sure https://github.com/cloudquery/plugin-sdk/pull/1905/files#r1774764467 is resolved, also #1905 (comment)

I'd rather if we don't change the existing type behavior as that implicitly change spec parsing behavior for plugins. If plugins want to support the new human friend format mixed duration/time format they should do it explicitly I think

The new mixed duration/time format is opt-in, but the new duration format does extend the old plain time.ParseDuration-based format.

How would you prefer the opt-in be done? A DurationV2 type? A separate package?

erezrokah · 2024-09-30T17:49:19Z

How would you prefer the opt-in be done? A DurationV2 type? A separate package?

I don't mind I mostly don't want to change the existing behavior of the current type. You can also put everything under the new Time type without a separate type

erezrokah · 2024-09-30T18:03:18Z

configtype/time_test.go

+		{
+			Name: "complex relative duration",
+			Err:  false,
+			Spec: `"10 months 3 days 4h20m from now"`,


Should we change this to @hermanschaaf's suggestion of 10 months 3 days 4 hours 20 minutes from now?

In its current state, both are handled and whatever was input will be emitted. I added some test cases to make this clearer. 1a5ae0a

erezrokah · 2024-10-01T09:26:15Z

Nothing blocking on my end, so you can merge at will. We might want to wait with releasing the new SDK until after today's plugins release cycle to avoid creating noise for the plugins teams

disq · 2024-10-01T14:52:09Z

I think (sorry for the late comment) the from now syntax can be confusing, as people could expect 2 days from <some date here> or worse, 2 days from new year's eve to work.

rhino1998 · 2024-10-01T15:12:03Z

I think (sorry for the late comment) the from now syntax can be confusing, as people could expect 2 days from <some date here> or worse, 2 days from new year's eve to work.

We could pretty easily add support for stuff like 2 days from <timestamp>. That actually generalizes the input pretty nicely and will simplify/merge validation cases

rhino1998 · 2024-10-02T16:07:00Z

@disq Do you consider the above blocking?

disq · 2024-10-03T08:30:37Z

@disq Do you consider the above blocking?

@rhino1998 No not at all.

erezrokah · 2024-10-22T13:58:50Z

Not sure what's the status here, should we merge this or close it as stale?

🤖 I have created a release *beep* *boop* --- ## [4.68.0](v4.67.1...v4.68.0) (2024-10-31) ### Features * Add Time configtype ([#1905](#1905)) ([f57c3eb](f57c3eb)) * Support for quota query interval header ([#1948](#1948)) ([bfce6fe](bfce6fe)) * Test `MeterUsage` API call on initial setup of client ([#1906](#1906)) ([78df77d](78df77d)) ### Bug Fixes * Clean up usage retry logic ([#1950](#1950)) ([ca982f9](ca982f9)) * **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.25.0 ([#1946](#1946)) ([b8e3e10](b8e3e10)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

feat: Add Time configtype

7bc83b0

rhino1998 requested review from a team, hermanschaaf and marianogappa September 23, 2024 16:36

github-actions bot added the feat label Sep 23, 2024

cleanup

6c6e1a6

erezrokah reviewed Sep 23, 2024

View reviewed changes

rhino1998 added 3 commits September 23, 2024 12:41

return zero time on nil Time

34a31d6

add time type and fix zero value

93f2449

function typing

852db5a

github-actions bot added feat and removed feat labels Sep 23, 2024

zero handling

70a90a4

github-actions bot added feat and removed feat labels Sep 23, 2024

use time.Time instead of func() time.Time

3111bb1

github-actions bot added feat and removed feat labels Sep 23, 2024

erezrokah requested changes Sep 24, 2024

View reviewed changes

erezrokah reviewed Sep 24, 2024

View reviewed changes

configtype/time.go Outdated Show resolved Hide resolved

add ParseTime

3c5a1ac

lints

35bf99e

hermanschaaf requested a review from erezrokah September 25, 2024 08:01

hermanschaaf reviewed Sep 25, 2024

View reviewed changes

erezrokah requested changes Sep 25, 2024

View reviewed changes

rhino1998 and others added 4 commits September 25, 2024 14:43

AsTime

bdb52db

Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>

fix test

bdfd108

implement requested changes

263750b

remove unused

465e5d7

rhino1998 added 3 commits September 30, 2024 13:57

dont modify Duration

e972602

remove unused

b519916

group vars

5768cbc

erezrokah reviewed Sep 30, 2024

View reviewed changes

add clarifying tests

1a5ae0a

erezrokah approved these changes Oct 1, 2024

View reviewed changes

rhino1998 added the automerge label Oct 22, 2024

Merge branch 'main' into feat/configtype-time

fdcdcad

kodiakhq bot merged commit f57c3eb into main Oct 22, 2024
7 checks passed

kodiakhq bot deleted the feat/configtype-time branch October 22, 2024 19:58

cq-bot mentioned this pull request Oct 22, 2024

chore(main): Release v4.68.0 #1945

Merged

feat: Add Time configtype #1905

feat: Add Time configtype #1905

Uh oh!

Conversation

rhino1998 commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

rhino1998 commented Sep 23, 2024

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hermanschaaf commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erezrokah commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hermanschaaf commented Sep 24, 2024

Uh oh!

hermanschaaf commented Sep 24, 2024

Uh oh!

erezrokah commented Sep 24, 2024

Uh oh!

hermanschaaf commented Sep 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhino1998 Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhino1998 Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hermanschaaf commented Sep 30, 2024

Uh oh!

erezrokah commented Sep 30, 2024

Uh oh!

rhino1998 commented Sep 30, 2024

Uh oh!

rhino1998 commented Sep 23, 2024 •

edited

Loading

hermanschaaf commented Sep 24, 2024 •

edited

Loading

erezrokah commented Sep 24, 2024 •

edited

Loading

rhino1998 Sep 25, 2024 •

edited

Loading

rhino1998 Sep 26, 2024 •

edited

Loading

erezrokah commented Sep 30, 2024 •

edited

Loading