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.

Sign up for GitHub

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

Jump to bottom

Transactions with UNIX timestamps #167

Merged
merged 5 commits into from
Mar 16, 2020
Merged

Transactions with UNIX timestamps #167

merged 5 commits into from
Mar 16, 2020

Conversation

unleashed
Copy link
Contributor

This adds support for specifying transaction timestamps as UNIX Epoch timestamps, ie. number of seconds since Jan 1, 1970.

@unleashed unleashed requested a review from davidor March 10, 2020 12:03
@unleashed unleashed force-pushed the transactions-with-unix-timestamps branch 5 times, most recently from ba86504 to 90b9f8e Compare March 10, 2020 12:18
philipgough added a commit to 3scale/3scale-go-client that referenced this pull request Mar 10, 2020
@philipgough
http: Add support for reporting timestamps in HTTP client
51531b7 
Prior to this change, timestamps passed as part of a transaction to the
report API were totally ignored. This change adds any non-default
timestamps to the encode string to be sent to 3scale server.

Requires 3scale version(s) 3.8+
See: 3scale/apisonator#167
@philipgough philipgough mentioned this pull request Mar 10, 2020
Merged
davidor
davidor reviewed Mar 11, 2020
View reviewed changes
test/integration/report_test.rb Show resolved Hide resolved
test/integration/report_test.rb Outdated Show resolved Hide resolved
@unleashed unleashed force-pushed the transactions-with-unix-timestamps branch from 90b9f8e to 3cf3795 Compare March 11, 2020 11:32
@unleashed unleashed requested a review from davidor March 11, 2020 11:33
@unleashed
Copy link
Contributor Author

So... 2012garbage2012 is an invalid date, but '201210garbage201210' is totally valid, no questions asked.

@davidor
Copy link
Contributor

davidor commented Mar 11, 2020

What? 😵

@unleashed
Copy link
Contributor Author

What? dizzy_face

fliptableagain

@unleashed
Copy link
Contributor Author

So I think the only reasonable way out of this surprising legacy from the past, especially given this is behaviour is not documented, would be to remove this offending test, since it does not make sense any way to do the negative testing (and even then it was so broken), and accept (for now) the discovery that completely broken timestamps like the one I specified can actually mean some random thing to the system, just the same way it's always been, and then consider whether to fix what was already broken adding validation.

Ideas?

@davidor
Copy link
Contributor

davidor commented Mar 11, 2020

@unleashed That sounds good to me 👍

unleashed added a commit that referenced this pull request Mar 11, 2020
@unleashed
test/extensions/time: add a pending test for a known failure in times…
5e9593a 
…tamp parsing

The current code uses Date._parse without proper validation, and while
it correctly says '2012garbage2012' is not a date, it swallows the
string '201210garbage201210' as a date.

See #167.
@unleashed unleashed force-pushed the transactions-with-unix-timestamps branch from 3cf3795 to 5e9593a Compare March 11, 2020 18:36
@unleashed
Copy link
Contributor Author

So in the end I added a pending test that is way more likely to bug people when running the suite to go and fix this issue one way or another, and also changed the "negative" testing to look for changes in 201201.

@unleashed
extensions/time: accept UNIX timestamps when parsing dates to UTC
1073dfd 
This allows us to take UNIX timestamps for transactions.
@unleashed
test/{unit/extensions/time,integration/report}: test that we accept U…
530bc06 
…NIX timestamps

We had to disable some negative testing, ie. value 0 does not parse, and
fix a value being tested that matched a UNIX timestamp, as well as being
incorrect anyway for the negative test that was being executed right
next after it.
@unleashed
listener: document timestamp field in transactions receives UNIX time…
65edbdc 
…stamps
@unleashed
active_docs/SM API: update swagger json docs to include timestamp cha…
7835f86 
…nges
@unleashed
test/extensions/time: add a pending test for a known failure in times…
78459a9 
…tamp parsing

The current code uses Date._parse without proper validation, and while
it correctly says '2012garbage2012' is not a date, it swallows the
string '201210garbage201210' as a date.

See #167.
@unleashed unleashed force-pushed the transactions-with-unix-timestamps branch from 5e9593a to 78459a9 Compare March 11, 2020 18:59
@davidor
Copy link
Contributor

davidor commented Mar 12, 2020

Is there something we can do about that? Have you taken a look to see if we can replace that Date._parsecall with a similar call that does not cause other issues?

@unleashed It'd be good to open an issue so we remember to address that.

@unleashed
Copy link
Contributor Author

Is there something we can do about that? Have you taken a look to see if we can replace that Date._parsecall with a similar call that does not cause other issues?

Maybe, but would certainly change more semantics - we can defer that to another PR.

@unleashed unleashed mentioned this pull request Mar 16, 2020
Open
@davidor davidor merged commit 3c7245d into master Mar 16, 2020
@bors bors bot deleted the transactions-with-unix-timestamps branch March 16, 2020 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidor davidor davidor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@unleashed @davidor