Configurable extra field propagation behavior

[budaimartin]

Feature Extra field propagation behavior is configurable such that

• what to do in case of field collision (the request has already contain the field during injection)
• allow/block list for downstream services.

Rational Currently, extra field propagation can be customized with ExtraFieldCustomizer, but that is unaware of the context. It would be reasonable to refine extra field propagation behavior based on the request context. For example which host the extra field is about to passed to, and if there is anything in the current request context that might affect propagation.

Example Scenarios

Scenario 1

Across a system of microservices communicating over HTTP, there is a request header that identifies the client interacting with it (mobile app, desktop, mobile web, etc.). For tracing, this information is passed through all microservices in the system. However, one of the downstreams now require a header with the same name, but with a different value, so we add it manually during the request. Currently, when the same header is also added manually, brave.http.HttpClientRequest simply appends the one coming from the upstream and both values will be sent. There is no way to mitigate this, but to redact this header from all downstreams of the given microservice. In this case, it would be good if:

• the header was rather be skipped from propagation for that particular downstream automatically (zipkin notices that the header is already present in the request) OR
• the header could be excluded from propagation only for that particular downstream, achieved with a special, context-aware ExtraFieldCustomizer.

Scenario 2

In a system, extra field propagation is used for passing some data about the user in every call. This might contain sensitive data, which we wouldn't like to pass to 3rd party downstreams. Since ExtraFieldCustomizer is unaware of the request context, we can only redact the fields for the whole service, so not just that particular 3rd party, but all other downstream services won't receive these fields.

[codefromthecrypt]

Hi, thanks for the issue. For the first pass, do you mind reviewing BaggagePropagation as it is not deprecated as ExtraFieldCustomizer is. It has some options including redaction built-in and might help narrow down your concerns to a smaller feature or request.

You can also take a peek at a custom request-aware propagation system we designed here, but please review the basic one first as it might be overkill. https://github.com/openzipkin-contrib/zipkin-secondary-sampling/blob/master/brave/src/main/java/brave/secondary_sampling/SecondarySamplingExtractor.java

[budaimartin]

Hi @adriancole,

Thanks for the quick response and your suggestions.

Currently, I use baggage for the issue described in the first point. However, It prefixes the header name, so a different logic is needed on the edge layer and internal services (e.g. on edge layer, the header name is something like ClientId, which is propagated to internal services as baggage-ClientId and baggage_ClientId). I tried using extra field propagation via Spring Cloud Sleuth's spring.sleuth.propagation-keys property, but encountered with the problem described above: if I also set ClientId manually, both that and the one coming from upstream is appended. It would be ideal if instead of appending the header value, it rather be just skipped, so we could use a centralized solution for this kind of tracing.

All-in-all, I think this feature request could be narrowed down to having a possibility to configure extra field injection to ignore already existing fields in the carrier.

[codefromthecrypt]

if a setter is appending and not replacing a header, that's a bug. The extra prefixing in sleuth is only when you use certain properties, and that has really nothing to do with Brave as it doesn't do any prefixing.

I'm going to move this to the support org for now, as it doesn't seem like a code change will occur in brave.

First you need to identify what is causing the duplicate header value as brave.http.HttpClientRequest is a base type. It doesn't implement setting of headers and should absolutely be implemented to overwrite, not append. Sleuth uses some instrumentation that are not in this repository, so it is important to know which you are talking about. We can however, add a test case here to ensure that things are overwritten, not appended, but there are also instrumentation outside this repository who never ran, disabled or deleted our test cases.

Secondly, the properties you are discussing are a discussion to have in sleuth. Particularly, they are optional, and ExtraFieldPropagation type is deprecated. Brave is a library used by sleuth, but we cannot control how it uses that library directly. In other words, something about properties would currently be something to discuss with sleuth.

