Conversation
tra0x
force-pushed
the
tra0x/add-logging-to-cloudflare
branch
4 times, most recently
from
b853e41 to
cafd10a
Compare
| $stderr.puts "Server error: #{response.code} #{response.message}" | ||
| raise RecordStore::Provider::Error, "Server error: #{response.code} #{response.message}" | ||
| elsif response.code.to_i >= 400 | ||
| error_messages = JSON.parse(response.body)['errors'].map { |error| error['message'] }.join(', ') |
There was a problem hiding this comment.
🤔 I believe we should not assume "errors" will be present (even if the docs "guarantees" it, we should be defensive). Should we just always log the full response body + throw if the status is not successful (200-299)?
nit: I also wonder if just throwing the error is sufficient and assume something at a higher level should log the error message anyway (no strong opinion on this)
There was a problem hiding this comment.
I like your more generalized approach to just log the full response. More robust and reliable solution there. Will update.
I also wonder if just throwing the error is sufficient and assume something at a higher level should log the error message anyway (no strong opinion on this)
Is this in reference to the stderr.puts? I think I see what you're getting at. Right now record-store is the only service using record_store gem. The service isn't properly hooked up to logging or anything. Debugging is done through reading shipit job logs (example). Logs are what is looked at to walk through and debug the entire deploy and update operation.
There was a problem hiding this comment.
Is this in reference to the stderr.puts?
Yes. I would think that if you raise an error with the response body + status in its message and no one catches that error, the error message will be outputted by the ruby program anyway before exiting (as we saw an error message for the null pointer exception thing). But it's really a nit, I wouldn't really mind seeing the same error message twice, as long as it's there :)
There was a problem hiding this comment.
This gem is public, let's not assume that record-store is the only user. I agree that raising an error only and letting the caller handle it is more flexible
| end | ||
|
|
||
| if response.code.to_i >= 500 | ||
| $stderr.puts "Server error: #{response.code} #{response.message}" |
There was a problem hiding this comment.
let's also print the body if available
| $stderr.puts "Server error: #{response.code} #{response.message}" | |
| $stderr.puts "Server error: #{response.code} #{response.message}" | |
| $stderr.puts response.body if response.body_permitted? |
tra0x
force-pushed
the
tra0x/add-logging-to-cloudflare
branch
3 times, most recently
from
f91c53e to
3bb5f2c
Compare
tra0x
force-pushed
the
tra0x/add-logging-to-cloudflare
branch
from
3bb5f2c to
ccd61d5
Compare
3 participants
No description provided.