-
Notifications
You must be signed in to change notification settings - Fork 556
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
Don't log to stdout in dev #27677
Don't log to stdout in dev #27677
Conversation
dev-build/conf/logback.xml
Outdated
<if condition='!property("STAGE").contains("DEV")'> | ||
<then> | ||
<appender-ref ref="ASYNCSTDOUT"/> | ||
</then> | ||
</if> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't seen this approach (using Conditional processing & Janino) for having different PROD & DEV logging configuration before, but doing a GitHub search I can see it's being used in 4 other places elsewhere at the Guardian, presumably successfully?
The approach I was more familiar with, from Ophan, was having two separate logback files:
logback.xml
- this gets used in devlogback-PROD.xml
...then we take advantage of the fact that when we create Production artifacts, we can specify custom javaOptions
in build.sbt
, adding a -Dlogger.resource=logback-PROD.xml
argument - so only built artifacts get run with that parameter, not when you're running the code locally in dev.
Like I say, I wasn't familiar with the Conditional processing & Janino approach, but I think both approaches have advantages/disadvantages.
Does property("STAGE")
mean the environment variable or the system property - and what is actually set in our running Frontend instances?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks maybe we will be better off with that approach although we have many files for all of the frontend applications.
The property("STAGE") is a system property that we are setting in the .sbtopts file to -DSTAGE=DEV
Co-Authored-By: Andrew Nowak <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! I can’t wait to see this merged into main so I can rebase my branch. I'm very much in need of getting rid of the noisy logs in the branch I'm working on right now 😆
Seen on ADMIN-PROD (merged by @DanielCliftonGuardian 12 minutes and 55 seconds ago)
|
Seen on FRONTS-PROD (merged by @DanielCliftonGuardian 13 minutes and 1 second ago)
|
What is the value of this and can you measure success?
We do not wish to log to stdout in dev as this is hindering the developer experience with the high output of logs.
This was introduced when we switched to using dev-x logs #27403
Resolves #27653
What does this change?
This PR disables stdout logging when running locally when stage is equal to dev.
Added the system variable -DSTAGE=DEV to the sbt bash script
Added Janino as necessary for conditional rendering https://logback.qos.ch/manual/configuration.html#conditional
Deployed to code. Still sending logs
Checklist