Update the app for GAE Go 1.12+ runtime #37

Temikus · 2020-08-30T23:43:57Z

Since bbolt which we plan to use as a storage engine requires Go 1.13+, we need to update the app to support GAE 1.12+ runtime (which we would've inevitably needed to do anyway).

Rough outline of changes here:

Moving to go modules
Changing the runtime to 1.13
Migrate logging to logrus
Remove urlfetch, appengine.Main() and other GAE library-specific routines
Get runtime info from ENV instead of using appengine lib
Change some config paths and associated docs due to different PWD

NOTE TO REVIEWER:
I know this is a large PR, but the migration scope is pretty large so I want to avoid merging commits that will make the project unbuildable. As such, I've partitioned the PR into small commits that should be limited in scope and easier to review. I plan to squash them after the review is finished.

- This is needed if we're dealing with storage engines that need additional operations to be performed (e.g. DB to be closed) - Additionally, rename newConfig to newRuntimeConfig to make it less likely to confuse it with tsbridge.NewConfig

Currently this is just a no-op but is needed to support storage engines that need to close their DB's

app/main.go

datastore/datastore_manager.go

datastore/emulator.go

.travis.yml

app/main.go

datadog/metric_test.go

datastore/emulator.go

1.12+ runtime requires the app to use go modules - Remove dep manifest, lock and vendor folder

- Add a .go-version file to use with goenv

AppEngine logging lib is no longer supported in 1.12+ runtimes

1.12+ doesn't support urlfetch

app/main.go

datadog/metric_test.go

dparrish · 2020-09-07T03:34:29Z

datadog/metric_test.go

 	code := m.Run()
-	done()
+	testCtxCancel()


The shutdown code in the emulator probably won't be run here, there's a race condition with os.Exit(), the next line.

I'll have a chat with you about how to fix this outside this review.

@dparrish So as discussed I've decided to address this via returning a channel that gets pushed a message into it when it's done - PTAL

datadog/metric_test.go

datastore/datastore_manager.go

datastore/datastore_manager_test.go

dparrish · 2020-09-07T05:21:28Z

datastore/emulator.go

+func Emulator(ctx context.Context) error {
+
+	// Set log formatter to process newlines since emulator gives a lot of output
+	log.SetFormatter(&log.TextFormatter{DisableQuote: true})


This changes the global logging state, probably not what you want. logrus can create child loggers with different formats which might be good here.

or move this SetFormatter line to main().

Good point. I dug quite a bit through the docs, but couldn't find specifics on child loggers, can you point at an example somewhere?

https://godoc.org/github.com/sirupsen/logrus#New

Fixed it via log.WithContext() PTAL

Since we can no longer use AppEngine SDK for it

Working dir is now module root so we need to change some things accordingly

In favour of generic serving, as suggested by the migration guide

- Match the go version with the .go-version - Drop godep - Update CloudSDK

datadog/metric.go

datadog/metric_test.go

stackdriver/adapter_test.go

tsbridge/config.go

tsbridge/config_test.go

dparrish · 2020-09-10T05:38:04Z

tsbridge/config_test.go

+	setProjectID(fakeProjectID)
+	_, errNoProject := NewConfig(ctx, &ConfigOptions{Filename: "testdata/valid.yaml", Storage: storage})
+
+	if errNoProject == nil {


errNoProject really should just be err for consistency.

Question - otherwise I'll get the No new variables on left side of := - is it ok to use = to redefine err?

If you use the single-line form then you can redefine err! e.g. this is valid:

var err error if err := Operation(); err != nil { ... }

tsbridge/metric_test.go

Temikus

Fixed most of it - just 2 comments outstanding - PTAL.

Temikus · 2020-09-11T01:00:57Z

datadog/metric_test.go

 	if err == nil {
 		t.Error("expected an error when server returns 404")
 	}

 	// A query needs to return a single time series.
 	handler.filename = "multiple_ts.json"
-	_, _, err = m.StackdriverData(testCtx, time.Now().Add(-time.Minute), &datastore.StoredMetricRecord{})
+	_, _, err = m.StackdriverData(ctx, time.Now().Add(-time.Minute), &datastore.StoredMetricRecord{Storage: storage})


So genuine question here - if I do that - the function legit doesn't fit in my 27inch monitor - is it ok to leave it like this for readability sake?

datadog/metric.go

datastore/datastore_record.go

datastore/datastore_record_test.go

datastore/emulator.go

influxdb/metric_test.go

stackdriver/adapter_test.go

datastore/emulator.go

Addressed all changes.

- Migrating to use cloud datastore libraries - Add a client field to Datastore.manager and hold the client session info in it - Add a datastore emulator harness (using `gcloud`) for tests as `gaetest` is no longer supported past Go 1.11 - Update tests & mocks fixup

Temikus added 3 commits August 24, 2020 17:01

Add storageManager mocks

2c1a3ed

Refactor main.go to load storage separately

00133ae

- This is needed if we're dealing with storage engines that need additional operations to be performed (e.g. DB to be closed) - Additionally, rename newConfig to newRuntimeConfig to make it less likely to confuse it with tsbridge.NewConfig

Add Close() to StorageManager interface

f58a72d

Currently this is just a no-op but is needed to support storage engines that need to close their DB's

Temikus requested review from Dnefedkin and knyar August 30, 2020 23:43

Temikus force-pushed the go_112_plus branch 8 times, most recently from bd049f2 to c99d093 Compare September 1, 2020 07:02

Dnefedkin reviewed Sep 1, 2020

View reviewed changes

app/main.go Show resolved Hide resolved

Dnefedkin reviewed Sep 1, 2020

View reviewed changes

datastore/datastore_manager.go Outdated Show resolved Hide resolved

datastore/emulator.go Outdated Show resolved Hide resolved

.travis.yml Show resolved Hide resolved

knyar previously requested changes Sep 3, 2020

View reviewed changes

app/main.go Outdated Show resolved Hide resolved

app/main.go Outdated Show resolved Hide resolved

datadog/metric_test.go Outdated Show resolved Hide resolved

datastore/emulator.go Outdated Show resolved Hide resolved

Temikus added 4 commits September 7, 2020 10:16

1.12+ update - move to go modules

1f36bb6

1.12+ runtime requires the app to use go modules - Remove dep manifest, lock and vendor folder

1.12+ update - change runtime to 1.13

53db99d

- Add a .go-version file to use with goenv

1.12+ update - migrate logging to logrus

f0c6b3d

AppEngine logging lib is no longer supported in 1.12+ runtimes

1.12+ update - remove urlfetch

c3656fe

1.12+ doesn't support urlfetch

Temikus force-pushed the go_112_plus branch 2 times, most recently from d959c3a to 9b3b38b Compare September 7, 2020 00:45

dparrish suggested changes Sep 7, 2020

View reviewed changes

Temikus force-pushed the go_112_plus branch 2 times, most recently from a599b0f to b97e6c3 Compare September 7, 2020 08:04

Temikus added 5 commits September 8, 2020 16:00

1.12+ update - Get runtime info from the environment

edb8006

Since we can no longer use AppEngine SDK for it

1.12+ update - changing paths due to different working dir

dde9332

Working dir is now module root so we need to change some things accordingly

1.12+ update - Remove appengine.Main() routine

11a2b84

In favour of generic serving, as suggested by the migration guide

1.12+ update - Remove appengine libs from Main, switch to context

e9965bd

1.12+ update - update Travis for go modules

f2a6fc3

- Match the go version with the .go-version - Drop godep - Update CloudSDK

Temikus force-pushed the go_112_plus branch 2 times, most recently from 38523ea to 50bec22 Compare September 9, 2020 06:31

dparrish suggested changes Sep 10, 2020

View reviewed changes

Temikus force-pushed the go_112_plus branch from 50bec22 to fc833a2 Compare September 11, 2020 01:53

Temikus commented Sep 11, 2020

View reviewed changes

Temikus force-pushed the go_112_plus branch from fc833a2 to 709acff Compare September 11, 2020 02:01

dparrish approved these changes Sep 11, 2020

View reviewed changes

datastore/emulator.go Outdated Show resolved Hide resolved

Temikus force-pushed the go_112_plus branch from 709acff to ee76673 Compare September 11, 2020 04:33

Temikus changed the title ~~[WORK IN PROGRESS] Update the app for GAE Go 1.12+ runtime~~ Update the app for GAE Go 1.12+ runtime Sep 11, 2020

Temikus merged commit 15c8d4d into google:master Sep 11, 2020

Temikus deleted the go_112_plus branch September 11, 2020 05:44

Dnefedkin mentioned this pull request Sep 14, 2020

Containerize ts-bridge, remove GAE dependencies, and make it deployable in K8S #35

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the app for GAE Go 1.12+ runtime #37

Update the app for GAE Go 1.12+ runtime #37

Temikus commented Aug 30, 2020

dparrish Sep 7, 2020

Temikus Sep 7, 2020

dparrish Sep 7, 2020

Temikus Sep 10, 2020

dparrish Sep 10, 2020

Temikus Sep 11, 2020

dparrish Sep 10, 2020

Temikus Sep 11, 2020

dparrish Sep 11, 2020

Temikus left a comment

Temikus Sep 11, 2020

Update the app for GAE Go 1.12+ runtime #37

Update the app for GAE Go 1.12+ runtime #37

Conversation

Temikus commented Aug 30, 2020

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

Temikus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment