correct gc stats not coming from web server process#6743
correct gc stats not coming from web server process#6743FireLemons merged 2 commits intorubyforgood:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the GC-stats-to-Discord rake task so it reports GC stats from the running web server process (via a new /health/gc endpoint) rather than from the rake task process itself.
Changes:
- Add
GET /health/gcroute and controller action to return current GC stats from the Rails web process. - Add a token check for the new GC endpoint.
- Update the rake task to fetch GC stats over HTTP and post the response to Discord.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| lib/tasks/post_gc_stat_to_discord.rake | Fetches GC stats from the app via HTTP and posts them to Discord. |
| config/routes.rb | Adds a new health collection route for gc. |
| app/controllers/health_controller.rb | Implements /health/gc and token verification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| stats = GC.stat | ||
| require "net/http" | ||
|
|
||
| url = URI("https://casavolunteertracking.org/health/gc?token=#{ENV["GC_ACCESS_TOKEN"]}") |
There was a problem hiding this comment.
This rake task hardcodes the production host and interpolates the token directly into the URL. This makes the task non-portable across environments and risks leaking the token via logs/proxies (query string) as well as incorrect encoding. Prefer deriving the base URL from configuration (e.g., an env var) and building the query via URI.encode_www_form / URI::HTTPS.build, or sending the token in a header instead of the query string.
| url = URI("https://casavolunteertracking.org/health/gc?token=#{ENV["GC_ACCESS_TOKEN"]}") | |
| base_url = ENV.fetch("GC_BASE_URL", "https://casavolunteertracking.org") | |
| url = URI.join(base_url, "/health/gc") | |
| url.query = URI.encode_www_form(token: ENV["GC_ACCESS_TOKEN"]) |
| stats = response.body | ||
|
|
||
| unless ENV["DISCORD_WEBHOOK_URL"].nil? | ||
| formatted_stats = JSON.pretty_generate(stats) | ||
| discord_message = <<~MULTILINE | ||
| ```json | ||
| #{formatted_stats} | ||
| #{stats} | ||
| ``` |
There was a problem hiding this comment.
stats = response.body is posted to Discord as-is. If the endpoint returns structured JSON (array/hash), consider parsing and pretty-printing it here so the Discord message is consistently readable, and so the task fails fast on invalid JSON (instead of posting an error page/HTML).
| response = Net::HTTP.get_response(url) | ||
|
|
||
| unless response.is_a?(Net::HTTPSuccess) | ||
| raise "Failed to fetch GC stats. HTTP status code:#{response.code}" |
There was a problem hiding this comment.
The raised error message is missing a space after the colon (code:) and omits the response body, which can make diagnosing failures harder. Consider including a space and (optionally) a truncated body or response.message for context.
| raise "Failed to fetch GC stats. HTTP status code:#{response.code}" | |
| raise "Failed to fetch GC stats. HTTP status code: #{response.code} #{response.message}. Body: #{response.body&.slice(0, 200)}" |
| def gc | ||
| verify_token_for_gc_stats | ||
|
|
||
| render json: JSON.pretty_generate([ | ||
| Time.now.in_time_zone("Central Time (US & Canada)").strftime("%H"), | ||
| GC.stat | ||
| ]) | ||
| end |
There was a problem hiding this comment.
New /health/gc behavior (token enforcement + response shape) isn't covered by request specs, while the other health endpoints are covered in spec/requests/health_spec.rb. Please add request coverage for success (valid token) and forbidden (missing/invalid token) so regressions are caught.
app/controllers/health_controller.rb
Outdated
| render json: JSON.pretty_generate([ | ||
| Time.now.in_time_zone("Central Time (US & Canada)").strftime("%H"), | ||
| GC.stat | ||
| ]) |
There was a problem hiding this comment.
render json: JSON.pretty_generate(...) passes a pre-serialized String to render json:; Rails will JSON-encode that String again, so the response becomes a JSON string (with escaped \n) rather than an array/object. Return a Ruby object (e.g., an array/hash) and let Rails serialize it, or use render plain: JSON.pretty_generate(...), content_type: 'application/json' if you truly want to send a raw JSON string.
| render json: JSON.pretty_generate([ | |
| Time.now.in_time_zone("Central Time (US & Canada)").strftime("%H"), | |
| GC.stat | |
| ]) | |
| render json: [ | |
| Time.now.in_time_zone("Central Time (US & Canada)").strftime("%H"), | |
| GC.stat | |
| ] |
| def verify_token_for_gc_stats | ||
| head :forbidden unless params[:token] == ENV.fetch("GC_ACCESS_TOKEN") | ||
| end |
There was a problem hiding this comment.
verify_token_for_gc_stats uses ENV.fetch('GC_ACCESS_TOKEN'), which will raise and 500 if the env var isn't set (e.g., in dev/test or a misconfigured deploy). Prefer handling a missing token explicitly (return 500/forbidden with a clear message) and use a constant-time compare (e.g., ActiveSupport::SecurityUtils.secure_compare) when comparing tokens.
app/controllers/health_controller.rb
Outdated
| verify_token_for_gc_stats | ||
|
|
There was a problem hiding this comment.
The gc action calls verify_token_for_gc_stats, but the same method is already registered as a before_action for :gc. This duplication makes the control flow harder to follow and (if the before_action is ever changed) could lead to head being called without halting, followed by a second render. Consider removing the in-action call (or return immediately after head inside the verifier).
| verify_token_for_gc_stats |
What changed, and why?
A rake task has its own process. So the gc stats I've been collecting aren't the stats from the app. Now they are.