Database Integration #10

darahayes · 2018-02-19T16:32:58Z

initial connection and setup logic
Business logic for inserting records
Unit/Integration tests

Update

This PR ended up turning into a mega PR but this is the initial implementation that allows us to store mobile metrics in the database. There are a number of things to be aware of:

There is a small amount of validation. The incoming data is parsed into a struct which filters out unnecessary fields. additionally, there are some small checks at the DB layer. ClientId cannot be an empty string and by data field will not accept invalid json. There is no other validation (right now).
Right now, the client timestamp is being parsed but not being saved. The event_time field is populated by Postgres at the time of insert.
The db_test.go and dao_test.go are actually integration tests. However by default they will not run because of the // +build integration build tag added at the top of those files. You can run integration + unit tests using the standard test command, but also passing the flag tags=integration @maleck13 Not sure how you feel about this. I found this solution in this stackoverflow answer. I know it's not as clear as integration tests having their own folder but it's still fairly obvious by looking at the file that the test is an integration test.
Removed references to timescaledb for now (as suggested by @david-martin )
Still need to implement some standardised logging approach
Docs need updating
Code Coverage at a fairly good 87.6%

The original estimate was 4d and I've spent about 3 days on this so far so there's still time to do some housekeeping around docs, build process, release, etc etc.

How to verify

fetch this branch and start the database using docker-compose.

Now start the server using go run cmd/metrics-api/metrics-api.go

You should see a message like

2018/02/22 17:28:30 Starting application... going to listen on :3000

Now you can try out the /metrics endpoint with the following curl request:

curl -X POST \
  http://localhost:3000/metrics \
  -H 'Cache-Control: no-cache' \
  -H 'Content-Type: application/json' \
  -H 'Postman-Token: 76a14738-976f-f0ea-4039-12efeebf4f1e' \
  -d '{
  "clientId": "12345",
  "data": {
  	"app": {
    "id": "com.example.someApp",
    "sdkVersion": "2.4.6",
    "appVersion": "256"
	},
	"device": {
    	"platform": "android",
    	"platformVersion": "27"
	}
  }
}'

You should get a the following response:

{"timestamp":0,"clientId":"12345","data":{"app":{"id":"com.example.someApp","sdkVersion":"2.4.6","appVersion":"256"},"device":{"platform":"android","platformVersion":"27"}}}

Note the timestamp field in the response corresponds to the client timestamp which is currently not being handled or saved.

You can log into the database and verify that the record was created:

psql -U postgres --host localhost
password: postgres

\c aerogear_mobile_metrics
select * from mobileappmetrics;

           clientid           |          event_time           |                                                                      data
------------------------------+-------------------------------+------------------------------------------------------------------------------------------------------------------------------------------------
 org.aerogear.metrics.testing | 2018-02-22 15:14:03.859053+00 | {"app": {"id": "com.example.someApp", "appVersion": "256", "sdkVersion": "2.4.6"}, "device": {"platform": "android", "platformVersion": "27"}}

I'd love to get your reviews @maleck13 @david-martin @wtrocki

wtrocki · 2018-02-19T17:48:30Z

@darahayes Verified that locally. It works for me.
Going to check that once logic will be added.

coveralls · 2018-02-19T17:52:17Z

Pull Request Test Coverage Report for Build #46

35 of 40 (87.5%) changed or added relevant lines in 5 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+50.8%) to 87.603%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/mobile/metrics.go	4	6	66.67%
pkg/web/healthzHandler.go	14	17	82.35%

Files with Coverage Reduction	New Missed Lines	%
pkg/web/router.go	4	77.78%

Totals
Change from base Build #30:	50.8%
Covered Lines:	106
Relevant Lines:	121

💛 - Coveralls

paolobueno · 2018-02-19T18:38:22Z

pkg/dao/db.go

+		return err
+	}
+	return nil
+}


Might be worth looking into https://github.com/mattes/migrate for these

Indeed this is something I mentioned on the previous call. Really this PR is still under heavy development. :)

paolobueno · 2018-02-19T19:11:01Z

@darahayes I'm basing off of this to just finish exposing the Ping() on the healthz endpoint as a softer introduction to golang 👍

aliok · 2018-02-20T09:17:20Z

pkg/dao/db.go

+	}
+	if _, err := db.Exec("CREATE TABLE IF NOT EXISTS sdkVersionForClient(clientId varchar(10) not null primary key, version varchar(40) not null, event_time timestamp with time zone)"); err != nil {
+		return err
+	}


