Replace default logging with semantic logging - #363
Conversation
This defines different appenders for dev and prod tiers. The prod tier is taken from the gem docs, the dev tier is a bit of a judgement call on my part.
There was a problem hiding this comment.
🟡 Changes recommended
The environment configs currently contain conflicting logger setups (semantic logger appenders plus explicit config.logger overrides) that can override or duplicate logging behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces Semantic Logger–based structured logging by adding rails_semantic_logger, configuring environment-specific appenders (JSON to stdout in production, color/file output in development), and setting a consistent SemanticLogger.application name.
Changes:
- Add
rails_semantic_logger(and supporting gems) to the bundle. - Configure semantic logger appenders in
production.rbanddevelopment.rb. - Set
SemanticLogger.applicationviaRAILS_APP_NAME(with a default) inconfig/application.rb.
File summaries
| File | Description |
|---|---|
| Gemfile.lock | Adds locked dependencies for semantic logging and pretty-printing. |
| Gemfile | Adds rails_semantic_logger and amazing_print dependencies. |
| config/environments/production.rb | Adds production semantic logger appender configuration (JSON). |
| config/environments/development.rb | Adds development semantic logger appender configuration (color + file + server). |
| config/application.rb | Sets global SemanticLogger.application name via ENV/default. |
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| # Log configuration for rails_semantic_logger | ||
| config.rails_semantic_logger.appenders do |appenders| | ||
| appenders.add(file_name: "log/#{Rails.env}.log", formatter: :color) |
There was a problem hiding this comment.
I don't recommend we log to a file even in development.
Timdex has this commented out: https://github.com/MITLibraries/timdex/blob/main/config/environments/development.rb#L68-L71
Timdex-UI does as well:
https://github.com/MITLibraries/timdex-ui/blob/main/config/environments/development.rb#L80-L83
They both use a stdout line as the only dev mode and it's pretty useful output. I haven't checked what the format of this is though so I'm not specifically saying to to what the other apps do other than use stdout instead of the log file.
There was a problem hiding this comment.
No worries - for the times when I want to parse a larger number of log lines in an editor, I can grab them from the terminal output.
matt-bernhardt
changed the title
| # This application leverages SemanticLogger for more useful logging and | ||
| # insights. The details of that implementation vary by environment, but | ||
| # certain configuration is the same everywhere. | ||
| SemanticLogger.application = ENV.fetch("RAILS_APP_NAME", "tacos") |
There was a problem hiding this comment.
Interesting. While I agree this is fine, I'm not confident any tier other than prod really needs this. The risk of it being here is that it separates the configuration across multiple config files which might make it harder to keep track of. I don't see the value of setting this in dev or test which are the only env other than production focus.
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
Let's not log dev to a file. We'll never remember to delete it and inevitably be confused when we don't have hard drive space over time :)
- Remove logging to a file in dev environments - Move the application name to the prod environment specifically
| # Log configuration for rails_semantic_logger | ||
| config.rails_semantic_logger.appenders do |appenders| | ||
| appenders.add_server( | ||
| formatter: {color: {ap: {multiline: true}}} |
There was a problem hiding this comment.
I'm curious about how to handle multiline true vs false.
timdex-ui and api do not set this to true. If we have a different preference on the team for how this works across apps, we should likely make this a consistently configurable value via ENV that defaults to the majority preference on the team. I'll bring this up in Slack before we proceed with this and other apps to ensure we are normalizing where appropriate.
There was a problem hiding this comment.
@matt-bernhardt Team agreement in Slack was to make this value configurable. See Slack for convo about the default value and suggested implementation pattern.
This replaces the default logging configuration with semantic logging, in anticipation of integrating with a different cloud logging provider.
We define the application name as a blanket attribute, and then use different log targets for development and production environments. In order to aid local development, we also turn to the amazing_print gem for better formatting.
On Heroku, we use the JSON formatter that will prepare log content in a way that's sympathetic to cloud logging providers.
Developer
Ticket(s)
https://mitlibraries.atlassian.net/browse/TCO-220
Accessibility
all issues introduced by these changes have been resolved or opened
as new issues (link to those issues in the Pull Request details above)
Documentation
ENV
Stakeholders
Dependencies and migrations
YES dependencies are updated
NO migrations are included
Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing