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.

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

Merged
merged 5 commits into from
Jul 3, 2019

Conversation

codefromthecrypt
Copy link
Member

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

@@ -143,25 +150,20 @@ public FactoryBuilder addPrefixedFields(String prefix, Collection<String> fieldN
}

public Factory build() {
if (prefixedNames.isEmpty()) {
Copy link
Member Author

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);
Copy link
Member Author

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.

Copy link
Member

@devinsba devinsba left a 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

@codefromthecrypt
Copy link
Member Author

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.

@alonbl
Copy link

alonbl commented Jun 26, 2019

@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.

Adrian Cole added 4 commits July 2, 2019 09:30
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
@codefromthecrypt
Copy link
Member Author

@devinsba PTAL I added a disclaimer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Store misc information within TraceContext which are not propagated
4 participants