@darahayes @wtrocki I actually found out a much better way for the knowledge base.

Now that we're using Postgres, much more complex aggregations are possible. ElasticSearch had limitations...

We can create a view that contains the latest entries for the "mobileAppMetrics" table (created below) and use that in Grafana.

No need for 2nd insertion/update operation nor a separate table.

But we need to create a view:

create view clientKnowledgeBase as select entry.clientId, entry.event_time, entry.data from mobileappmetrics entry inner join (select clientId, max(event_time) as latestEntryTime from mobileappmetrics group by clientId) latestEntry on entry.clientId = latestEntry.clientId and entry.event_time = latestEntryTime;

Doing select * from clientKnowledgeBase would return the data de-duplicated per client id.

This, however, means that we should use this table only for our metrics, not custom metrics as we would want this view to have entries only for the metrics that needs de-duplication.

I gave this one a try on the POC but haven't checked in anything.

@aliok Sounds really nice. Perhaps you should push it to a branch so we can look at it!

aliok · 2018-02-20T12:11:25Z

Note that http endpoint is already implemented part of this task and #14 adds tests for it. #14 is blocked by this PR.

wtrocki · 2018-02-21T12:31:57Z

@darahayes @aliok PR is good to go from my point view.

darahayes · 2018-02-21T13:28:47Z

This PR is still in progress. Just finishing out some tests for the business logic layer and perhaps db integration tests

…

On 21 Feb 2018 12:40 p.m., "Wojciech Trocki" ***@***.***> wrote: @darahayes <https://github.com/darahayes> @aliok <https://github.com/aliok> PR is good to go from my point view. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIQVPCC60GnRmepd2cA_0M58ednmY_luks5tXAy_gaJpZM4SK0Wc> .

* AEROGEAR-2044 - test metrics endpoint * AEROGEAR-2044 - use a setup func for creating the server * AEROGEAR-2044 - use Testify for mocking * AEROGEAR-2044 - add test case when there is an impl error in MetricsService (http 500) * AEROGEAR-2044 - use Testify's assertions * AEROGEAR-2044 - add Testify dep to Gopkg * AEROGEAR-2044 - fix tests after rebase * fix: move serialization of metrics data into mobile layer * AEROGEAR-2044 - use built-in errors * Add ping endpoint implementation and tests with mocking * Use `boom` instead of `http.Error` * Remove generated mock * Remove extra ping method * Use testify * Update gopkg

wtrocki · 2018-02-22T17:07:15Z

pkg/dao/db.go

-func Connect() (*sql.DB, error) {
-	if db != nil {
-		return db, nil
+// var db *sql.DB


Get that guy out :P

wtrocki · 2018-02-22T17:13:12Z

pkg/dao/dao.go

-func (m *MetricsDAO) Update() {
-
+func (m *MetricsDAO) Create(clientId string, metricsData []byte) error {
+	_, err := m.db.Exec("INSERT INTO mobileappmetrics(clientId, data) VALUES($1, $2)", clientId, metricsData)


[Optional] mobileappmetrics table is used in various SQL calls. DAO here works fine, but maybe it can also handle the actual table creation logic (to fully extract that there is some SQL/postgre involved) Alternative will be to just use constant for mobileappmetrics table name. It's not big deal, since we already have abstraction for SQL queries, but creation of the table seems to be leaking that we using sql.db interface.

+1 I think a constant/config item for the database table is the best option

maleck13 · 2018-02-22T17:13:39Z

pkg/config/config_test.go

+
+	config := GetConfig()
+
+	fmt.Println(config)


nit keep tests quiet

Thanks! It's been a long day!

maleck13 · 2018-02-22T17:13:53Z

pkg/config/config_test.go

+
+	config := GetConfig()
+
+	fmt.Println(config)


same as above

maleck13 · 2018-02-22T17:15:48Z

pkg/dao/dao_test.go

+	}
+}
+
+func TestIshealthyWhenDisconnected(t *testing.T) {


TestIshealthyWhenDisconnected -> TestIsHealthyWhenDisconnected

maleck13 · 2018-02-22T17:16:35Z

pkg/dao/dao_test.go

+	dbHandler := DatabaseHandler{}
+
+	err := dbHandler.Connect(config.DBHost, config.DBUser, config.DBPassword, config.DBName, config.SSLMode)
+	dbHandler.DoInitialSetup()


should this be after err check

maleck13 · 2018-02-22T17:20:48Z

pkg/dao/db.go

 	}
 	//connection logic
-	return nil, nil
+	connStr := fmt.Sprintf("host=%v user=%v password=%v dbname=%v sslmode=%v", dbHost, dbUser, dbPassword, dbName, sslMode)


may want to consider looking into setting values for

db.SetMaxOpenConns() db.SetConnMaxLifetime()

as otherwise you can end up with invalid connections. Have a read of what they do and figure out what makes sense for you

+1 I was reading up on them yesterday but I totally forgot about it today!

maleck13 · 2018-02-22T17:21:38Z

pkg/dao/db.go

+		return err
+	}
+
+	// assign db variable declared above


not sure we need that level of docs on the code 😄

This is actually an outdated comment from when the db was declared at a package level. Good catch!

david-martin · 2018-02-22T22:36:01Z

@darahayes I'll give this a spin locally to understand it better, if you don't mind the wait for merge approval

darahayes · 2018-02-22T23:18:03Z

@david-martin no problem at all. The docs may need some work so ping me if you have any questions

maleck13 · 2018-02-23T09:33:27Z

pkg/config/config_test.go

+	}
+}
+
+func TestGetConfigCustomEnvVariables(t *testing.T) {


I would suggest looking into table tests. These are heavily used in go. Here is an example https://github.com/aerogear/mobile-cli/blob/master/pkg/cmd/clients_test.go#L37

maleck13 · 2018-02-23T09:47:51Z

pkg/web/metricsHandler_test.go

+	mock "github.com/stretchr/testify/mock"
+)
+
+type MockMetricsService struct {


general question, should there be a mock here. Could we inject just a mock dao into the real service here giving us something closer to an e2e test that can run without the db

I would agree with you on this. I think the kind of testing you're suggesting would give us a much greater ROI. However I'm conscious that this code was already reviewed and merged as part of #14. I'm hoping that this PR doesn't grow any larger than it already is.

I would like to see a new issue/ticket created for e2e style testing. The original goal of this ticket was just the database parts but then two other PRs ended up being merged into this branch. (lesson learned - let's avoid doing that again)

wtrocki

LGTM.
This PR consist of the multiple different PR's that were reviewed. I have reviewed and tested that myself. IMHO It's important to merge that and do verifications with Android and IOS implementations

david-martin

Approving this, with the knowledge there may be fixes/changes as part of integration tests with the sdks as covered by https://issues.jboss.org/browse/AGIOS-654 & https://issues.jboss.org/browse/AGDROID-735

psturc · 2018-02-26T09:50:45Z

@darahayes I've noticed some failures being present in the console output when I run integration tests locally. It's also in the log of circle ci build (make test-integration-cover part). For some reason, it has exit code 0. Is this expected?

darahayes · 2018-02-26T11:01:38Z

@psturc Thanks so much for noticing this. The reason for the failing tests is simple. Now the reason why the exit code was still 0 is a little more unclear. Will fix this very soon.

wtrocki requested review from wtrocki and aliok February 19, 2018 17:48

paolobueno reviewed Feb 19, 2018

View reviewed changes

aliok reviewed Feb 20, 2018

View reviewed changes

aliok mentioned this pull request Feb 20, 2018

AEROGEAR-2044 - HTTP Endpoint #14

Merged

Dara Hayes added 6 commits February 20, 2018 17:08

feat: added initial db connection and setup logic

8b1580b

feat: initial parsing of client payload into JSON

471ed8b

fix: use JSONB column for mobile app metrics data

96cb9c1

feat: initial implementation of Create() in MetricsDAO

c045758

fix: rename metrics endpoint to /metrics

dab3e1b

fix: update docker-compose file

f6ff2fa

darahayes force-pushed the db-implementation branch from 512e8c4 to f6ff2fa Compare February 20, 2018 17:08

paolobueno mentioned this pull request Feb 20, 2018

Add ping endpoint implementation and tests with mocking #17

Merged

fix: move serialization of metrics data into mobile layer

687a127

darahayes force-pushed the db-implementation branch from 1bdb37c to 687a127 Compare February 22, 2018 09:21

paolobueno and others added 9 commits February 22, 2018 09:24

feat: basic unit tests for mobile package

1248bca

fix: move config parsing into separate package

a783200

fix: get port number from environment variable

8007a6e

fix: remove timescale extension

b0c2ebf

fix: wrong environment variable name in config

21cef30

feat: wrap db instance in handler struct, add integration tests

a2d1003

fix: fix test commands in makefile

c0a094d

feat: run integration tests in CircleCI build

62ba301

Dara Hayes added 2 commits February 22, 2018 16:07

fix: add make integration command that merges coverage reports

0fa5084

fix: refactor config package to return map, add tests

6fb382e

wtrocki changed the title ~~[WIP] Database Integration~~ Database Integration Feb 22, 2018

wtrocki reviewed Feb 22, 2018

View reviewed changes

pkg/dao/db.go Outdated

func Connect() (*sql.DB, error) {

if db != nil {

return db, nil

// var db *sql.DB

Copy link

Contributor

wtrocki Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get that guy out :P

fix: remove comment

cf3c314

wtrocki reviewed Feb 22, 2018

View reviewed changes

maleck13 reviewed Feb 22, 2018

View reviewed changes

pkg/config/config_test.go Outdated

config := GetConfig()

fmt.Println(config)

Copy link

Contributor

maleck13 Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

fix: remove print statements

0f2f3bf

maleck13 reviewed Feb 22, 2018

View reviewed changes

Dara Hayes added 2 commits February 22, 2018 17:18

fix: minor fixes to tests

9b0b49b

fix: check error condition in dao_test.go

ea454e4

maleck13 reviewed Feb 22, 2018

View reviewed changes

fix: remove comments

0565639

maleck13 reviewed Feb 23, 2018

View reviewed changes

david-martin self-assigned this Feb 23, 2018

wtrocki approved these changes Feb 23, 2018

View reviewed changes

david-martin approved these changes Feb 23, 2018

View reviewed changes

david-martin merged commit bc1ee5d into master Feb 23, 2018

david-martin deleted the db-implementation branch February 23, 2018 11:15

paolobueno mentioned this pull request Feb 23, 2018

Aerogear 2047 add app metrics to metrics apb aerogearcatalog/metrics-apb#28

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Database Integration #10

Database Integration #10

darahayes commented Feb 19, 2018 •

edited

Loading

wtrocki commented Feb 19, 2018 •

edited

Loading

coveralls commented Feb 19, 2018 •

edited

Loading

paolobueno Feb 19, 2018

darahayes Feb 20, 2018

paolobueno commented Feb 19, 2018

aliok Feb 20, 2018 •

edited

Loading

darahayes Feb 20, 2018

aliok commented Feb 20, 2018

wtrocki commented Feb 21, 2018 •

edited

Loading

darahayes commented Feb 21, 2018 via email

wtrocki Feb 22, 2018

wtrocki Feb 22, 2018

darahayes Feb 22, 2018

maleck13 Feb 22, 2018

darahayes Feb 22, 2018

maleck13 Feb 22, 2018

maleck13 Feb 22, 2018

maleck13 Feb 22, 2018

maleck13 Feb 22, 2018

darahayes Feb 22, 2018

maleck13 Feb 22, 2018

darahayes Feb 22, 2018

david-martin commented Feb 22, 2018

darahayes commented Feb 22, 2018

maleck13 Feb 23, 2018

maleck13 Feb 23, 2018

darahayes Feb 23, 2018

wtrocki left a comment

david-martin left a comment

psturc commented Feb 26, 2018

darahayes commented Feb 26, 2018

Database Integration #10

Database Integration #10

Conversation

darahayes commented Feb 19, 2018 • edited Loading

Update

How to verify

wtrocki commented Feb 19, 2018 • edited Loading

coveralls commented Feb 19, 2018 • edited Loading

Pull Request Test Coverage Report for Build #46

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paolobueno commented Feb 19, 2018

aliok Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliok commented Feb 20, 2018

wtrocki commented Feb 21, 2018 • edited Loading

darahayes commented Feb 21, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-martin commented Feb 22, 2018

darahayes commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki left a comment

Choose a reason for hiding this comment

david-martin left a comment

Choose a reason for hiding this comment

psturc commented Feb 26, 2018

darahayes commented Feb 26, 2018

darahayes commented Feb 19, 2018 •

edited

Loading

wtrocki commented Feb 19, 2018 •

edited

Loading

coveralls commented Feb 19, 2018 •

edited

Loading

aliok Feb 20, 2018 •

edited

Loading

wtrocki commented Feb 21, 2018 •

edited

Loading