MeltanoLabs / tap-cloudwatch

Apache License 2.0
1 stars 1 forks source link

Support relative end_date syntax #27

Open pnadolny13 opened 1 year ago

pnadolny13 commented 1 year ago

Initially discussed in https://github.com/MeltanoLabs/tap-cloudwatch/pull/26#issuecomment-1452625404 and https://github.com/meltano/sdk/issues/922.

TLDR;

What do you think of us defining end_date to be either an ISO 8601 date or datetime value (as is expected for start_date) or an ISO 8601 time interval. We could then use the following interpretation logic: ...... So, from the above, our proposed default of "5 minutes ago" would be "end_date": "-PT5M". To include exactly 10 days of data from the provided start_date, the end date could use a interval:"end_date": "P10D".

pnadolny13 commented 1 year ago

@aaronsteers I was thinking of making the buffer from https://github.com/MeltanoLabs/tap-cloudwatch/pull/26 configurable in a future iteration by adding another config key for something like buffer_s but your proposed solution sounds like it would achieve it without having a new key and it would also be a good implementation reference for https://github.com/meltano/sdk/issues/922.

One thing I'm considering is that as the tap developer I think I still want to force a buffer like this to avoid users shooting themselves in the foot. It took a fair amount of iteration and monitoring for me to figure out this was the problem, and I still need to see it work in prod to know for sure, so I'd want to make that an implementation detail of this tap. If users provide an end_date newer than 5 mins then I'd still automatically force the 5 mins buffer for safety. If we find cloudwatch docs explaining it with a more accurate SLA (likely in the seconds range vs mins) then I can shorten the hard coded buffer but I wouldnt want to give the user unsafe configuration options.

aaronsteers commented 1 year ago

One thing I'm considering is that as the tap developer I think I still want to force a buffer like this to avoid users shooting themselves in the foot.

Love it! Yes 👍

If users provide an end_date newer than 5 mins then I'd still automatically force the 5 mins buffer for safety. If we find cloudwatch docs explaining it with a more accurate SLA (likely in the seconds range vs mins) then I can shorten the hard coded buffer but I wouldnt want to give the user unsafe configuration options.

That's really smart. I hadn't really thought that far, but yes, I totally agree an enforced minimum offset sounds like the right thing to do.