Logging in Watchbot4 #205

tapaswenipathak · 2018-06-05T09:08:38Z

Closes #204.

cc @mapbox/platform-engine-room.

* adds logging of watcher-level errors, worker receives, and completion status * prefixed logs from child processes

Move binary-split from dev to dependency

* change scale-down MetricIntervalLowerBound to MetricIntervalUpperBound

jakepruitt · 2018-06-05T17:25:52Z

lib/logger.js

@@ -31,11 +31,11 @@ class Logger {
  }

  workerFailure(results) {
-    this.log(`[failure] ${JSON.stringify(results)}`);
+    this.log(`[worker] [failure] ${JSON.stringify(results)}`);


These logs are not coming from the internals of the worker, but are instead coming from the watchbot listen process. Should this be prefixed with [watchbot] instead?

I see workerFailure is called from

ecs-watchbot/lib/worker.js

Line 51 in 7c5b226

this.logger.workerFailure(results);

do you also suggest changing the method names as watchbotFailure()?

Other than this I think it is necessary to prefix [worker] here

ecs-watchbot/lib/worker.js

Lines 19 to 20 in 7c5b226

child.stdout.pipe(logger.stream());

child.stderr.pipe(logger.stream());

Right, it's necessary to prefix [worker] on those streamed logs since they are actually coming from the worker child process, while the message with the results JSON is coming from the code inside of watchbot. I think the function should still be called workerFailure, but since this log is taking place inside of the watchbot code and not inside of the code defined by the user in the subprocess, the log should be prefixed with [watchbot].

* Add alarms and alarm docs * Add failedPlacementAlarmPeriods * Add CloudWatch Alarms snapshots * Update template jest snapshots * Add CloudWatch Alarms snapshots * Add failedworker and failedworkerplacement metric * Typo r/LogGroup/Logs * Change metric name * Metric Filter of worker errors to "[failure]" * Have current published version instead of undefined * Jake's Review * uh update-jest * Update alarms.md

…into logging

* Add travis user * Ensure this fails * Add validation for notificationEmail or notificationTopic

…queue threshold, info to doc (#211) * Closes #208, #207, #206, #182, #149, #72, #15 (cherry picked from commit 8de328df79ccf52b8d612c625891555808c2fa0e) * Add minSize as option * update jest tests * Change MinSize to 0 * update jest * identation and minSize to 0 * Add deadletterThreshold info in Worker-retry-cycle

…into logging

jakepruitt · 2018-06-18T21:08:12Z

Seems like this PR needs to be rebased on master. @tapasweni-pathak let us know when you're ready for this to get a final review!

tapaswenipathak · 2018-06-19T07:28:39Z

@jakepruitt The Travis is failing, can you run in your local and suggest what is missing in the tests? Plus I don't think I would be rebasing this one, I would just create a new one.

jakepruitt · 2018-06-19T19:08:50Z

Hmmm @tapasweni-pathak I was able to fix the test locally, but it looks like I can't replicate the ECONNRESET error that's showing up in travis 😕 . This may be a case of strange things happening due to the commit history of this branch - I'd start by resolving that and see if the problem persists.

tapaswenipathak · 2018-06-20T05:32:38Z

@jakepruitt I added an empty commit for ECONNRESET error, I think usually this error goes away with an empty commit or with just a restart Travis build. Travis errors with Error: write EPIPE as child.stdout.write has writable:false, did the test pass locally with 285710d? I edited this in some commit the master has [worker] on line 154. child.stdin.write should be used to write to subprocess. I get better errors with it, #225.

jakepruitt · 2018-06-20T21:10:21Z

@tapasweni-pathak yeah, the EPIPE was passing for me locally. Could you set this up on a new branch/rebased branch and see if the error persists? I think we should address it in a fresh environment that we know is up to date with master, since it's tough to know the state of the files in this branch.

tapaswenipathak · 2018-06-21T12:28:06Z

@jakepruitt yeah error persists, I already created a separate PR #225 before checking again if the tests worked locally for you. Tests didn't work locally or on Travis for me. child.stdout.write and child.stdout.stderr has writable property as false and think the error would appear. Could you suggest why Travis is failing with Error: write EPIPE on Travis #225?

jakepruitt · 2018-06-25T19:43:43Z

Could we close this PR to avoid confusion with #225?

jakepruitt · 2018-06-28T19:52:43Z

Closing in favor of #225

rclark and others added 11 commits February 8, 2018 09:39

♻ that container

1625b22

Add logging (#185)

50d0c22

* adds logging of watcher-level errors, worker receives, and completion status * prefixed logs from child processes

fixes logger factory to accept a message

8b7307e

--> false for legibility

c5e9e78

move binary split to dependency

050d764

package lock changes

299f506

Merge pull request #186 from mapbox/binary-split-dep

7c5b226

Move binary-split from dev to dependency

Scale down threshold (#187)

a4f61be

* change scale-down MetricIntervalLowerBound to MetricIntervalUpperBound

Add [worker] in logs wherever applicable:

f5eaa1c

Add enhanced logging and filter pattern

a356132

cleaup

c369ffe

tapaswenipathak force-pushed the logging branch from 4e97508 to c369ffe Compare June 5, 2018 09:39

jakepruitt reviewed Jun 5, 2018

View reviewed changes

tapaswenipathak and others added 3 commits June 7, 2018 12:16

prefix [watchbot] in workerfailure and workerError methods logs

a056e67

Update [watchbot] in workerFailure and workerError method logs

bb7fc75

tapaswenipathak force-pushed the logging branch 3 times, most recently from 572eb90 to b48ba7e Compare June 9, 2018 07:47

tapaswenipathak and others added 2 commits June 9, 2018 13:34

hello epipe error and add more tests

4851c46

tapaswenipathak force-pushed the logging branch 3 times, most recently from 8ce7c9f to 566c4e8 Compare June 9, 2018 09:03

does travis error?

a45334c

tapaswenipathak force-pushed the logging branch from 538a221 to 9909e73 Compare June 9, 2018 09:11

log epipe

60ec163

tapaswenipathak force-pushed the logging branch 2 times, most recently from 427a8ca to 1c1b28d Compare June 9, 2018 09:23

travis and local error are different yay

bdbceaf

tapaswenipathak force-pushed the logging branch from a4b0960 to 10c649e Compare June 9, 2018 13:52

tapaswenipathak added 2 commits June 9, 2018 19:58

add more tests bump for logging demolition-derby branch

7d5d7dd

Merge branch 'container-recycling' of github.com:mapbox/ecs-watchbot …

6ef8860

…into logging

tapaswenipathak force-pushed the logging branch from 10c649e to 6ef8860 Compare June 9, 2018 14:29

Jake Pruitt and others added 6 commits June 11, 2018 16:31

Add template validation tests (#215)

3ac7786

* Add travis user * Ensure this fails * Add validation for notificationEmail or notificationTopic

Update tests with maxSize property

63cba8b

Merge branch 'container-recycling' of github.com:mapbox/ecs-watchbot …

6b33dec

…into logging

does it print?

9f5bf0e

epipe error in local

a83eb4c

tapaswenipathak force-pushed the logging branch from f0ce08a to a83eb4c Compare June 13, 2018 12:25

jakepruitt force-pushed the container-recycling branch from 3a196da to 1892852 Compare June 16, 2018 00:48

tapaswenipathak changed the base branch from container-recycling to master June 18, 2018 04:35

Fix logging test

285710d

empty commit

7d99709

tapaswenipathak mentioned this pull request Jun 20, 2018

Logging in Watchbot4 #225

Merged

jakepruitt closed this Jun 28, 2018

YannickMeeus deleted the logging branch March 23, 2023 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logging in Watchbot4 #205

Logging in Watchbot4 #205

tapaswenipathak commented Jun 5, 2018 •

edited

jakepruitt Jun 5, 2018

tapaswenipathak Jun 6, 2018

jakepruitt Jun 6, 2018 •

edited

jakepruitt commented Jun 18, 2018

tapaswenipathak commented Jun 19, 2018

jakepruitt commented Jun 19, 2018

tapaswenipathak commented Jun 20, 2018

jakepruitt commented Jun 20, 2018

tapaswenipathak commented Jun 21, 2018

jakepruitt commented Jun 25, 2018 •

edited

jakepruitt commented Jun 28, 2018

	child.stdout.pipe(logger.stream());
	child.stderr.pipe(logger.stream());

Logging in Watchbot4 #205

Logging in Watchbot4 #205

Conversation

tapaswenipathak commented Jun 5, 2018 • edited

jakepruitt Jun 5, 2018

Choose a reason for hiding this comment

tapaswenipathak Jun 6, 2018

Choose a reason for hiding this comment

jakepruitt Jun 6, 2018 • edited

Choose a reason for hiding this comment

jakepruitt commented Jun 18, 2018

tapaswenipathak commented Jun 19, 2018

jakepruitt commented Jun 19, 2018

tapaswenipathak commented Jun 20, 2018

jakepruitt commented Jun 20, 2018

tapaswenipathak commented Jun 21, 2018

jakepruitt commented Jun 25, 2018 • edited

jakepruitt commented Jun 28, 2018

tapaswenipathak commented Jun 5, 2018 •

edited

jakepruitt Jun 6, 2018 •

edited

jakepruitt commented Jun 25, 2018 •

edited