OBPIH-6860 set log level to INFO in prod#4964
Conversation
github-actions
bot
added
type: maintenance
|
|
||
| // Now that we've populated the MDC, log the request itself, including query parameters. | ||
| String queryString = StringUtils.isBlank(request?.queryString) ? "" : "?${request?.queryString}" | ||
| log.info("${request.requestURI}${queryString} [user:${session?.user?.username}, location:${session?.warehouse?.name}]") |
There was a problem hiding this comment.
I moved what was in AccessLogInterceptor to here since it felt like it should be encompassed by the logging interceptor. I also added the query parameters to the end of the request.
|
No need to review this before the release. I was just needing a break from Cycle Count so I did this for fun. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4964 +/- ##
=========================================
Coverage 7.61% 7.62%
Complexity 816 816
=========================================
Files 600 599 -1
Lines 42275 42272 -3
Branches 10277 10281 +4
=========================================
+ Hits 3221 3223 +2
+ Misses 38586 38579 -7
- Partials 468 470 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
awalkowiak
left a comment
awalkowiak
left a comment
There was a problem hiding this comment.
@ewaterman overall this looks good to me, I think I never looked at the RoleInterceptor logs and usually I was irritated by them. I additionally looked if there are any log.debugs that we might miss after the change and I don't see anything required there (plus we could also change the level through openboxes-config.groovy config when we need to debug some user roles). I am good with the change, but would like to get an approval from @jmiranda as well.
And I read this after review 😅 but yeah my thought was that this does not have to go to the release yet. |
ewaterman
added
the
warn: do not merge
|
marking this change as do not merge as we're going to wait until 0.9.3 is cut |
ewaterman
removed
the
warn: do not merge
|
|
||
| // Now that we've populated the MDC, log the request itself, including query parameters. | ||
| String queryString = StringUtils.isBlank(request?.queryString) ? "" : "?${request?.queryString}" | ||
| log.info("${request.requestURI}${queryString} [user:${session?.user?.username}, location:${session?.warehouse?.name}]") |
There was a problem hiding this comment.
I think this is the log message that @awalkowiak doesn't like. I find it helpful to demarcate logging for requests along with the message that reports response time.
Aside: Now that I'm looking at the logs I'm noticing that the response time is no longer reported. @awalkowiak Do you know when that got removed?
Anyway, I can be overruled if others would prefer to remove it or make it a DEBUG message.
There was a problem hiding this comment.
@jmiranda this one I like. I often look at this one. The one I dislike that is causing a lot of garbage is the one in the UserService -> log.info "Is role ${role.roleType} in ${acceptedRoleTypes} = ${acceptedRoleType}" (and I am happy that it was demoted to debug level)
3 participants
✨ Description of Change
Link to GitHub issue or Jira ticket: https://pihemr.atlassian.net/browse/OBPIH-6860
Description: Demoted some noisy logs to DEBUG and added the ability to set log level per deployment method (development is staying at DEBUG, production is changed to INFO).
Some logs we won't see anymore on prod:
📷 Screenshots & Recordings (optional)