Conversation
❌ 2 blocking issues (2 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| used_deprecated_arguments: analyzer_data[:used_deprecated_arguments] || [] | ||
| ) | ||
|
|
||
| response |
Why are these changes being introduced: * Our new logging platform will support structured logging * While moving to JSON logs, it was worth understanding how to add custom fields to the log payload so this demonstrates with a few examples. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-678 How does this address that need: * Removed Lograge and adds rails_semantic_logger * Builds a few individual logging lines into a single rich line
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes the production logging stack and request log payload behavior in ways that should be manually verified in real runtime/log aggregation before approval.
Pull request overview
This PR replaces the app’s request logging approach (previously Lograge + custom per-line logging) with rails_semantic_logger, aiming to emit richer single-line structured logs—especially for GraphQL search requests—by aggregating analyzer and resolver data into the request log payload.
Changes:
- Remove
logrageand its initializer; configurerails_semantic_loggerappenders for production (JSON) and development (color). - Refactor GraphQL request logging to collect analyzer output and per-search events into the controller log payload (via
append_info_to_payload), instead of ad-hoc logger lines and a custom tracer. - Add
amazing_printfor development/test.
File summaries
| File | Description |
|---|---|
| Gemfile.lock | Removes lograge/request_store, adds rails_semantic_logger + semantic_logger and amazing_print. |
| Gemfile | Swaps lograge for rails_semantic_logger; adds amazing_print to dev/test. |
| config/initializers/lograge.rb | Deletes Lograge configuration (params/time/host payload enrichment). |
| config/environments/production.rb | Configures Semantic Logger JSON output to stdout. |
| config/environments/development.rb | Configures Semantic Logger color output to stdout (with optional file appender commented). |
| app/graphql/types/query_type.rb | Stops per-request log line; pushes structured search event data into GraphQL context for controller payload logging. |
| app/graphql/timdex_request_tracer.rb | Removes custom GraphQL tracer previously used to expose analyzer data. |
| app/graphql/timdex_field_usage_analyzer.rb | Writes analyzer results directly to query.context[:graphql_analysis] and returns structured analysis data. |
| app/controllers/graphql_controller.rb | Adds @graphql_search_events collection and appends GraphQL-specific structured data into the Rails log payload. |
| app/controllers/application_controller.rb | Adds global log payload enrichment (host) and adjusts preferred-domain redirect URL construction. |
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| private | ||
|
|
||
| # Appends additional information to the log payload that are not Controller specific |
Contributor
There was a problem hiding this comment.
Very French: << les informations >>
Comment on lines
19
to
+20
| Rails.logger.info("Handling Domain Redirect: #{request.host}") | ||
| redirect_to "https://#{ENV['PREFERRED_DOMAIN']}", status: :moved_permanently, allow_other_host: true | ||
| redirect_to "https://#{ENV.fetch('PREFERRED_DOMAIN', nil)}", status: :moved_permanently, allow_other_host: true |
Member
Author
There was a problem hiding this comment.
FWIW I agree, but the linters auto-fix it to this so it's pointless to fight the bots on it.
jazairi
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds a few individual logging lines into a single rich line
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
YES