Skip to content

All QosExceptions are SafeLoggable #612

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

Jump to bottom
Merged
merged 3 commits into from
Oct 8, 2020
Merged

All QosExceptions are SafeLoggable #612

merged 3 commits into from
Oct 8, 2020

Conversation

@iamdanfox
Copy link
Contributor

Before this PR

I was just looking at an internal stacktrace and saw the message was redacted, which made me wonder what it was:

Caused by: com.palantir.conjure.java.api.errors.QosException$Throttle: {throwable1_message}
	at com.palantir.conjure.java.api.errors.QosException.throttle(QosException.java:60)
	at java.util.Optional.orElseGet(Optional.java:369)
	at com.palantir.conjure.java.dialogue.serde.ErrorDecoder.decode(ErrorDecoder.java:88)

Turns out there was nothing interesting in this, but it made me curious enough that I wasted 2 minutes opening up the QosException.java class.

After this PR

==COMMIT_MSG==
All QosExceptions are now SafeLoggable
==COMMIT_MSG==

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented Oct 8, 2020 •
edited by iamdanfox
Loading

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

All QosExceptions are now SafeLoggable

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from fawind October 8, 2020 14:53
iamdanfox
iamdanfox commented Oct 8, 2020
View reviewed changes
errors/src/main/java/com/palantir/conjure/java/api/errors/QosException.java
@Override
public List<Arg<?>> getArgs() {
List<Arg<?>> args = new ArrayList<>();
args.add(SafeArg.of("redirectTo", redirectTo));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've flipped this to an UnsafeArg too.

I'm pretty sure this codepath is unused at the moment because none of our services actually send the redirect-to header, but figured we might as well fix it at the same time!

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@bulldozer-bot bulldozer-bot bot merged commit 932afa6 into develop Oct 8, 2020
@bulldozer-bot bulldozer-bot bot deleted the dfox/qosexception-is-safeloggable branch October 8, 2020 16:19
@svc-autorelease
Copy link
Collaborator

Released 2.16.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fawind fawind Awaiting requested review from fawind

1 more reviewer

@carterkozak carterkozak carterkozak approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autorelease merge when ready

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@iamdanfox @svc-autorelease @carterkozak