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

feat(web,server)!: runtime log level #5672

Merged
merged 4 commits into from
Dec 14, 2023
Merged

feat(web,server)!: runtime log level #5672

merged 4 commits into from
Dec 14, 2023

Conversation

jrasm91
Copy link
Contributor

@jrasm91 jrasm91 commented Dec 13, 2023

Warning

Breaking Change

LOG_LEVEL value of simple has been removed.
Use log instead (or remove the env entirely)

Change the log level at runtime:

  • Log level options include: Fatal, Error, Warn, Log, Debug, and Verbose.
  • The default log level is Log.
  • Runtime logging is ignore when LOG_LEVEL is set at the environment variable level.
  • An error is thrown why trying to change the log level from the UI (if environment variable is set)

image

image

Copy link

cloudflare-pages bot commented Dec 13, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: d154fdb
Status:⚡️  Build in progress...

View logs

Copy link
Member

@bo0tzz bo0tzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice! I do think we should keep the option to configure log level early via an env var, I can imagine things breaking hard enough that you can't even make it into the UI to set the log level. A config file would work there, but is relatively quite a bit more work to set up and might interfere with other troubleshooting.

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! Just some minor things... ;)
Also, I agree with Boet and I think we should actually keep the env variable for this

server/src/domain/system-config/system-config.service.ts Outdated Show resolved Hide resolved
server/src/infra/logger.ts Show resolved Hide resolved
server/src/infra/repositories/communication.repository.ts Outdated Show resolved Hide resolved
@jrasm91
Copy link
Contributor Author

jrasm91 commented Dec 13, 2023

How should I resolve the two? Should I just ignore the runtime value while the environment variable is set?

@jrasm91 jrasm91 marked this pull request as ready for review December 13, 2023 20:47
@jrasm91 jrasm91 requested a review from bo0tzz December 13, 2023 20:49
@jrasm91 jrasm91 changed the title feat(web,server): runtime log level feat(web,server)!: runtime log level Dec 13, 2023
@jrasm91 jrasm91 enabled auto-merge (squash) December 14, 2023 16:53
@jrasm91 jrasm91 merged commit 9768931 into main Dec 14, 2023
17 of 18 checks passed
@jrasm91 jrasm91 deleted the feat/runtime-log-level branch December 14, 2023 16:55
jonhnet pushed a commit to jonhnet/immich that referenced this pull request Dec 15, 2023
* feat: change log level at runtime

* chore: open api

* chore: prefer env over runtime

* chore: remove default env value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants