Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce log level of sampling error logging #309

Merged
merged 4 commits into from
Nov 12, 2021
+2 −2
Merged

Reduce log level of sampling error logging #309

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff9b869
Reduce log level of target poller throwables
willarmiros Nov 11, 2021
60dca53
Update RulePoller.java
willarmiros Nov 11, 2021
095e910
Update RulePoller.java
willarmiros Nov 12, 2021
2f01b03
Update TargetPoller.java
willarmiros Nov 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...order-sdk-core/src/main/java/com/amazonaws/xray/strategy/sampling/pollers/RulePoller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public void start() {
try {
pollRule();
} catch (Throwable t) {
logger.error("Encountered error polling GetSamplingRules: ", t);
logger.warn("Encountered error polling GetSamplingRules: ", t);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in practice error and warn are the same, probably should switch to info.

Copy link
Contributor Author

@willarmiros willarmiros Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that makes sense - I guess I'm getting my languages confused with default log levels. Went ahead and changed to info.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi! willarmiros! When will the latest version of the log level reduction for sampling error logs be released

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yijiang-song unfortunately we cannot provide ETAs for our releases, please feel free to watch the repo for the next release.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I will continue to pay attention to this issue.

// Propagate if Error so executor stops executing.
// TODO(anuraaga): Many Errors aren't fatal, this should probably be more restricted, e.g.
// https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/internal/Throwables.java
Expand Down
2 changes: 1 addition & 1 deletion ...der-sdk-core/src/main/java/com/amazonaws/xray/strategy/sampling/pollers/TargetPoller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public void start() {
try {
pollManifest();
} catch (Throwable t) {
logger.error("Encountered error polling GetSamplingTargets: ", t);
logger.warn("Encountered error polling GetSamplingTargets: ", t);
// Propagate if Error so executor stops executing.
// TODO(anuraaga): Many Errors aren't fatal, this should probably be more restricted, e.g.
// https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/internal/Throwables.java
Expand Down