The logger component requires cluster (which in turn requires repl), but the libraries are not required in either dependencies nor documented as required.

We should wire up CI to run list + tests for all PRs and merges.

When we started this package, our first helper was the logger and thus, we introduced winston as a dependency to this package.

However, lot of our users are using the server or newrelic helpers and don't need winston

I suggest we move winston to a peerDependency or remove it all together and ask clients to install it explicitly

I've noticed that the format for the JSON log is a mix of string text and JSON like so

// => The format is: <timestamp> <namespace> JSON
// => 2018-08-03T09:54:07.013Z application:application_type {"message":"This is a log from my application","level":"info","timestamp":"2018-08-03T09:54:07.013Z","app":"application","app_type":":application_type" // etc // }

But for almost all examples of JSON logging for logstash I've seen, the format is always fully JSON.
See the example JSON logs for use with logstash here.

I think this makes sense as logstash would not need a custom parser to fish out the JSON part from the string text part. So to support better parsing and search on logstash, we should do the same as well.

See also Extracting fields and Wrangling Data

We currently do the same for the logger:

https://github.com/Automattic/vip-go-node/blob/master/src/logger/index.js#L7

We should start newrelic only if it's VIP Go

Current behavior:

Generally, node_modules folder isn't transpiled and we do not transpile the code before publishing it on npm. This will create a mess for the users

Expected behavior:

Transpiled code pushed to npm

Steps to correct the bug:

Add a transpile step before publishing to npm

In some internal apps, we have a helper library to instantiate a Redis client. This will be helpful to others as well and should be made available here. @nickdaugherty noted that we may want to update that lib to use ioredis instead though before opening it up for use.

We should also include some utility functions to get the host/port/password from the appropriate env vars so apps don't have to worry about those implementation details.

When the .listen() callback fires, we should log that the server is now listening, so each implemention doesn't need to add its own logging.

This issue provides visibility into Renovate updates and their statuses. Learn more

Rate Limited

These updates are currently rate limited. Click on a checkbox below to force their creation now.

• Update dependency express to v4.17.3
• Update actions/checkout action to v3
• Update actions/setup-node action to v3
• Update dependency chalk to v5
• Update dependency eslint to v8
• Update dependency eslint-plugin-flowtype to v8
• Update dependency execa to v6
• Update dependency ioredis to v5

Edited/Blocked

These updates have been manually edited so Renovate will no longer make changes. To discard all commits and start over, click on a checkbox.

• Update dependency ajv to 6.12.3 [SECURITY]
• Update dependency color-string to 1.5.5 [SECURITY]
• Update dependency hosted-git-info to 2.8.9 [SECURITY]
• Update dependency json-schema to 0.4.0 [SECURITY]
• Update dependency lodash to 4.17.21 [SECURITY]
• Update dependency node-notifier to 8.0.1 [SECURITY]
• Update dependency path-parse to 1.0.7 [SECURITY]
• Update dependency tmpl to 1.0.5 [SECURITY]
• Update dependency ws to 7.4.6 [SECURITY]
• Update dependency y18n to 4.0.1 [SECURITY]
• Pin dependencies (@babel/cli, @babel/preset-env, jest, publish-please)
• Update eslint-config-wpvip digest to d9e3231
• Update dependency publish-please to v5.5.2
• Update babel monorepo (@babel/cli, @babel/plugin-transform-runtime, @babel/preset-env, @babel/runtime, babel-eslint)
• Update dependency eslint-plugin-flowtype to v4.7.0
• Update dependency eslint-plugin-import to v2.26.0
• Update dependency eslint-plugin-json to v2.1.2
• Update dependency eslint-plugin-jsx-a11y to v6.5.1
• Update dependency eslint-plugin-react to v7.29.4
• Update dependency jest to v25.5.4
• Update dependency eslint-plugin-json to v3
• Update dependency eslint-plugin-wpcalypso to v5
• Update dependency jest to v27
• Update dependency supertest to v6

Open

These updates have all been created already. Click a checkbox below to force a retry/rebase of any.

