Support Extra Propagation Fields with Trace #2290

meltsufin · 2020-03-27T15:30:47Z

Providing a Trace Propagation Factory Builder instead of just the Factory,
which allows us to rely on the TraceAutoConfiguration.sleuthPropagation()
rather than completely skipping it.

Fixes: #2268.

Providing a Trace Propagation Factory Builder instead of just the Factory, while allows us to rely on the `TraceAutoConfiguration.sleuthPropagation()` rather than completely skipping it. Fixes: #2268.

meltsufin · 2020-03-27T15:34:55Z

@adriancole This almost works except that when there are no extra propagation fields, we get this exception:

java.lang.NullPointerException: fieldNames is empty
	at brave.internal.PredefinedPropagationFields.<init>(PredefinedPropagationFields.java:28)
	at brave.propagation.ExtraFieldPropagation$Extra.<init>(ExtraFieldPropagation.java:464)
	at brave.propagation.ExtraFieldPropagation$ExtraFactory.create(ExtraFieldPropagation.java:450)
	at brave.propagation.ExtraFieldPropagation$ExtraFieldExtractor.extract(ExtraFieldPropagation.java:414)
	at brave.http.HttpServerHandler.handleReceive(HttpServerHandler.java:109)
	at brave.servlet.TracingFilter.doFilter(TracingFilter.java:74)
	at org.springframework.cloud.sleuth.instrument.web.LazyTracingFilter.doFilter(TraceWebServletAutoConfiguration.java:138)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
	at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:201)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
	at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:202)
	at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:96)
	at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:541)
	at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:139)
	at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:92)
	at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74)
	at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:343)
	at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:367)
	at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:65)
	at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:868)
	at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1639)
	at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
	at java.base/java.lang.Thread.run(Thread.java:834)

I think we should just be providing a general Propagation.FactoryBuilder if that existed, rather than the specific ExtraFieldPropagation.FactoryBuilder. What do you think?

meltsufin · 2020-03-27T15:42:53Z

@adriancole Ultimately, what we're after is the ability to configure the delegate for TraceAutoConfiguration, which is currently hardcoded as B3Propagation.FACTORY. I'm open to whatever would be the best way to do it. It will probably require some tweaks to TraceAutoConfiguration in Sleuth though.

codefromthecrypt · 2020-03-28T00:09:30Z

the exception will go away with a new brave release which will immediately go into the next sleuth release.

openzipkin/brave#1128

meltsufin · 2020-03-28T13:06:46Z

@adriancole Thanks for such a quick turnaround!

spring-cloud-gcp-dependencies/pom.xml

codecov · 2020-05-12T20:35:44Z

Codecov Report

Merging #2290 into master will decrease coverage by 7.19%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #2290      +/-   ##
============================================
- Coverage     81.13%   73.94%   -7.20%     
+ Complexity     2319     2098     -221     
============================================
  Files           260      260              
  Lines          7575     7575              
  Branches        785      785              
============================================
- Hits           6146     5601     -545     
- Misses         1103     1614     +511     
- Partials        326      360      +34

Flag	Coverage Δ	Complexity Δ
#integration	`?`	`?`
#unittests	`73.94% <100.00%> (ø)`	`2098.00 <1.00> (ø)`

Impacted Files	Coverage Δ	Complexity Δ
...igure/trace/StackdriverTraceAutoConfiguration.java	`72.30% <100.00%> (ø)`	`12.00 <1.00> (ø)`
...gcp/secretmanager/SecretManagerPropertySource.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-4.00%)`
...a/spanner/repository/query/SpannerQueryMethod.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-6.00%)`
...retmanager/SecretManagerPropertySourceLocator.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-2.00%)`
...figure/config/GcpConfigBootstrapConfiguration.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-2.00%)`
...e/spanner/GcpSpannerEmulatorAutoConfiguration.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-2.00%)`
...epository/config/SpannerRepositoriesRegistrar.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-3.00%)`
...ository/config/DatastoreRepositoriesRegistrar.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-3.00%)`
...restore/GcpFirestoreEmulatorAutoConfiguration.java	`0.00% <0.00%> (-84.85%)`	`0.00% <0.00%> (-4.00%)`
...ramework/cloud/gcp/vision/DocumentOcrTemplate.java	`17.64% <0.00%> (-73.53%)`	`4.00% <0.00%> (-5.00%)`
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fdb7e3...5ad71b8. Read the comment docs.

sonarcloud · 2020-05-12T21:11:18Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
4 Code Smells

100.0% Coverage
0.0% Duplication

meltsufin · 2020-05-12T21:28:51Z

Looks like TraceSampleApplicationTests fails as soon as we upgrade spring-cloud-sleuth to 2.2.3.BUILD-SNAPSHOT. 😞

elefeint · 2020-05-12T21:43:45Z

I can look into it Thursday -- I've had a lot of tracing on my mind lately.

meltsufin · 2020-05-18T16:17:32Z

We should rework this PR based on spring-cloud/spring-cloud-sleuth#1647 as well.

elefeint · 2020-05-18T19:03:51Z

My first thought was that we needed a newer brave, but sleuth snapshot already pulls in the latest...

elefeint · 2020-05-18T19:15:32Z

Ah. HexCodec got moved into a subpackage in openzipkin/brave#1193.

We need to update https://github.com/openzipkin/zipkin-gcp/blob/8b2462c054737259bdd707de49da0cc32aaa6806/propagation-stackdriver/src/main/java/zipkin2/propagation/stackdriver/XCloudTraceContextExtractor.java

meltsufin · 2020-05-18T19:49:11Z

See: openzipkin/zipkin-gcp#175

codefromthecrypt · 2020-05-19T00:50:44Z

zipkin-gcp 0.17.0 is on the way now. shading the internal HexCodec thing so it should be safe.

Notably, this fixes the propagation package also, though former is still there. brave.propagation instead of zipkin2.propagation

Finally, this allows you to control the underlying B3PropagationFactory, as mentioned
spring-cloud/spring-cloud-sleuth#1647

One subtle difference 2.2.x to 3.x is the change in the primary inject format.

2.2.x is

return BaggagePropagation.newFactoryBuilder(B3Propagation.newFactoryBuilder()
                         .injectFormat(B3Propagation.Format.MULTI).build());

3.0 is

return BaggagePropagation.newFactoryBuilder(B3Propagation.newFactoryBuilder()
                         .injectFormat(B3Propagation.Format.SINGLE_NO_PARENT).build());

...ava/org/springframework/cloud/gcp/autoconfigure/trace/StackdriverTraceAutoConfiguration.java

...g-cloud-gcp-samples/spring-cloud-gcp-trace-sample/src/test/java/brave/internal/HexCodec.java

meltsufin · 2020-05-19T14:02:00Z

@adriancole Can you please do a final review of this PR to make sure I configured the propagation factory correctly. Thanks!

...ava/org/springframework/cloud/gcp/autoconfigure/trace/StackdriverTraceAutoConfiguration.java

Support Extra Propagation Fields with Trace

6f555df

Providing a Trace Propagation Factory Builder instead of just the Factory, while allows us to rely on the `TraceAutoConfiguration.sleuthPropagation()` rather than completely skipping it. Fixes: #2268.

meltsufin added the trace label Mar 27, 2020

Update dependencies

11ae661

meltsufin requested a review from elefeint May 12, 2020 20:23

Merge branch 'master' into trace-extra-fields

b8df134

elefeint previously approved these changes May 12, 2020

View reviewed changes

spring-cloud-gcp-dependencies/pom.xml Outdated Show resolved Hide resolved

undo unrelated version updates

43908d8

meltsufin dismissed elefeint’s stale review via 43908d8 May 12, 2020 21:02

meltsufin added 3 commits May 18, 2020 18:11

Remove sampler auto-config

81302d5

temporarily add HexCodec

90e2f9f

switch to BaggagePropagation

1fd018d

codefromthecrypt reviewed May 19, 2020

View reviewed changes

...ava/org/springframework/cloud/gcp/autoconfigure/trace/StackdriverTraceAutoConfiguration.java Outdated Show resolved Hide resolved

codefromthecrypt reviewed May 19, 2020

View reviewed changes

...g-cloud-gcp-samples/spring-cloud-gcp-trace-sample/src/test/java/brave/internal/HexCodec.java Outdated Show resolved Hide resolved

meltsufin mentioned this pull request May 19, 2020

Upgrade to Spring Cloud Sleuth version that is Ilford GoogleCloudPlatform/spring-cloud-gcp#4

Closed

upgrade to zipkin-gcp 0.17.0

5ad71b8

meltsufin commented May 19, 2020

View reviewed changes

...ava/org/springframework/cloud/gcp/autoconfigure/trace/StackdriverTraceAutoConfiguration.java Show resolved Hide resolved

elefeint approved these changes May 19, 2020

View reviewed changes

meltsufin merged commit 514fcff into master May 19, 2020

meltsufin deleted the trace-extra-fields branch May 19, 2020 16:21

codefromthecrypt mentioned this pull request May 20, 2020

Make it possible to configure the "trace format" bean separately from baggage spring-cloud/spring-cloud-sleuth#1651

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Extra Propagation Fields with Trace #2290

Support Extra Propagation Fields with Trace #2290

meltsufin commented Mar 27, 2020 •

edited

meltsufin commented Mar 27, 2020

meltsufin commented Mar 27, 2020

codefromthecrypt commented Mar 28, 2020

meltsufin commented Mar 28, 2020

codecov bot commented May 12, 2020 •

edited

sonarcloud bot commented May 12, 2020

meltsufin commented May 12, 2020

elefeint commented May 12, 2020

meltsufin commented May 18, 2020

elefeint commented May 18, 2020

elefeint commented May 18, 2020

meltsufin commented May 18, 2020

codefromthecrypt commented May 19, 2020

meltsufin commented May 19, 2020

Support Extra Propagation Fields with Trace #2290

Support Extra Propagation Fields with Trace #2290

Conversation

meltsufin commented Mar 27, 2020 • edited

meltsufin commented Mar 27, 2020

meltsufin commented Mar 27, 2020

codefromthecrypt commented Mar 28, 2020

meltsufin commented Mar 28, 2020

codecov bot commented May 12, 2020 • edited

Codecov Report

sonarcloud bot commented May 12, 2020

meltsufin commented May 12, 2020

elefeint commented May 12, 2020

meltsufin commented May 18, 2020

elefeint commented May 18, 2020

elefeint commented May 18, 2020

meltsufin commented May 18, 2020

codefromthecrypt commented May 19, 2020

meltsufin commented May 19, 2020

meltsufin commented Mar 27, 2020 •

edited

codecov bot commented May 12, 2020 •

edited