Moves Slf4jScopeDecorator to use CorrelationField internally (#1597)

benchmarks/pom.xml

@@ -34,7 +34,7 @@
 		<maven.compiler.target>1.8</maven.compiler.target>
 		<maven.compiler.source>1.8</maven.compiler.source>
 		<spring-boot.version>2.1.10.RELEASE</spring-boot.version>
-		<brave.version>5.11.0</brave.version>
+		<brave.version>5.11.1</brave.version>
 		<okhttp.version>3.11.0</okhttp.version>
 	</properties>
 

pom.xml

@@ -264,7 +264,7 @@
 		<spring-cloud-stream.version>Fishtown.SR4</spring-cloud-stream.version>
 		<spring-cloud-netflix.version>2.1.6.BUILD-SNAPSHOT</spring-cloud-netflix.version>
 		<spring-cloud-openfeign.version>2.1.6.BUILD-SNAPSHOT</spring-cloud-openfeign.version>
-		<brave.version>5.11.0</brave.version>
+		<brave.version>5.11.1</brave.version>
 		<spring-security-boot-autoconfigure.version>2.1.2.RELEASE
 		</spring-security-boot-autoconfigure.version>
 

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/log/Slf4jScopeDecorator.java

@@ -22,6 +22,7 @@ import java.util.TreeSet;
 
 import brave.baggage.BaggageField;
 import brave.baggage.BaggageFields;
+import brave.baggage.CorrelationField;
 import brave.baggage.CorrelationScopeDecorator;
 import brave.context.slf4j.MDCScopeDecorator;
 import brave.propagation.CurrentTraceContext.Scope;
@@ -45,34 +46,43 @@ final class Slf4jScopeDecorator implements ScopeDecorator {
 
 	// Backward compatibility for all logging patterns
 	private static final ScopeDecorator LEGACY_IDS = MDCScopeDecorator.newBuilder()
-			.clear().addField(BaggageFields.TRACE_ID, "X-B3-TraceId")
+			.clear()
-			.addField(BaggageFields.PARENT_ID, "X-B3-ParentSpanId")
+			.addField(CorrelationField.newBuilder(BaggageFields.TRACE_ID)
-			.addField(BaggageFields.SPAN_ID, "X-B3-SpanId")
+					.name("X-B3-TraceId").build())
-			.addField(BaggageFields.SAMPLED, "X-Span-Export").build();
+			.addField(CorrelationField.newBuilder(BaggageFields.PARENT_ID)
+					.name("X-B3-ParentSpanId").build())
+			.addField(CorrelationField.newBuilder(BaggageFields.SPAN_ID)
+					.name("X-B3-SpanId").build())
+			.addField(CorrelationField.newBuilder(BaggageFields.SAMPLED)
+					.name("X-Span-Export").build())
+			.build();
 
 	private final ScopeDecorator delegate;
 
 	Slf4jScopeDecorator(SleuthProperties sleuthProperties,
 			SleuthSlf4jProperties sleuthSlf4jProperties) {
 		CorrelationScopeDecorator.Builder builder = MDCScopeDecorator.newBuilder().clear()
-				.addField(BaggageFields.TRACE_ID).addField(BaggageFields.PARENT_ID)
+				.addField(CorrelationField.create(BaggageFields.TRACE_ID))
-				.addField(BaggageFields.SPAN_ID)
+				.addField(CorrelationField.create(BaggageFields.PARENT_ID))
-				.addField(BaggageFields.SAMPLED, "spanExportable");
+				.addField(CorrelationField.create(BaggageFields.SPAN_ID))
+				.addField(CorrelationField.newBuilder(BaggageFields.SAMPLED)
+						.name("spanExportable").build());
 
 		Set<String> whitelist = new TreeSet<>(String.CASE_INSENSITIVE_ORDER);
 		whitelist.addAll(sleuthSlf4jProperties.getWhitelistedMdcKeys());
 
+		// Note: we are adding all the keys as-is because correlation context doesn't
+		// prefix, only ExtraFieldPropagation does
 		Set<String> retained = new LinkedHashSet<>();
 		retained.addAll(sleuthProperties.getBaggageKeys());
 		retained.addAll(sleuthProperties.getPropagationKeys());
+		retained.retainAll(whitelist);
 
+		// For backwards compatibility set all fields dirty, so that any changes made by
+		// MDC directly are reverted.
 		for (String name : retained) {
-			if (whitelist.contains(name)) {
+			builder.addField(CorrelationField.newBuilder(BaggageField.create(name))
-				// Until we move off ExtraFieldPropagation onto BaggagePropagation,
+					.dirty().build());
-				// manually create the fields...
-				builder.addField(BaggageField.create(name));
-				builder.addDirtyName(name);
-			}
 		}
 
 		this.delegate = builder.build();

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/log/Slf4JSpanLoggerTest.java

@@ -18,8 +18,10 @@ package org.springframework.cloud.sleuth.log;
 
 import brave.Span;
 import brave.Tracer;
+import brave.baggage.CorrelationField;
 import brave.propagation.CurrentTraceContext.Scope;
 import brave.propagation.ExtraFieldPropagation;
+import org.assertj.core.api.InstanceOfAssertFactories;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -40,7 +42,7 @@ import static org.assertj.core.api.Assertions.assertThat;
  */
 @RunWith(SpringRunner.class)
 @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.NONE, properties = {
-		"spring.sleuth.baggage-keys=my-baggage",
+		"spring.sleuth.baggage-keys=my-baggage,my-baggage-two",
 		"spring.sleuth.propagation-keys=my-propagation",
 		"spring.sleuth.log.slf4j.whitelisted-mdc-keys=my-baggage,my-propagation" })
 @SpringBootConfiguration
@@ -148,4 +150,54 @@ public class Slf4JSpanLoggerTest {
 		assertThat(MDC.get("traceId")).isEqualTo("A");
 	}
 
+	// #1416
+	@Test
+	public void should_clear_any_mdc_entries_when_their_keys_are_whitelisted()
+			throws Exception {
+
+		Scope scope = this.slf4jScopeDecorator.decorateScope(this.span.context(), () -> {
+		});
+
+		MDC.put("my-baggage", "A");
+		MDC.put("my-propagation", "B");
+
+		assertThat(MDC.get("my-baggage")).isEqualTo("A");
+		assertThat(MDC.get("my-propagation")).isEqualTo("B");
+
+		scope.close();
+
+		assertThat(MDC.get("my-baggage")).isNullOrEmpty();
+		assertThat(MDC.get("my-propagation")).isNullOrEmpty();
+	}
+
+	@Test
+	public void should_only_include_whitelist() {
+		assertThat(this.slf4jScopeDecorator).extracting("delegate.fields")
+				.asInstanceOf(InstanceOfAssertFactories.array(CorrelationField[].class))
+				// my-baggage-two is baggage not in the whitelist
+				.extracting(CorrelationField::name).containsExactly("traceId", "parentId",
+						"spanId", "spanExportable", "my-baggage", "my-propagation");
+	}
+
+	@Test
+	public void should_pick_previous_mdc_entries_when_their_keys_are_whitelisted() {
+
+		MDC.put("my-baggage", "A1");
+		MDC.put("my-propagation", "B1");
+
+		Scope scope = this.slf4jScopeDecorator.decorateScope(this.span.context(), () -> {
+		});
+
+		MDC.put("my-baggage", "A2");
+		MDC.put("my-propagation", "B2");
+
+		assertThat(MDC.get("my-baggage")).isEqualTo("A2");
+		assertThat(MDC.get("my-propagation")).isEqualTo("B2");
+
+		scope.close();
+
+		assertThat(MDC.get("my-baggage")).isEqualTo("A1");
+		assertThat(MDC.get("my-propagation")).isEqualTo("B1");
+	}
+
 }

spring-cloud-sleuth-dependencies/pom.xml

@@ -31,7 +31,7 @@
 	<name>spring-cloud-sleuth-dependencies</name>
 	<description>Spring Cloud Sleuth Dependencies</description>
 	<properties>
-		<brave.version>5.11.0</brave.version>
+		<brave.version>5.11.1</brave.version>
 		<brave.opentracing.version>0.33.13</brave.opentracing.version>
 		<grpc.spring.boot.version>3.0.1</grpc.spring.boot.version>
 	</properties>