• Update dependency kind-of to 6.0.3 [SECURITY]
• Update dependency minimist [SECURITY]
• Update dependency ioredis to v4.28.5
• Click on this checkbox to rebase all open PRs at once

• Check this box to trigger a request for Renovate to run again on this repository

There are some options not documented in the README: transport, cluster ...

We need a new part (called Options?) where we can provide extra information on those ^

Expected/Desired Behavior

1. When following the instructions, I get no weird errors

Actual Behavior

When following the instructions, a node version complaint is issues because New Relic 4.x is not compatible with newer and supported versions of node.js

Steps to Reproduce the Problem

1. yarn add newrelic@^4.8.0
2. A message saying the engine node is incompatible with this module

(Optional) Additional notes

Generally, perhaps version requirements should be omitted from the readme unless absolutely necessary, and if so, kept up to date.

Easy interface for integrating New Relic APM: https://docs.newrelic.com/docs/agents/nodejs-agent/installation-configuration/install-nodejs-agent

Node Cluster lets us run N workers for a Node app, for better fault tolerance and utilization of available CPU cores.

Our logger should natively detect when Cluster is being used and log information about the worker process, if applicable.

https://nodejs.org/api/cluster.html

Essentially:

const cluster = require( 'cluster' );

let appWorker = 'none';

if ( cluster.isMaster ) {
  appWorker = 'master';
} else if ( cluster.isWorker ) {
  appWorker = `worker_${ cluster.worker.id }`;
}

// ... add `app_worker: appWorker` to logged entry

Since NR is an optional part of this module, we shouldn't add it to peerDependencies because that incorrectly indicates that it's required and throws an npm warning on every project using this repo w/o the NR piece.

The warnings aren't terrible, but there are build scripts out there (such as those that come out of the box with create-react-app, I believe) that treat any warnings as errors when building for production, so this package breaks builds on projects that aren't using New Relic.

Instead of enforcing it programmatically, we should ensure the docs make that clear. We already have custom error handling if newrelic isn't installed, here:

Would make it much easier during development and simplify usage across different environments.

i.e. npx @automattic/vip-go-preflight-checks /path/to/app

• Add an X-Powered-By: WordPress.com VIP header to all requests served by the server module

• Add a way to easily opt out for clients if they don't want the header

It would be good to pass winston parameters to our logger. We can take parameters we want and pass the remaining to winston

Initially started this in #1 but noting requirements:

• Should boot up the server at PORT.
• Should accept requestHandler to allow someone to pass in their own server framework like express.
• Should return the created server.
• Should add a request handler for the /cache-healthcheck? path and return a 200 response.

Bonus: Should have cluster support (see src/server.js in public api codebase as an example).

This was spotted when trying to migrate vip-ui to the server module.

Current behavior:

Our current matcher for the health check route will respond to the client and pass the request along. If the client is using a general matcher for routes, i.e. app.get('*'), this will trigger another HTTP response and cause an Headers already sent error

Desired behavior:

Respond with our matcher only

Steps to correct the bug:

Make the handler return after responding instead of passing the call

Example:

log.info( 'Testing string interpolation, %o', { some: 'fancy', object: { with: 'nested' } } );

The log file and logstash then get really confused and everything is jumbled. I haven't really dug into what's wrong here, but I think here is a good place to start looking:

On registry https://registry.npmjs.org/, the "latest" version (v6.24.1) of dependency babel-preset-es2015 has the following deprecation notice:

🙌 Thanks for using Babel: we recommend using babel-preset-env now: please read babeljs.io/env to update!

Marking the latest version of an npm package as deprecated results in the entire package being considered deprecated, so contact the package author you think this is a mistake.

Please take the actions necessary to rename or substitute this deprecated package and commit to your base branch. If you wish to ignore this deprecation warning and continue using babel-preset-es2015 as-is, please add it to your ignoreDeps array in Renovate config before closing this issue, otherwise another issue will be recreated the next time Renovate runs.

Affected package file(s): package.json

Would you like to disable Renovate's deprecation warning issues? Add the following to your config:

"suppressNotifications": ["deprecationWarningIssues"]

Possibly a script (e.g. npx vip-go-preflight-checks) that can be run to verify that the app meets all the necessary requirements to work smoothly on VIP Go.

Rather than modifying the app in the current folder, make a copy of it (minus node_modules) and run the checks on that instead. This would avoid conflicts caused by the checks which can be frustrating or misleading.

Currently we raise an error and the app is not usable if env vars are missing, we should probably log this error and start the app so we don't break the clients apps

Expected/Desired Behavior

On the VIP environment, there is a VIP_GO_APP_ID environment available, which some apps make use of to detect the environment (and conditionally run builds, etc).

We need to emulate production as closely as possible.

Actual Behavior

VIP_GO_APP_ID is not set, which means conditional logic in package.json and the apps themselves will behave differently in the preflight tests than they do in production.

Steps to Reproduce the Problem

1. Have a package.json with a line like this: "build": "if [ -z $VIP_GO_APP_ID ]; then npm run webpack; else exit 0; fi",
2. Run the preflight tests
3. Note that the npm run webpack line tries to execute, when it wouldn't in production - this is the bug

PR's labeled Needs Review should go to the VIP slack.

Currently we're using the debug package across our apps. However, Node apps on Go require the logging format to be very specific which is a bit harder to do with debug. It also doesn't support error levels so we'd need to add a wrapper library to add that functionality.

We should extend winston (or a similar library) with a custom transport that pipes to stdOut and matches the expected output by our Logstash parser.

We should also have a separate transport for local use as well (since JSON isn't very easy to read :)).

The logs need to be formatted as JSON with a specific set of fields. This allows the log parser to actually pick them up and we can them access them via Kibana.

See src/logger/index.js in the happychat repo for an example of what the output should look like.

If an app crashes with uncaughtException we need to make sure we log it.

If we add in cluster support (as noted in #4), the error message should include specifics around which worker caused the fatal.

AFAICT the default Console transport for winston doesn't send debug level messages to the console - we should probably change that for local dev (?).

We should think about this more though - we could go with an approach like what the debug npm package does (read what to log from env var), or maybe have the log level in the env var.

Via @nickdaugherty in #8 (comment)

Oh another consideration, and something we don't necessarily have to support in this PR, is using Node Cluster for more resilient and performant apps out of the box.

The library could abstract away and hide the details of setting up a clustered process, though maybe should only be enabled with an option since it requires the application to be written with multi process in mind.

For an example of how we're using Cluster on Go now (but sadly only using a single worker), see the public API repo.

We shouldn't bother installing it if it's not being used. We should make the calling application handle the installation instead.

Things to do:

• Switch to a peer dependency
• If possible, gracefully fail with error if newrelic not installed
• Update docs on how to install newrelic as a peer dependency

All the code is ready to go. A couple left things to do:

• Fix (New Relic) build issue on the Go side
• Prepare changelog
• Publish!

There is an error with this repository's Renovate configuration that needs to be fixed. As a precaution, Renovate will stop PRs until it is resolved.

Error type: Preset name not found within published preset config (monorepo:babel6). Note: this is a nested preset so please contact the preset author if you are unable to fix it yourself.

• Integrate publish-please
• Package package on npm (@Automattic/vip-go)
• Document the process (something similar to https://github.com/Automattic/vip/blob/master/CONTRIBUTING.md#releasing--publishing)

Expected/Desired Behavior

Add ability for apps to easily purge a URL from the VIP edge caches.

Something akin to the wpcom_vip_purge_edge_cache_for_url() PHP function.

Actual Behavior

Currently there is no support for purging a URL.

Steps to Reproduce the Problem

Only workaround for now is to have a Node app call a WP app and use the above PHP function, or for the Node app to make the PURGE request directly, once the correct URLs and args have been established.

It would be nice to have a way to globally set the transports, (overridable per-instance).

The main use-case that comes up is tests - during CI tests the logs should probably be disabled, but currently there isn't a way to do that without adding conditional transports to every instance of the logger.