[#10929] Migrate Task Queue API and Logs API to GCP clients #10930

wkurniawan07 · 2021-01-23T12:14:17Z

Note: don't be alarmed by the amount of code addition. There is an explanation for that.

Changed Task Queue API usage to Cloud Tasks API.
- In dev server, obviously Cloud Tasks will not trigger. It is replaced by immediate execution, which can be disabled if not desired.
Changed Logs API usage to Cloud Logging API.
- Changing the API to be used is straightforward; the challenge comes in parsing the output.
- In order to parse the GAE request logs obtained from the Stackdriver, we need the request format which is documented in https://cloud.google.com/logging/docs/reference/v2/rpc/google.appengine.logging.v1#google.appengine.logging.v1.RequestLog. However, its Java format is not available in any currently published GCP client libraries.
- Thus I manually generated the Java format from the protobuf in https://github.com/googleapis/googleapis/blob/master/google/appengine/logging/v1/request_log.proto; that explains the > 9000 (coincidence?) line addition in one of the commits, which can be fully ignored.
- There may be better ways such as generating the protobuf as part of script, but right now this is the minimally viable setup.

In addition, one thing that this PR shows is that separating the services between dev server and staging/production server will be more of a challenge in the future (GAE really did us a favor in this aspect).

madanalogy · 2021-01-24T15:18:10Z

Actually, why not use cloud logging v2 instead?

However, its Java format is not available in any currently published GCP client libraries

It's not the ListLogsRequest class here? https://googleapis.dev/java/google-cloud-logging/latest/index.html

I'm referencing https://cloud.google.com/logging/docs/reference/libraries for the java client library

wkurniawan07 · 2021-01-24T15:47:07Z

That's the request object for the protobuf client and has nothing to do here. The log format that we need is very specifically this: https://cloud.google.com/logging/docs/reference/v2/rpc/google.appengine.logging.v1#google.appengine.logging.v1.RequestLog.

madanalogy · 2021-02-08T14:02:04Z

src/main/java/teammates/ui/webapi/CompileLogsAction.java

-        LogService logService = LogServiceFactory.getLogService();
+    private List<LogEntry> getErrorLogs() {
+        if (Config.isDevServer()) {
+            // Not supported in dev server


Maybe just print out a message to console or something equivalent here? So that developers know logs are not supported in dev environment rather than having to hunt down the code here

I don't think printing to console would help. This API is not deliberately used by anyone in production system (cron job doesn't really count as any"one", does it?), and if someone really wants to test this, chances are s/he already knows that this is a GCP-specific feature.

madanalogy · 2021-02-08T14:03:23Z

src/main/resources/build.template.properties

+# This indicates whether task queues are active (e.g. items added to task queue will be queued for execution).
+# This flag is only used during development mode; in production, task queue will always be active.
+# In addition, during development mode, there is no "queueing", i.e. all tasks will be immediately executed.
+app.taskqueue.active=true


@damithc once this goes live will need to add lines to build.properties, just fyi for deployment of v7.10

No: This flag is only used during development mode

What about app.region?

app.region is the other way around: necessary in staging/production but ignored in dev server

Ah okay so that's the one that will need to be added then haha

rrtheonlyone

LGTM 💯

Really nice work parsing the logs using the protobuf - tested on staging environment and works well.

I tried to dig and see if there is a way to do it without generating the bindings by hand but could not find anything. Others seemed to have done the same: https://stackoverflow.com/a/59414384/13750147

rrtheonlyone · 2021-02-14T05:01:49Z

src/main/java/teammates/ui/webapi/CompileLogsAction.java

-                LogLevel logLevel = currentLog.getLogLevel();
+                logLines = reconvertedLog.getLineList();
+            } catch (InvalidProtocolBufferException e) {
+                // TODO


What are we planning on doing here?

@wkurniawan07 did you intend for this to be done in this PR or separately (i.e. needs some discussion)?

I'm not sure myself what best to do here. If we do a error-level log here, then this log will be part of the next logs compilation, which will also fail because of the same reason. At best we can do a warn-level log here, but let's be real: that's as good as no logs.

Yea I think this will need some discussion and possibly research, maybe settle another time with another PR/issue. We go ahead with what we have for now then

madanalogy · 2021-02-21T05:59:31Z

src/main/resources/build.template.properties

+# This is the deployment region of the app.
+# For dev server, this could be anything.
+# For staging/production server, this must match the region of your GAE application.
+app.region=us-central1


@damithc I forgot to bring your attention to changes needed in the src/main/resources/build.properties file with the release of V7.10.0 which might be the reason for errors in #10974

I see. How do I find the region used for the production version.

@damithc you can type gcloud app describe --project <project-name> (replace <project-name with the project in GAE)

You should see something like this:

authDomain: gmail.com codeBucket: staging.myapp-1337.appspot.com defaultBucket: myapp-1337.appspot.com defaultHostname: myapp-1337.appspot.com featureSettings: splitHealthChecks: true gcrDomain: us.gcr.io id: myapp-1337 locationId: us-central name: apps/myapp-1337 servingStatus: SERVING

For teammates, we should be us-central - so region should be us-central1 (the docs mention that for this region we have to map the name slightly differently):

Note: Two locations, which are called europe-west and us-central in App Engine commands and in the Google Cloud Console, are called europe-west1 and us-central1, respectively, elsewhere in Google documentation.

Got it. Thanks both. Will update and deploy again soon.

wkurniawan07 added the s.ToReview The PR is waiting for review(s) label Jan 23, 2021

wkurniawan07 requested review from madanalogy and rrtheonlyone January 23, 2021 12:32

wkurniawan07 force-pushed the runtime-upgrade/taskqueue-logging branch from a1bad7e to 51921f6 Compare January 23, 2021 14:33

wkurniawan07 mentioned this pull request Feb 1, 2021

[#10944] Migrate Blobstore API to Google Cloud Storage #10945

Merged

rrtheonlyone self-assigned this Feb 2, 2021

wkurniawan07 added 5 commits February 8, 2021 01:42

Change taskqueue API to Cloud Tasks API

8966774

Add option to execute task queues immediately in dev server

0110ea9

Use Cloud Logging API to read logs

0b5c20c

Add generated protobuf files for RequestLog

fd42516

Enable request logs to be parsed and processed

f09a08b

wkurniawan07 force-pushed the runtime-upgrade/taskqueue-logging branch from 8a85f3b to f09a08b Compare February 7, 2021 17:43

madanalogy reviewed Feb 8, 2021

View reviewed changes

madanalogy added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Feb 8, 2021

madanalogy self-assigned this Feb 8, 2021

madanalogy added this to the V7.10.0 milestone Feb 8, 2021

madanalogy approved these changes Feb 11, 2021

View reviewed changes

madanalogy requested review from rrtheonlyone and removed request for rrtheonlyone February 11, 2021 02:11

Merge branch 'master' into runtime-upgrade/taskqueue-logging

2851cc6

rrtheonlyone approved these changes Feb 14, 2021

View reviewed changes

madanalogy mentioned this pull request Feb 15, 2021

Release V7.10.0 #10963

Closed

4 tasks

madanalogy added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Feb 15, 2021

Merge branch 'master' into runtime-upgrade/taskqueue-logging

d8eccb8

madanalogy added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Feb 15, 2021

madanalogy merged commit d32428b into TEAMMATES:master Feb 15, 2021

wkurniawan07 deleted the runtime-upgrade/taskqueue-logging branch February 15, 2021 15:07

madanalogy reviewed Feb 21, 2021

View reviewed changes

madanalogy mentioned this pull request Feb 21, 2021

NPE from scheduleFeedbackSessionPublishedEmail #10974

Closed

madanalogy mentioned this pull request Mar 2, 2021

[#10950] Add logging functionality to backend #10984

Merged

wkurniawan07 mentioned this pull request Apr 11, 2022

[#11725] [#11744] Update logs compilation function + version filter #11726

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#10929] Migrate Task Queue API and Logs API to GCP clients #10930

[#10929] Migrate Task Queue API and Logs API to GCP clients #10930

wkurniawan07 commented Jan 23, 2021

madanalogy commented Jan 24, 2021

wkurniawan07 commented Jan 24, 2021

madanalogy Feb 8, 2021

wkurniawan07 Feb 10, 2021 •

edited

Loading

madanalogy Feb 8, 2021

wkurniawan07 Feb 8, 2021

madanalogy Feb 8, 2021

wkurniawan07 Feb 8, 2021

madanalogy Feb 8, 2021

rrtheonlyone left a comment •

edited

Loading

rrtheonlyone Feb 14, 2021

madanalogy Feb 15, 2021

wkurniawan07 Feb 15, 2021

madanalogy Feb 15, 2021

madanalogy Feb 21, 2021

damithc Feb 21, 2021

rrtheonlyone Feb 21, 2021

damithc Feb 22, 2021

[#10929] Migrate Task Queue API and Logs API to GCP clients #10930

[#10929] Migrate Task Queue API and Logs API to GCP clients #10930

Conversation

wkurniawan07 commented Jan 23, 2021

madanalogy commented Jan 24, 2021

wkurniawan07 commented Jan 24, 2021

Choose a reason for hiding this comment

wkurniawan07 Feb 10, 2021 • edited Loading

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

rrtheonlyone left a comment • edited Loading

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

wkurniawan07 Feb 10, 2021 •

edited

Loading

rrtheonlyone left a comment •

edited

Loading