Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coupling of CloudRuntime (ATHENS_CLOUD_RUNTIME) config var with log formatting is unexpected #1924

Closed
adamrothman opened this issue Mar 19, 2024 · 2 comments · Fixed by #1926 or #1927
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@adamrothman
Copy link

adamrothman commented Mar 19, 2024

Describe the bug
The only way to configure the formatter applied to Athens' logs is by setting the CloudRuntime / ATHENS_CLOUD_RUNTIME config var. The default value "none" configures unstructured text logging, which is undesirable outside of dev/debug contexts.

Error Message
N/A

To Reproduce
The maximal example config file here says:

# CloudRuntime is the Cloud Provider on which the Proxy/Registry is running.
# Currently available options are "GCP", or "none". Defaults to "none"
# Env override: ATHENS_CLOUD_RUNTIME
CloudRuntime = "none"

We don't run Athens in GCP, so we leave this as-is. When this value is "none", the logger setup code uses devFormatter:

athens/pkg/log/log.go

Lines 22 to 23 in d877af2

case "none":
l.Formatter = getDevFormatter()

This formatter outputs unstructured text, but we are running Athens in a production context and would like JSON logs to send to our ingest pipeline. I think can work around this by setting CloudRuntime = "anything else", but IMO this coupling is unintuitive.

Expected behavior
JSON logging should be configured by a dedicated config variable, independent of cloud provider.

Environment (please complete the following information):

  • OS: Ubuntu 22.04 64-bit
  • Go version: 1.22.0
  • Proxy version: 0.13.1
  • Storage (fs/mongodb/s3 etc.): S3

Additional context
N/A

@matt0x6F
Copy link
Contributor

matt0x6F commented Mar 20, 2024

I agree this isn't the best way to configure a logger. Logrus' API isn't super complex, I think we could probably configure two additional environment variables that configure the logger more succinctly if this value is not set. Something like, ATHENS_LOG_FORMAT (plain/json) and (the existing setting) ATHENS_LOG_LEVEL.

Any other configuration that you think would be needed/appropriate?

@matt0x6F matt0x6F added this to the 0.13.2 milestone Mar 21, 2024
@matt0x6F matt0x6F added the enhancement New feature or request label Mar 21, 2024
@matt0x6F matt0x6F self-assigned this Mar 21, 2024
@adamrothman
Copy link
Author

Apologies for not replying sooner – your change looks great to me. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants