Fix #74 Change some log levels from info to debug #75
Conversation
lib/fhir_client/client.rb
Outdated
| @@ -301,7 +301,7 @@ def scrubbed_response_headers(result) | |||
|
|
|||
| def get(path, headers) | |||
| url = Addressable::URI.parse(build_url(path)).to_s | |||
| FHIR.logger.info "GETTING: #{url}" | |||
| FHIR.logger.debug "GETTING: #{url}" | |||
There was a problem hiding this comment.
Personally: I think it might be nice to leave this one as info. Other than a search by MRN or something, it shouldn't be much of a risk. But I feel like it would be nice to not have to set debug level to know that a request was made, or to which resources.
There was a problem hiding this comment.
@wesrich I was thinking about changing the log level because I think most of rails related gems are not so verbose at this level. But I'm open to implement another solution.
There was a problem hiding this comment.
I think the debug level is fine for most things. This particular line should only be logging the request URL though, which seems (to me) like it should be tolerably safe.
We are currently filtering our logs such that the request URL is logged inline, but the remainder of the request/response cycle is effectively debug. This particular line shouldn't be logging any headers or session, which would likely contain authentication details... so I would expect this line to be tolerably safe.
That said ... in most of our discussions internally, if the server is handling PHI, the logs, database, etc. should probably ALL be in a PHI-safe zone at all times in Production.
There was a problem hiding this comment.
All that said ... I'm not entirely opposed to this change as is, it just leaves you not knowing what request was made (if any) without switching to debug mode. So if an error does happen or a user reports that they're seeing unexpected data, it may not be as obvious if the request was sent off properly (we sent the URL we expected, but something went wrong after), or if the request was bad to begin with.
lib/fhir_client/client.rb
Outdated
| @@ -382,7 +382,7 @@ def get(path, headers) | |||
|
|
|||
| def post(path, resource, headers) | |||
| url = URI(build_url(path)).to_s | |||
| FHIR.logger.info "POSTING: #{url}" | |||
| FHIR.logger.debug "POSTING: #{url}" | |||
There was a problem hiding this comment.
Same here — and under put, patch, delete, and head, so I won't make the same comment over and over. 😄
in logs and avoid too much verbose information in level info log
mtparet
force-pushed
the
change_info_to_debug
branch
from
07a30ae
to
79c132a
Compare
|
Following comments of @wesrich I changed the code to keep the level |
The goal is to avoid in production leaking headers containing secrets (
security_headers) and also logging all requests should be reserved to the debug mode.Fix #74