Add structured logger (#54)

[nishkrishnan]

This is two changes:

• Replacing all usages of SimpleLogger with it's interface SimpleLogging which makes it easier to swap out loggers going forward
• Nukes SimpleLogger in favor of StructuredLogger which allows us to better analyze logs in our log management system we choose.

Right now the real changes are scoped to simple_logger.go and logging_test the rest of the changes are just interface changes + test changes to support the new constructor for NoopLogger

Before:

2021/03/08 03:11:56+0000 [INFO] runatlantis/atlantis#1234: Refreshing git tokens for Github App

After:

{"level":"info","ts":1616434996.1451647,"caller":"events/github_app_working_dir.go:26","msg":"Refreshing git tokens for Github App","json":{"repo":"runatlantis/atlantis","pull":"1234"}}

Note: As a base logger I'm using zap

[codecov]

Codecov Report

Merging #1467 (dbdc5d6) into master (13fba40) will increase coverage by 0.29%.
The diff coverage is 39.65%.

@@            Coverage Diff             @@
##           master    #1467      +/-   ##
==========================================
+ Coverage   69.93%   70.23%   +0.29%     
==========================================
  Files          94       94              
  Lines        6447     6441       -6     
==========================================
+ Hits         4509     4524      +15     
+ Misses       1558     1533      -25     
- Partials      380      384       +4     

Impacted Files	Coverage Δ
cmd/server.go	79.09% <ø> (ø)
server/events/delete_lock_command.go	75.00% <ø> (ø)
server/events/git_cred_writer.go	78.94% <ø> (ø)
server/events/github_app_working_dir.go	62.50% <ø> (ø)
server/events/models/models.go	74.10% <ø> (ø)
server/events/project_command_runner.go	47.53% <ø> (ø)
server/events/project_finder.go	90.90% <ø> (ø)
server/events/project_locker.go	81.81% <ø> (ø)
server/events/runtime/policy/conftest_client.go	63.41% <ø> (ø)
server/events/runtime/runtime.go	77.77% <ø> (ø)
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13fba40...dbdc5d6. Read the comment docs.

[lkysow]

👍 couple questions but overall good change

[lkysow]

@nishkrishnan can you show the before/after for what the logs will look like?

[nishkrishnan]

@nishkrishnan can you show the before/after for what the logs will look like?

added in the PR description

[msarvar]

This is great! The only concern I have is potentially breaking existing integration pipelines for the Atlantis users. I don't think it is a major issue.

[lkysow]

This is great! The only concern I have is potentially breaking existing integration pipelines for the Atlantis users. I don't think it is a major issue.

Do you mean folks that are parsing the logs somehow?

[nishkrishnan]

This is great! The only concern I have is potentially breaking existing integration pipelines for the Atlantis users. I don't think it is a major issue.

Do you mean folks that are parsing the logs somehow?

yeah that's what he means. Though it's hard to believe given the existing format is human readable not so much machine readable.

This will be released with v0.17.0 so since it's a major release, we can bring this up in the changelog as a backwards incompatibility.

[lkysow]

Two thoughts:

1. I was thinking that maybe folks would like a -log-format option to view the non-json logs in case they prefer the old view
2. I was wondering if we need the json: {} key, instead can we just have additional k/v's appended and not nested under the json key?

[nishkrishnan]

Two thoughts:

1. I was thinking that maybe folks would like a -log-format option to view the non-json logs in case they prefer the old view

Adds some additional complexity here to support both loggers, but I can do that if you feel strongly. was thinking this is just arguably better and should probably be the standard. Also, keep in mind that the history surfaced on PRs is still in the old format for readability so this only affects customers that read the logs on the server directly.

2. I was wondering if we need the json: {} key, instead can we just have additional k/v's appended and not nested under the json key?

I gotta say this was kind of specific to my company as our log parser by default looks for the json key and indexes all the fields under them. I think this makes sense because otherwise you would need to go back and edit the parser each time as you add a new field to your structured log. If you feel strongly about this I can remove it. If not I can add documentation around this.

[lkysow]

is just arguably better and should probably be the standard

I think this depends on the size of the company. If you're large and you already have log ingress with json parsing then yes. But if you're most likely to be looking at the logs as is, or testing out locally, then it's harder to read as JSON. I wonder if there's some detection if it's running in a terminal or not. I don't think this should block this PR.

our log parser by default looks for the json key

Ahh interesting. Is that a standard way to do it? If you're going to automatically index all fields under json why can't the parser be configured to index all fields separated by key=value key2="value 2". Not sure I see the difference here unless it's a limitation of the log parser configuration. Sounds like its okay to leave it as is and see if there are issues though.

[nishkrishnan]

I think this depends on the size of the company. If you're large and you already have log ingress with json parsing then yes. But if you're most likely to be looking at the logs as is, or testing out locally, then it's harder to read as JSON. I wonder if there's some detection if it's running in a terminal or not. I don't think this should block this PR.

Yeah this is fair, it's worth noting though that the test version of this logger (Nooplogger) actually prints in human readable format so for the tests it looks really nice for debugging but yes readability would take a hit when running the server locally.

Ahh interesting. Is that a standard way to do it? If you're going to automatically index all fields under json why can't the parser be configured to index all fields separated by key=value key2="value 2". Not sure I see the difference here unless it's a limitation of the log parser configuration. Sounds like its okay to leave it as is and see if there are issues though.

having a nested key avoids you indexing fields you wouldn't want to index such as the message itself. Unsure whether this is a standard, it's just what Lyft does 🤷

I think I'll just ship as part of the 0.17 release and we can get feedback on whether to bring back the human readable format or not. Something like this: https://pkg.go.dev/go.uber.org/zap#NewDevelopmentConfig seems it could be used for local servers since it doesn't use json encoding.

[alexdaviestray]

Hey @nishkrishnan, could you give any suggestions for how to enable and forward logs? For example, do I need to supply atlantis with a specific config or parameter to enabling logging? What events can I capture? And are logs saved to file or can I forward them somehow? Or will I need to implement something more custom?

Apologies if this is super obvious there's not much info in the docs when it comes to logging!