fix: log json in production

[rethab]

in production mode -- according to the docs -- we should be logging json.

however with the most recent refactoring, that seems to got lost and probot now always logs in the "pretty" format.

this PR reinstates that: if the NODE_ENV is set to production, json logging is used unless specified otherwise. Else, it defaults to pretty.

fixes #1573

[rethab]

The tests fail, but hopefully you can still take a look, @gr2m just to make sure the general idea is sound.

re test failure: I guess it depends on the terminal whether colors are printed. Perhaps it would be enough to just make sure no { is part of the log output? (in other words, just remove the specific expects that are failing now). Please let me know how elaborate you want these tests to be

[rethab]

@gr2m friendly ping :)

[gr2m]

sorry it will probably take some time before I find the time to look into this :( maybe some other @probot/maintainers can help out

[rethab]

Removed the color expectation to make the tests green. Hopefully this convinces a maintainer to take a look :)

[rethab]

Hi @gr2m, who would be the other maintainers? Or is there something I could do to make it easier for you to review this change?

[rethab]

@gr2m friendly ping 😬 😢

[gr2m]

No need to keep pinging us. I hope to have some time to look into it soon, but cannot make promises, sorry. I just don't have free time right now

[tcbyrd]

Great work and thanks for the PR! Found a blocker that I think we'll have to address before merging to fully fix this.

[tcbyrd]

This part isn't working when run from from the CLI because logFormat defaults to "pretty" when not otherwise defined:

probot/src/bin/read-cli-options.ts

Line 47 in 02a57d1

process.env.LOG_FORMAT || "pretty"

So logFormat is always defined, which means we never hit this block that sets it to json. We could either ignore LOG_FORMAT if isProd() or do something like:

Suggested change

	if (isProd() && !getTransformStreamOptions.logFormat) {
	getTransformStreamOptions.logFormat = "json";
	}
	const logPretty = !isProd() && getTransformStreamOptions.logFormat == undefined
	getTransformStreamOptions.logFormat = logPretty ? "pretty" : "json";

The latter option would mean removing the CLI option that defaults to "pretty", and should work just fine if you explicitly set LOG_FORMAT since it wouldn't be undefined when we get here. Thoughts?

[rethab]

Thanks for your feedback, @tcbyrd. Two things to consider here:

• Should json and pretty be the only options? With your suggestion, we'd limit that.
• Should we always use json in production?

I'm curious what your thoughts are on this. Is there a guideline we should follow from probot's perspective?

My thinking was: If the user specifies something, we take it. If the user doesn't specify anything and we're in production, then we use json. This has the consequence that, when running from the CLI, the user would have to explicitly specify json if they want to log json.

Depending on what you think about the two points above, we could also change the default for the cli to something like process.env.LOG_FORMAT || (isProd() ? "json" : "pretty").

[gr2m]

Should json and pretty be the only options

json and pretty are the only formats built into pino. If you want another format, you'd have to use a custom pino logger.

process.env.LOG_FORMAT || (isProd() ? "json" : "pretty").

Looks like the right approach to me.

[rethab]

addressed here

[tcbyrd]

LGTM! 🌮

[github-actions]

🎉 This PR is included in version 12.1.3 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

[rethab]

Thanks @gr2m and @tcbyrd 🎉 ❤️