The only thing to discuss in Brave is whether or not to double-check existing value before setting something. I would encourage you to take the base case which is assume headers are never appended to, rather overwritten. Ex if there was no bug would you still ask for this? Then, consider basically all other implementations of tracing would also overwrite anything you set into baggage blindly. Is your primary goal to have inconsistent client-id values across the trace? In that case, we can chat further.. if you are just trying to prevent double-header addition. please focus on that knowing it is a bug.

[codefromthecrypt]

PS the BaggagePropagation tool was literally written to allow value change to be different on a per-field basis. There is something here still which I wrote when still employed by spring. I know sleuth 3 is changing still particularly property names, but the Brave apis mentioned are stable https://github.com/spring-cloud/spring-cloud-sleuth/wiki/Spring-Cloud-Sleuth-3.0-Migration-Guide#new-features

[budaimartin]

During debugging my service, I ended up in brave.http.HttpClientRequest, where the request.header() call appended a header. Actually, it did a very weird thing afterwards, but I didn't debug any further. We have a feign request interceptor that sets Client-Id to let's say A. If the service receives Client-Id value B to propagate, then in the downstream, it ends up [A, B, A, B]. If you think this is a bug in Brave, I can pull up a PoC project for you, and/or open a different issue.

However, I can argue for all three behaviors, hence my idea was to make it rather configurable.

• Addition is fine, as headers are multi-value maps; this way, no value is lost, both propagated to downstream
• Overwriting is fine, as the header value is set to be propagated; extra field propagation is context-unaware, so the user must make sure not to intend to use that header for anything else
• Skipping is fine, as the user may explicitly set that header to a different value, and extra field propagation can be understood as an implicit act, hence should be lower prio

I was hesitating that this is a question for you (Brave) or Splring Cloud Sleuth. Sorry if I chose the wrong door to knock in. If you think the feature request is for them, I'm happy to close this issue and ask there.

[codefromthecrypt]

There is no feign instrumentation at the moment, so I think you are referring to what's in sleuth, which indeed has a bug and it isn't running our tests anyway so wouldn't find it https://github.com/spring-cloud/spring-cloud-sleuth/blob/2.2.x/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java#L157

^^ is not setting the header but conditionally doing so.

headers in HTTP have to be explicitly permitted to be multi-value, and almost no HTTP header used in tracing would sensibly do so. Even when they are multi-value, there are combination rules and are not forbidden to be combined into a single value.

I don't know how our javadoc on the remote setter could be more clear on this..

     * Replaces a propagation field with the given value.
     *
     * <p><em>Note</em>: Implementations attempt to overwrite all values. This means that when the
     * caller is encoding multi-value (comma-separated list) HTTP header, they MUST join all values
     * on comma into a single string.
     *
     * @param request see {@link #<R>}
     * @param fieldName typically a header name
     * @param value non-{@code null} value to replace any values with

regardless, the main problem is a bug in the feign code inside sleuth where instead of overwriting the header it is trying to be smart.

Skipping isn't ok as it leads to inconsistent behavior, even if you can probably try to hack to make that happen I wouldn't advise it.

[codefromthecrypt]

Here's a hint on fixing that btw (from the creator of feign heh), make a copy of this test class and do some find/replace to make it work with sleuth's feign RequestWrapper. At the bottom of this test is one on headers. you can add another test case to prove things aren't trying to be smart and conditionally set https://github.com/openzipkin/brave/blob/master/instrumentation/jaxrs2/src/test/java/brave/jaxrs2/ClientRequestContextWrapperTest.java

[codefromthecrypt]

meanwhile it is probably a good idea to see if the openfeign org is open to hosting a cleaned up copy of that logic, as it could have tests run like this which would find a bunch of bugs as it would actually invoke the tests including things with baggage setup https://github.com/openzipkin/brave/blob/master/instrumentation/jaxrs2/src/test/java/brave/jaxrs2/ITTracingJaxRSClientBuilder.java

best luck and do stop by https://gitter.im/openzipkin/zipkin if you have more questions!