-
Notifications
You must be signed in to change notification settings - Fork 713
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.
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
Adds ability to redact fields from downstream propagation #931
Conversation
@@ -143,25 +150,20 @@ public FactoryBuilder addPrefixedFields(String prefix, Collection<String> fieldN | |||
} | |||
|
|||
public Factory build() { | |||
if (prefixedNames.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most of the red here was just me trying to pay for the new code by removing old code
} | ||
int i = 0; | ||
for (String fieldName : fieldNames) { | ||
if (redactedFieldNames.contains(fieldName)) redacted.set(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the only line that matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I'd maybe add a note to caution against using this when extending another mechanism might be more appropriate, for instance extending the Authentication
object in spring-security so you can use the SecurityContextHolder
instead
will add some text tomorrow @devinsba probably on the extra fields propagation type itself and the README. I've also noticed in sleuth support people abusing extra fields a lot just to get things done in process. Not sure if folks are always aware of the options out there. Aside, I wanted to mention that anything more custom than this is possible with composition. An approach to the same could be making a wrapping PropagationFactory like this: @Override public <C> Injector<C> injector(Setter<C, K> setter) {
return delegate.injector((carrier, key, value) -> {
if (key.equals("redacted key")) return;
setter.put(carrier, key, value);
});
} However, a bunch of wrapped things itself can get gnarly for the simple case, hence #849. |
@devinsba: the spring security requires an explicit delegation while the brave/sleuth tracking the transaction implicitly, even to cases in which are not spring aware. this is why using a single mechanism to propagate data in the scope of transaction is preferable. The Authentication object is authoritative object rather than informative. Of course we use this to set subject and authorities, however, it is not have event processing of assigning into new threads or when calling 3rd parties. |
This lets you use the extra field propagation mechanism to propagate in, but not out of process. This can be used to correlate properties with logs in a way that doesn't add network overhead. This design was chosen as it incurs very little in-process overhead and has the least burden on the code base. It can be used as a stop-gap until a more fully featured correlation context feature is developed. Fixes #929 See also #577
9a6daf3
to
8450f77
Compare
@devinsba PTAL I added a disclaimer |
This lets you use the extra field propagation mechanism to propagate in,
but not out of process. This can be used to correlate properties with
logs in a way that doesn't add network overhead.
This design was chosen as it incurs very little in-process overhead and
has the least burden on the code base. It can be used as a stop-gap
until a more fully featured correlation context feature is developed.
Fixes #929
See also #577