fix: save client timestamp in database if present #31

darahayes · 2018-03-01T11:03:27Z

Verification Steps

Because this PR introduces changes to the database you must delete the old db container cached by docker-compose. Do the following:

docker ps -a | grep db_1
docker rm the ids of any containers returned back by the previous command
restart the db using docker-compose up --force-recreate --build db

For each case we are going to make a curl request and then check the results in the database.

Logging into the database and checking the contents of the mobileappmetrics table can be done as follows:

psql -U postgresql -d aerogear_mobile_metrics --host localhost
Password for user postgresql: #postgres

aerogear_mobile_metrics=> select * from mobileappmetrics;

For each section below, perform the curl request and then check the database for the expected result. We are going to test the following cases:

Client timestamp is present as a string
client timestamp is present as an integer
client timestamp is not present
client timestamp is not a valid timestamp

Client Timestamp Present (as a string)

curl -X POST \
  http://localhost:3000/metrics \
  -H 'Cache-Control: no-cache' \
  -H 'Content-Type: application/json' \
  -H 'Postman-Token: 6488102f-0b94-b698-61df-184edd1077f5' \
  -d '{
  "clientId": "abcdefgjdhdk",
  "timestamp": "123456789",
  "data": {
    "app": {
      "id": "com.example.someApp",
      "sdkVersion": "2.4.6",
      "appVersion": "256"
    },
    "device": {
      "platform": "android",
      "platformVersion": "27"
    }
  }
}'

Expected Result

200 OK

   clientid   |          event_time          |      client_time       |             data
--------------+------------------------------+------------------------+--------------------------------------------------------------------------------------------------------------------------------
 abcdefgjdhdk | 2018-03-01 10:55:15.98673+00 | 1973-11-29 21:33:09+00 | {"app": {"appId": "", "appVersion": "256", "sdkVersion": "2.4.6"}, "device": {"platform": "android", "platformVersion": "27"}}
(1 row)

Client Timestamp Preset as an Integer

curl -X POST \
  http://localhost:3000/metrics \
  -H 'Cache-Control: no-cache' \
  -H 'Content-Type: application/json' \
  -H 'Postman-Token: e7556ee7-220e-253b-3b9d-562847bc375b' \
  -d '{
  "clientId": "abcdefgjdhdk",
  "timestamp": 123456789,
  "data": {
    "app": {
      "id": "com.example.someApp",
      "sdkVersion": "2.4.6",
      "appVersion": "256"
    },
    "device": {
      "platform": "android",
      "platformVersion": "27"
    }
  }
}'

Expected Result

200 OK

   clientid   |          event_time           |      client_time       |              data
--------------+-------------------------------+------------------------+--------------------------------------------------------------------------------------------------------------------------------
 ... Previous results
 abcdefgjdhdk | 2018-03-01 10:58:30.349722+00 | 1973-11-29 21:33:09+00 | {"app": {"appId": "", "appVersion": "256", "sdkVersion": "2.4.6"}, "device": {"platform": "android", "platformVersion": "27"}}
(2 rows)

Client Timestamp is not present

curl -X POST \
  http://localhost:3000/metrics \
  -H 'Cache-Control: no-cache' \
  -H 'Content-Type: application/json' \
  -H 'Postman-Token: a69501dd-98b3-e5ee-51ae-12a14afe9b8c' \
  -d '{
  "clientId": "abcdefgjdhdk",
  "data": {
    "app": {
      "id": "com.example.someApp",
      "sdkVersion": "2.4.6",
      "appVersion": "256"
    },
    "device": {
      "platform": "android",
      "platformVersion": "27"
    }
  }
}'

Expected Result

200 OK

   clientid   |          event_time           |      client_time       |              data
--------------+-------------------------------+------------------------+--------------------------------------------------------------------------------------------------------------------------------
 ... Previous Results
 abcdefgjdhdk | 2018-03-01 11:00:13.101909+00 |                        | {"app": {"appId": "", "appVersion": "256", "sdkVersion": "2.4.6"}, "device": {"platform": "android", "platformVersion": "27"}}
(3 rows)

Client Timestamp is Invalid

curl -X POST \
  http://localhost:3000/metrics \
  -H 'Cache-Control: no-cache' \
  -H 'Content-Type: application/json' \
  -H 'Postman-Token: 4b58fcf0-ff80-bf70-4a78-14000c4a4749' \
  -d '{
  "clientId": "abcdefgjdhdk",
  "timestamp": "invalid",
  "data": {
    "app": {
      "id": "com.example.someApp",
      "sdkVersion": "2.4.6",
      "appVersion": "256"
    },
    "device": {
      "platform": "android",
      "platformVersion": "27"
    }
  }
}'

Expected Result

400 Bad Request

{
    "error": "Bad Request",
    "message": "timestamp must be a valid number",
    "statusCode": 400
}

Nothing should be saved in database

coveralls · 2018-03-01T11:10:02Z

Pull Request Test Coverage Report for Build #9

16 of 17 (94.12%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.6%) to 89.333%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/web/metricsHandler.go	0	1	0.0%

Totals
Change from base Build #8:	0.6%
Covered Lines:	201
Relevant Lines:	225

💛 - Coveralls

josemigallas · 2018-03-01T11:43:15Z

👀

david-martin · 2018-03-01T11:56:56Z

@darahayes would it not be better to be more opinionated and validate the timestamp should be a number, and always present (we may be using it in future)?
That would make validation slightly easier (don't need to try parse string into a number).
Plus it makes for a more well defined schema & API. The server sets the contract.
The client should stick to that contract.

@maleck13 Thoughts on this?

wtrocki · 2018-03-01T12:14:41Z

would it not be better to be more opinionated and validate the timestamp should be a number, and always present (we may be using it in future)?

We could go deeper and check if timestamp can be used to construct valid date.

maleck13 · 2018-03-01T12:17:38Z

@david-martin can you point me at the code that you are referring to?

darahayes · 2018-03-01T12:17:59Z

@david-martin I strongly considered this however there is one benefit to the approach I've implemented here.

You will notice that I previously defined the ClientTimestamp field in the Metric struct as an integer. With this approach, when the client sends an invalid value for timestamp, e.g. a string of gibberish, or even a valid integer but in a string format, JSON.marshal will return an error here which will return a generic error message to the client.

It seems that using the JSON.Number type when parsing numbers from client requests is pretty standard. That's what it's in the standard library for. The benefit of the current implementation means that we can send much more detailed errors back to the user. e.g.

We get this:

{
    "error": "Bad Request",
    "message": "timestamp must be a valid number",
    "statusCode": 400
}

instead of this:

{
    "error": "Bad Request",
    "message": "invalid JSON payload",
    "statusCode": 400
}

Now we could choose to send back whatever error message comes from JSON.marshal but I strongly advise against this because it reveals info about the internal implementation of the server. (bad security practice).

@wtrocki we check that the timestamp is a valid integer guarantees that a valid date can be constructed.

josemigallas · 2018-03-01T12:18:50Z

@darahayes if sending the metrics many times, new rows are added, right? If so, it works using the app 👍

darahayes · 2018-03-01T12:27:50Z

@david-martin @maleck13 Please see my updated comment above ^^

@maleck13 the code in question is the definition of ClientTimestamp as a *json.Number which is actually a string with some helper methods for converting the string to various numeric types.

This is the definition: https://github.com/aerogear/aerogear-app-metrics/pull/31/files#diff-2833ae78c6b668c514241e15283138a4L14

This is the validation logic: https://github.com/aerogear/aerogear-app-metrics/pull/31/files#diff-2833ae78c6b668c514241e15283138a4R49

This is where we eventually convert the string value to an integer (in the business logic): https://github.com/aerogear/aerogear-app-metrics/pull/31/files#diff-f0640233955767d355b6f2bdb5ee3b10R23

Note that the validation logic is called first in the handler, which means we never reach the business logic with an invalid timestamp. Only a good one or an empty one.

darahayes · 2018-03-01T12:29:52Z

@david-martin I'm just realising now you're also asking should we just make the timestamp always required. I'm totally happy to do this if you would like!

pb82 · 2018-03-01T12:31:36Z

Verified locally. Results were:

String:

 abcdefgjdhdk | 2018-03-01 12:16:58.187217+00 | 1973-11-29 21:33:09+00 | {"app": {"appId": "", "appVersion": "256", "sdkVersion": "2.4.6"}, "device": {"platform": "android", "platformVersion": "27"}}

Integer:

 abcdefgjdhdk | 2018-03-01 12:19:04.760987+00 | 1973-11-29 21:33:09+00 | {"app": {"appId": "", "appVersion": "256", "sdkVersion": "2.4.6"}, "device": {"platform": "android", "platformVersion": "27"}}

Not present:

 abcdefgjdhdk | 2018-03-01 12:20:03.509094+00 |                        | {"app": {"appId": "", "appVersion": "256", "sdkVersion": "2.4.6"}, "device": {"platform": "android", "platformVersion": "27"}}

Invalid (response in curl):

{"error":"Bad Request","message":"timestamp must be a valid number","statusCode":400}

pb82 · 2018-03-01T12:32:17Z

The timestamp field is left empty when no timestamp is submitted. Maybe the server should just generate and insert one?

maleck13 · 2018-03-01T12:40:48Z

This approach looks reasonable to me, we do validate that it is a number. The only concern I have is that we don't know that it is a valid timestamp (how important is that I am not sure) which @david-martin is perhaps what you are asking about

wtrocki · 2018-03-01T12:43:24Z

The timestamp field is left empty when no timestamp is submitted. Maybe the server should just generate and insert one?

In real case scenario that will be developer/integration error. Best to log that out as warning or something like this.

@wtrocki we check that the timestamp is a valid integer guarantees that a valid date can be constructed.

Getting your point. There are many different format of timestamps. Unix and Java timestamps are different (1000 times smaller number) etc.

darahayes · 2018-03-01T12:55:51Z

@maleck13 @david-martin We use the integer value (which is validated) to create a time object with time.Unix(). Given that it is a valid integer (which is guaranteed), it is guaranteed to be a valid timestamp. Is that reasonable? Of course I can also do the time.Unix() call in the Validate() function but it doesn't really make a difference.

paolobueno · 2018-03-01T12:58:43Z

+1 for timestamps being optional and generated on server time if not supplied.

Since metrics (either custom or our own defined ones) are for general telemetry most of them will not be very time-sensitive. The SDKs will always include the field anyway, so this should only cover users that for some reason decide to hit the http API directly.

It could allow for exotic clients like small iot devices that don't even keep track of time themselves.

david-martin · 2018-03-01T13:23:26Z

+1 for timestamps being optional and generated on server time if not supplied.

@paolobueno I think there's value in having the client timestamp & server timestamp.
In future, it may be the case that metrics are stored on device while offline & sent to the server when online. Client timestamp is important there (and we need to trust the time is set correctly on the device)

Server timestamp is good to have so we know when a metric entry was inserted in the db.

It could allow for exotic clients like small iot devices that don't even keep track of time themselves.

If this becomes a requirement, lets address it then. IOT devices may not be online all the time either, so how to tell when a metric was actually generated? Probably best not to think about these scenarios for IOT

david-martin · 2018-03-01T13:24:58Z

This approach looks reasonable to me, we do validate that it is a number. The only concern I have is that we don't know that it is a valid timestamp (how important is that I am not sure) which @david-martin is perhaps what you are asking about

@maleck13 If we validate it as a valid timestamp (can be parsed to a Date object) that would be sufficient IMO.

david-martin · 2018-03-01T13:27:31Z

@david-martin I'm just realising now you're also asking should we just make the timestamp always required. I'm totally happy to do this if you would like!

I think that would be best. It would avoid a situation where we use the client timestamp on the server in future, and there are old clients out there that don't send it.

wtrocki

Finally managed to get that running on my slow network. Verified with Android SDK having hardcoded config to localhost. Leaving approval to golang gurus.

david-martin · 2018-03-01T14:33:10Z

@darahayes I'm gonna approve for now. I've sent a mail to the group to help tease out some decision around timestamps rather than holding things up here
https://groups.google.com/forum/#!topic/aerogear/5Ra9CXzoj8Q

Dara Hayes added 5 commits March 1, 2018 10:27

fix: use varchar(128) for clientId

ab30d49

fix: enforce clientId max length at app layer

234370d

fix: save client timestamp in database

f378efc

fix: typo in comment

497809b

fix: update tests

f312964

darahayes requested review from paolobueno, psturc, wtrocki and StevenTobin March 1, 2018 11:10

wtrocki reviewed Mar 1, 2018

View reviewed changes

david-martin approved these changes Mar 1, 2018

View reviewed changes

darahayes merged commit dcc91f8 into master Mar 1, 2018

darahayes deleted the client-time branch March 1, 2018 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: save client timestamp in database if present #31

fix: save client timestamp in database if present #31

darahayes commented Mar 1, 2018

coveralls commented Mar 1, 2018

josemigallas commented Mar 1, 2018

david-martin commented Mar 1, 2018

wtrocki commented Mar 1, 2018

maleck13 commented Mar 1, 2018

darahayes commented Mar 1, 2018 •

edited

josemigallas commented Mar 1, 2018 •

edited

darahayes commented Mar 1, 2018

darahayes commented Mar 1, 2018

pb82 commented Mar 1, 2018

pb82 commented Mar 1, 2018

maleck13 commented Mar 1, 2018

wtrocki commented Mar 1, 2018 •

edited

darahayes commented Mar 1, 2018 •

edited

paolobueno commented Mar 1, 2018

david-martin commented Mar 1, 2018

david-martin commented Mar 1, 2018

david-martin commented Mar 1, 2018

wtrocki left a comment

david-martin commented Mar 1, 2018

fix: save client timestamp in database if present #31

fix: save client timestamp in database if present #31

Conversation

darahayes commented Mar 1, 2018

Verification Steps

Client Timestamp Present (as a string)

Expected Result

Client Timestamp Preset as an Integer

Expected Result

Client Timestamp is not present

Expected Result

Client Timestamp is Invalid

Expected Result

coveralls commented Mar 1, 2018

Pull Request Test Coverage Report for Build #9

💛 - Coveralls

josemigallas commented Mar 1, 2018

david-martin commented Mar 1, 2018

wtrocki commented Mar 1, 2018

maleck13 commented Mar 1, 2018

darahayes commented Mar 1, 2018 • edited

josemigallas commented Mar 1, 2018 • edited

darahayes commented Mar 1, 2018

darahayes commented Mar 1, 2018

pb82 commented Mar 1, 2018

pb82 commented Mar 1, 2018

maleck13 commented Mar 1, 2018

wtrocki commented Mar 1, 2018 • edited

darahayes commented Mar 1, 2018 • edited

paolobueno commented Mar 1, 2018

david-martin commented Mar 1, 2018

david-martin commented Mar 1, 2018

david-martin commented Mar 1, 2018

wtrocki left a comment

Choose a reason for hiding this comment

david-martin commented Mar 1, 2018

darahayes commented Mar 1, 2018 •

edited

josemigallas commented Mar 1, 2018 •

edited

wtrocki commented Mar 1, 2018 •

edited

darahayes commented Mar 1, 2018 •

edited