Reduce log level of sampling error logging - #309
Conversation
Codecov Report
@@ Coverage Diff @@
## master #309 +/- ##
=========================================
Coverage 58.86% 58.86%
Complexity 1206 1206
=========================================
Files 131 131
Lines 5066 5066
Branches 593 593
=========================================
Hits 2982 2982
Misses 1809 1809
Partials 275 275
Continue to review full report at Codecov.
|
| pollRule(); | ||
| } catch (Throwable t) { | ||
| logger.error("Encountered error polling GetSamplingRules: ", t); | ||
| logger.warn("Encountered error polling GetSamplingRules: ", t); |
There was a problem hiding this comment.
I think in practice error and warn are the same, probably should switch to info.
There was a problem hiding this comment.
The key difference is that warn is not emitted at the SDK's default log level, and I feel like warn is probably more accurate still. Info to me is more like "normal lifecycle event" which this is not.
There was a problem hiding this comment.
I don't think SDKs ever have a default log level - it's in the end user app usually in a logback.xml. It is fairly standard practice to only log things above info level I think. This is because many libraries just log all failures as warning and don't use error at all. I guess switching to warn doesn't practically change the chance of being annoyed by the log mmessages for many apps
There was a problem hiding this comment.
Ok that makes sense - I guess I'm getting my languages confused with default log levels. Went ahead and changed to info.
There was a problem hiding this comment.
hi! willarmiros! When will the latest version of the log level reduction for sampling error logs be released
There was a problem hiding this comment.
@yijiang-song unfortunately we cannot provide ETAs for our releases, please feel free to watch the repo for the next release.
There was a problem hiding this comment.
Thank you! I will continue to pay attention to this issue.
4 participants
Issue #, if available:
aws/aws-xray-java-agent#65
Description of changes:
Given that errors retrieving sampling rules/targets are non-impacting to the fundamental functionality of tracing, we shouldn't be logging them at error level, especially since they are typically the result of transient network blips. Customers have complained about the verbose, scary looking error message in their logs.
@anuraaga would appreciate if you took a look.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.