feat: exposed rotating-file log for json logging #948

mlucool · 2018-08-24T21:50:40Z

This both allows for logger type 'rotating-file' and
passing of other options from the config "option" example:

{type: rotating-file, format: json, path: /path/to/log.jsonl, level: http, options: {period: 1d}}

Resolves: #388
Refers: #427

ayusharma · 2018-08-24T23:29:05Z

src/lib/logger.js

+ */
+class VerdaccioRotatingFileStream extends Logger.RotatingFileStream { // We depend on mv so that this is there
+  write(obj) {
+    const msg = fillInMsgTemplate(obj.msg, obj, false);


Where is the definition of fillInMsgTemplate ?

verdaccio/src/lib/logger.js

Line 187 in ef02b2a

function fillInMsgTemplate(msg, obj, colors) {

Sorry, I forgot to update my master branch.

ayusharma · 2018-08-24T23:32:36Z

src/lib/logger.js

+
+      let dest;
+      let destIsTTY = false;
+      const prettyPrint = (obj) => print(obj.level, obj.msg, obj, destIsTTY) + '\n';


Can be defined at top as utils ... Please use template literals https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#Syntax

ayusharma · 2018-08-24T23:44:21Z

src/lib/logger.js

+      let destIsTTY = false;
+      const prettyPrint = (obj) => print(obj.level, obj.msg, obj, destIsTTY) + '\n';
+      const prettyTimestampedPrint = (obj) => obj.time.toISOString() + print(obj.level, obj.msg, obj, destIsTTY) + '\n';
+      const jsonPrint = (obj) => {


Same as above.

ayusharma · 2018-08-24T23:55:12Z

src/lib/logger.js

+      let destIsTTY = false;
+      const prettyPrint = (obj) => print(obj.level, obj.msg, obj, destIsTTY) + '\n';
+      const prettyTimestampedPrint = (obj) => obj.time.toISOString() + print(obj.level, obj.msg, obj, destIsTTY) + '\n';
+      const jsonPrint = (obj) => {


Same as above.

ayusharma · 2018-08-25T00:00:23Z

package.json

@@ -44,6 +44,7 @@
    "mime": "2.3.1",
    "minimatch": "3.0.4",
    "mkdirp": "0.5.1",
+    "mv": "^2.1.1",


Should it not already installed by Logger ? https://github.com/trentm/node-bunyan/blob/fe31b83e42d9c7f784e83fdcc528a7c76e0dacae/package.json#L27

It's optional and this forces it to be there. Their code does if(mv): https://github.com/trentm/node-bunyan/blob/master/lib/bunyan.js#L96

Remove the ^ please

ayusharma · 2018-08-25T00:01:30Z

src/lib/logger.js

+/**
+ * A RotatingFileStream that modifes the message first
+ */
+class VerdaccioRotatingFileStream extends Logger.RotatingFileStream { // We depend on mv so that this is there


@juanpicado I am sure about extending the Logger.RotatingFileStream

What do you suggest instead? Reimplementing it is a lot of work and this is public interface so a breaking change would be declarative

make sense.

ayusharma · 2018-08-25T00:06:10Z

src/lib/logger.js

+        });
+      } else if (target.type === 'stdout' || target.type === 'stderr') {
+        dest = target.type === 'stdout' ? process.stdout : process.stderr;
+        destIsTTY = dest.isTTY;


destIsTTY it is not used further. ??

It is used above, which is why those functions are in this scope. It's basically binding them all to the variables in this block scope

You can pass destIsTTY in pretty prettyTimestampedPrint, jsonPrint explicitily. Currently it is not readable.

Ok, I'll refactor this class. Was trying to avoid that, but it is now unreadable, its time to refactor :)

Thanks @mlucool feel free to refactor the whole logger.js if you wish

ayusharma · 2018-08-25T00:06:40Z

src/lib/logger.js

+      stream.writable = true;
+
+      let dest;
+      let destIsTTY = false;


it can be a constant.

ayusharma · 2018-08-25T00:07:49Z

src/lib/logger.js


+      if (target.type === 'file') {


define dest here. I'll suggest using full name instead of short forms. destination

juanpicado · 2018-08-25T09:32:25Z

src/lib/logger.js

@@ -46,42 +57,79 @@ function setup(logs) {
      return JSON.stringify({...obj, msg}, Logger.safeCycles()) + '\n';
    };

-    if (target.type === 'file') {


From line 53 to 58, the following code is duplicated below and not being unused.

const prettyPrint = (obj) => print(obj.level, obj.msg, obj, destIsTTY) + '\n'; const prettyTimestampedPrint = (obj) => obj.time.toISOString() + print(obj.level, obj.msg, obj, destIsTTY) + '\n'; const jsonPrint = (obj) => { const msg = fillInMsgTemplate(obj.msg, obj, destIsTTY); return JSON.stringify({...obj, msg}, Logger.safeCycles()) + '\n'; };

Meh, bad rebase on my part. Will fix

juanpicado · 2018-08-25T09:46:46Z

/Users/user/projects/verdaccio/src/lib/logger.js
  51:9   error  'dest' is defined but never used                             no-unused-vars
  53:11  error  'prettyPrint' is assigned a value but never used             no-unused-vars
  54:11  error  'prettyTimestampedPrint' is assigned a value but never used  no-unused-vars
  55:11  error  'jsonPrint' is assigned a value but never used               no-unused-vars

✖ 7 problems (4 errors, 3 warnings)

juanpicado · 2018-08-25T09:50:36Z

⚠️ Cluster mode

WARNING on cluster usage: Using Bunyan's rotating-file stream with node.js's "cluster" module can result in unexpected file rotation. You must not have multiple processes in the cluster logging to the same file path. In other words, you must have a separate log file path for the master and each worker in the cluster. Alternatively, consider using a system file rotation facility such as logrotate on Linux or logadm on SmartOS/Illumos. See this comment on issue #117 for details.

https://github.com/trentm/node-bunyan#stream-type-rotating-file

@mlucool that deserves to be in the docs as well. As a reference at the bottom below the table

juanpicado · 2018-08-25T09:59:05Z

It seems does not work, it fills files partially and then are emtpy.

# log settings
logs:
#  - {type: stdout, format: pretty, level: http}
#  - {type: file, path: /Users/user/verdaccio.log, level: trace}
  - {type: rotating-file, format: json, path: /Users/user/verdaccio-rotate.json, level: http, options: {period: 1000ms}}

➜ npm install babel-core

juanpicado

🔧

mlucool · 2018-08-25T11:35:00Z

@juanpicado @ashishsurana Thanks for the comments, I'll look at this next week.

@mlucool that deserves to be in the docs as well. As a reference at the bottom below the table

Good catch. Want me to also throw on this via checking isWorker?

It seems does not work, it fills files partially and then are emtpy.

That is expected. It is a time based rotate only and will rotate even if there is no new data. In your example, you have it rotating every 1s. Try navigating the website or installing something every second and you'll see that logs continue to have data.

This both allows for logger type 'rotating-file' and passing of other options from the config "option" example: {type: rotating-file, format: json, path: /path/to/log.jsonl, level: http, options: {period: 1d}}

juanpicado · 2018-08-30T06:07:11Z

If there are no changes on logger configuration, it will be shipped with next minor v3.8.0release.

mlucool · 2018-08-30T14:27:35Z

@juanpicado @ayusharma please re-review.

juanpicado · 2018-08-30T14:28:28Z

@mlucool I was on it but .... sadly my ISP decided I have to change my router 😢 ... I'll be offline rest of the week until fix that.

juanpicado · 2018-08-31T22:05:58Z

yarn.lock

@@ -6647,7 +6647,7 @@ mute-stream@0.0.7:
  version "0.0.7"
  resolved "https://registry.npmjs.org/mute-stream/-/mute-stream-0.0.7.tgz#3075ce93bc21b8fab43e1bc4da7e8115ed1e7bab"

-mv@~2:
+mv@^2.1.1, mv@~2:


mv@^2.1.1, mv@~2: <-- the ^ remains in lock file

juanpicado

👍

juanpicado · 2018-08-31T22:42:32Z

Thanks @mlucool ! great job

juanpicado · 2018-09-05T20:39:54Z

@mlucool fyi
https://github.com/verdaccio/verdaccio/releases/tag/v3.8.0

mlucool · 2018-09-05T22:32:03Z

Thanks!

lock · 2018-12-17T21:14:46Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

ayusharma requested changes Aug 25, 2018

View reviewed changes

ayusharma requested a review from juanpicado August 25, 2018 00:09

juanpicado reviewed Aug 25, 2018

View reviewed changes

juanpicado requested changes Aug 25, 2018

View reviewed changes

juanpicado added this to In Progress in Verdaccio 4 Aug 25, 2018

feat: exposed rotating-file log for json logging

3caa015

This both allows for logger type 'rotating-file' and passing of other options from the config "option" example: {type: rotating-file, format: json, path: /path/to/log.jsonl, level: http, options: {period: 1d}}

juanpicado added this to To do in Refactoring Roadmap via automation Aug 30, 2018

juanpicado removed this from In Progress in Verdaccio 4 Aug 30, 2018

juanpicado added this to the 3.x.x milestone Aug 30, 2018

mlucool force-pushed the rotating-file-stream branch from ef02b2a to 3caa015 Compare August 30, 2018 14:25

juanpicado reviewed Aug 31, 2018

View reviewed changes

juanpicado approved these changes Aug 31, 2018

View reviewed changes

Merge branch 'master' into rotating-file-stream

c901456

juanpicado merged commit 5ca0ca5 into verdaccio:master Aug 31, 2018

Refactoring Roadmap automation moved this from To do to Done Aug 31, 2018

juanpicado added the topic: feature request label Aug 31, 2018

lock bot locked as resolved and limited conversation to collaborators Dec 17, 2018

feat: exposed rotating-file log for json logging #948

feat: exposed rotating-file log for json logging #948

Conversation

mlucool commented Aug 24, 2018 • edited by juanpicado 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

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

juanpicado commented Aug 25, 2018 • edited Loading

juanpicado commented Aug 25, 2018

juanpicado commented Aug 25, 2018 • edited Loading

juanpicado left a comment

Choose a reason for hiding this comment

mlucool commented Aug 25, 2018 • edited Loading

juanpicado commented Aug 30, 2018

mlucool commented Aug 30, 2018

juanpicado commented Aug 30, 2018

Choose a reason for hiding this comment

juanpicado left a comment

Choose a reason for hiding this comment

juanpicado commented Aug 31, 2018

juanpicado commented Sep 5, 2018

mlucool commented Sep 5, 2018

lock bot commented Dec 17, 2018

mlucool commented Aug 24, 2018 •

edited by juanpicado

Loading

juanpicado commented Aug 25, 2018 •

edited

Loading

juanpicado commented Aug 25, 2018 •

edited

Loading

mlucool commented Aug 25, 2018 •

edited

Loading