fix: enhancements to config parsing and dao #28

darahayes · 2018-02-27T16:30:00Z

This PR introduces a number of enhancements:

Client ID max length is now enforced at the business logic layer using metric.Validate(). This allows us to respond with a proper message when the max length has been exceeded. Ping @psturc.
the dao package now has some small amount of retry logic which will let the server retry connecting to the db for up to 5 seconds before failing. This solves a minor issue particularly in docker-compose where the server sometimes starts before the database is ready to accept connections. Ping @josemigallas.
Configuration parsing has been hardened and refactored. All connection options supported by the postgres driver are now supported and the connection string can be dynamically generated based off what config/env vars are present. This means we won't need to do any refactoring of the DB connection in the future.
The dao package now calls db.SetMaxOpenConns() with a default of 100 which is apparently the default allowed by Postgres. See docs.

Verification

Delete all containers previously run by docker-compose. You can get the container IDs with the following docker ps -a | grep aerogearappmetrics

Then run docker rm <id> for each id.

Then forcefully recreate your docker-compose environment with the following:

docker-compose up --build --force-recreate

You should notice 2 things:

The metrics server did not crash while trying to connect to the database.
Make the following curl request:

curl -X POST \
  http://localhost:3000/metrics \
  -H 'Cache-Control: no-cache' \
  -H 'Content-Type: application/json' \
  -H 'Postman-Token: 7011091f-b785-2c89-bfe8-55383df253f9' \
  -d '{
  "clientId": "453de743211112345678900987654312345678908776423567890876543678907654356789076543536789765436789076543256789076543256789654321456789654321456789765432567869765432567896543256789765432145678976543214567897654324567896543214567897654321245678976543256789765432145678965432567896543256786543214567865432",
  "data": {
    "app": {
      "id": "com.example.someApp",
      "sdkVersion": "2.4.6",
      "appVersion": "256"
    },
    "device": {
      "platform": "android",
      "platformVersion": "27"
    }
  }
}'

And you should see the following message:

{
    "error": "Bad Request",
    "message": "clientId exceeded maximum length of 128",
    "statusCode": 400
}

psturc · 2018-02-28T08:32:17Z

@darahayes I couldn't verify according to the steps provided.

I'm getting error when running the docker-compose command:

Step 4/5 : COPY ${BINARY} /opt/aerogear-app-metrics
ERROR: Service 'api' failed to build: COPY failed: stat /var/lib/docker/tmp/docker-builder721829778/dist/linux_amd64/aerogear-app-metrics: no such file or directory

darahayes · 2018-02-28T08:45:24Z

My bad @psturc you must run make build_linux before running docker-compose

psturc · 2018-02-28T09:07:01Z

I'm not sure what I'm doing wrong - make build or make build_linux does not work for me.
However it's working fine in master branch.

darahayes · 2018-02-28T09:19:59Z

@psturc This is strange. It seems to me by the error message that you do not have the latest code. It's building on my machine and in CircleCI.

Can you try pull this branch again and ensure you have the latest changes? the most recent commit has the message fix: enforce clientId max length at app layer

psturc · 2018-02-28T09:32:18Z

@darahayes yep, confirming I have the latest changes:

commit a948c9820c2993bbce6c2eecebdb0ee6b400625e (HEAD -> db-fix, upstream/db-fix)
Author: Dara Hayes <dara.hayes@redhat.com>
Date:   Tue Feb 27 23:15:29 2018 +0000

    fix: enforce clientId max length at app layer

This will be probably some stupid think I'm overlooking.

btw I'm using

Docker version 17.09.0-ce, build afdb6d4
go version go1.10 darwin/amd64

…o db-fix

coveralls · 2018-02-28T09:35:11Z

Pull Request Test Coverage Report for Build #7

54 of 56 (96.43%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+2.3%) to 88.732%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/dao/db.go	10	12	83.33%

Totals
Change from base Build #5:	2.3%
Covered Lines:	189
Relevant Lines:	213

💛 - Coveralls

darahayes · 2018-02-28T09:35:58Z

@psturc I've merged master back into this branch can you try pulling again? If that doesn't work can you try deleting and fetching the branch again? Or perhaps cloning the repo again? I'm out of ideas beyond this 😅

psturc · 2018-02-28T12:27:08Z

@darahayes code lgtm, the clientId validation is fine, I think limit for 128 chars should be sufficient.

However I cannot verify this locally, I tried everything you've suggested above, but the build is still failing with the same error.

Could anybody else please try that too?

darahayes · 2018-02-28T12:48:59Z

@psturc I think I figured out your issue: in the logs you sent me I saw the path ~/gocode/src/github.com/aerogear/aerogear-metrics-api

Please ensure you have the latest code cloned to the directory aerogear-app-metrics. I think your compiler is pulling in old code from a different directory

We changed the name of the service recently

psturc · 2018-02-28T12:54:17Z

@darahayes lol of course, nice catch Dara :) I will verify it locally soon then

paolobueno · 2018-02-28T13:03:49Z

pkg/mobile/types.go

@@ -28,11 +32,19 @@ type DeviceMetric struct {
 	PlatformVersion string `json:"platformVersion"`
 }

+const clientIdMaxLength = 128
+
+var clientIdLengthError = fmt.Sprintf("clientId exceeded maximum length of %v", clientIdMaxLength)


@darahayes I'm missing context but is the clientId not a fixed length across the SDKs? Since you've pinged @psturc on the PR description probably you two are aware of why this is needed.

@paolobueno I have no idea what the clientId length is coming from Android/iOS. I just chose 128 as a sane default. If I can get more specific info that would be great!

it's 36 on iOS side

On the android sdk it's a UUID: https://github.com/aerogear/aerogear-android-sdk/blob/master/core/src/main/java/org/aerogear/mobile/core/utils/ClientIdGenerator.java#L39 so also 36.

psturc · 2018-02-28T13:24:05Z

@darahayes I just noticed one thing, unfortunately unrelated to this PR.

There's a difference between API and client side in one property (appId vs id):
Android & iOS vs. metrics-api

I think appId property is more clear - so maybe we would need a change on metrics-api side, wdyt?

darahayes · 2018-02-28T13:25:51Z

@psturc Great catch. Let's update the server and the original proposal.

darahayes · 2018-02-28T13:42:36Z

@psturc I've updated the id property as suggested.

psturc · 2018-02-28T15:45:37Z

Ok, verified that appId chage

darahayes · 2018-02-28T16:38:53Z

@psturc Did you manage to verify the other steps?

psturc · 2018-02-28T17:11:24Z

@darahayes yes, sorry I forgot to mention that it was all verified with iOS client

StevenTobin

Verified locally using verification instructions 👍

* fix: use varchar(128) for clientId * fix: basic connection retry logic * fix(config): major improvements to config parser * fix(dao) use new config and set max db connections * fix: enforce clientId max length at app layer * fix: rename data.app.id label to data.app.appId

Dara Hayes added 4 commits February 27, 2018 15:24

fix: use varchar(128) for clientId

d61b320

fix: basic connection retry logic

2b34ea6

fix(config): major improvements to config parser

36c4637

fix(dao) use new config and set max db connections

c071716

darahayes changed the title ~~Minor fixes to database implementation~~ fix: enhancements to config parsing and dao implementation Feb 27, 2018

darahayes changed the title ~~fix: enhancements to config parsing and dao implementation~~ fix: enhancements to config parsing and dao Feb 27, 2018

darahayes requested review from paolobueno, psturc and josemigallas February 27, 2018 22:24

fix: enforce clientId max length at app layer

a948c98

darahayes force-pushed the db-fix branch from 44f22dc to a948c98 Compare February 27, 2018 23:15

Merge branch 'master' of github.com:aerogear/aerogear-metrics-api int…

9d35f9f

…o db-fix

paolobueno reviewed Feb 28, 2018

View reviewed changes

fix: rename data.app.id label to data.app.appId

e40ba70

darahayes requested a review from wtrocki February 28, 2018 13:42

psturc approved these changes Feb 28, 2018

View reviewed changes

StevenTobin approved these changes Mar 1, 2018

View reviewed changes

darahayes merged commit 1478ca7 into master Mar 1, 2018

darahayes deleted the db-fix branch March 1, 2018 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enhancements to config parsing and dao #28

fix: enhancements to config parsing and dao #28

darahayes commented Feb 27, 2018 •

edited

Loading

psturc commented Feb 28, 2018

darahayes commented Feb 28, 2018

psturc commented Feb 28, 2018

darahayes commented Feb 28, 2018

psturc commented Feb 28, 2018

coveralls commented Feb 28, 2018 •

edited

Loading

darahayes commented Feb 28, 2018

psturc commented Feb 28, 2018

darahayes commented Feb 28, 2018 •

edited

Loading

psturc commented Feb 28, 2018

paolobueno Feb 28, 2018

darahayes Feb 28, 2018

psturc Feb 28, 2018

paolobueno Feb 28, 2018

psturc commented Feb 28, 2018

darahayes commented Feb 28, 2018

darahayes commented Feb 28, 2018

psturc commented Feb 28, 2018

darahayes commented Feb 28, 2018

psturc commented Feb 28, 2018

StevenTobin left a comment •

edited

Loading

fix: enhancements to config parsing and dao #28

fix: enhancements to config parsing and dao #28

Conversation

darahayes commented Feb 27, 2018 • edited Loading

Verification

psturc commented Feb 28, 2018

darahayes commented Feb 28, 2018

psturc commented Feb 28, 2018

darahayes commented Feb 28, 2018

psturc commented Feb 28, 2018

coveralls commented Feb 28, 2018 • edited Loading

Pull Request Test Coverage Report for Build #7

💛 - Coveralls

darahayes commented Feb 28, 2018

psturc commented Feb 28, 2018

darahayes commented Feb 28, 2018 • edited Loading

psturc commented Feb 28, 2018

paolobueno Feb 28, 2018

Choose a reason for hiding this comment

darahayes Feb 28, 2018

Choose a reason for hiding this comment

psturc Feb 28, 2018

Choose a reason for hiding this comment

paolobueno Feb 28, 2018

Choose a reason for hiding this comment

psturc commented Feb 28, 2018

darahayes commented Feb 28, 2018

darahayes commented Feb 28, 2018

psturc commented Feb 28, 2018

darahayes commented Feb 28, 2018

psturc commented Feb 28, 2018

StevenTobin left a comment • edited Loading

Choose a reason for hiding this comment

darahayes commented Feb 27, 2018 •

edited

Loading

coveralls commented Feb 28, 2018 •

edited

Loading

darahayes commented Feb 28, 2018 •

edited

Loading

StevenTobin left a comment •

edited

Loading