Added support for multiple reporters #1181

TYsewyn · 2019-01-13T16:16:51Z

Targets issue #1180.

codecov-io · 2019-01-13T16:27:46Z

Codecov Report

Merging #1181 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1181      +/-   ##
============================================
+ Coverage     69.82%   69.97%   +0.14%     
- Complexity      773      775       +2     
============================================
  Files           142      142              
  Lines          3526     3530       +4     
  Branches        387      388       +1     
============================================
+ Hits           2462     2470       +8     
+ Misses          841      836       -5     
- Partials        223      224       +1

Impacted Files	Coverage Δ	Complexity Δ
...ipkin2/sender/ZipkinRabbitSenderConfiguration.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
.../sender/ZipkinRestTemplateSenderConfiguration.java	`55.88% <ø> (ø)`	`3 <0> (ø)`	⬇️
...zipkin2/sender/ZipkinKafkaSenderConfiguration.java	`78.57% <ø> (ø)`	`4 <0> (ø)`	⬇️
...loud/sleuth/autoconfig/TraceAutoConfiguration.java	`96.22% <100%> (+0.3%)`	`23 <1> (+2)`	⬆️
.../cloud/sleuth/zipkin2/ZipkinAutoConfiguration.java	`100% <100%> (ø)`	`5 <1> (ø)`	⬇️
...ing/TracingConnectionFactoryBeanPostProcessor.java	`73.21% <0%> (+3.57%)`	`6% <0%> (ø)`	⬇️

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 89ca7d7...8327ecb. Read the comment docs.

codecov-io · 2019-01-13T16:27:46Z

Codecov Report

Merging #1181 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master   #1181      +/-   ##
===========================================
+ Coverage     69.78%   69.8%   +0.02%     
- Complexity      772     773       +1     
===========================================
  Files           142     142              
  Lines          3518    3521       +3     
  Branches        384     385       +1     
===========================================
+ Hits           2455    2458       +3     
  Misses          841     841              
  Partials        222     222

Impacted Files	Coverage Δ	Complexity Δ
...ipkin2/sender/ZipkinRabbitSenderConfiguration.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
.../sender/ZipkinRestTemplateSenderConfiguration.java	`55.88% <ø> (ø)`	`3 <0> (ø)`	⬇️
.../cloud/sleuth/zipkin2/ZipkinAutoConfiguration.java	`100% <ø> (ø)`	`5 <0> (ø)`	⬇️
...zipkin2/sender/ZipkinKafkaSenderConfiguration.java	`78.57% <ø> (ø)`	`4 <0> (ø)`	⬇️
...loud/sleuth/autoconfig/TraceAutoConfiguration.java	`96.07% <100%> (+0.24%)`	`21 <0> (+1)`	⬆️

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 218dd6c...07d2aa8. Read the comment docs.

marcingrzejszczak · 2019-01-14T16:47:39Z

...h-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin2/ZipkinAutoConfiguration.java

 	 * also replace with a standard one.
 	 */
-	@Bean
+	@Bean("zipkinReporter")


maybe extract to some constant?

marcingrzejszczak · 2019-01-14T16:47:46Z

...ain/java/org/springframework/cloud/sleuth/zipkin2/sender/ZipkinKafkaSenderConfiguration.java

 	private String topic;

-	@Bean
+	@Bean("zipkinSender")


maybe extract to some constant?

marcingrzejszczak · 2019-01-14T16:47:54Z

...in/java/org/springframework/cloud/sleuth/zipkin2/sender/ZipkinRabbitSenderConfiguration.java

 	private String queue;

-	@Bean
+	@Bean("zipkinSender")


maybe extract to some constant?

marcingrzejszczak · 2019-01-14T16:49:08Z

...kin/src/test/java/org/springframework/cloud/sleuth/zipkin2/ZipkinAutoConfigurationTests.java

 	}

+	@Test
+	public void supportsMultipleReporters() {


Can you report a span and verify that both senders and reporters were actually called?

devinsba · 2019-01-15T04:44:29Z

I'm worried that this will result in a hidden mis-configuration. I think this feature should require a flag

IE: Maybe I bring in 2 different starters that have Reporters in them and neither of them with @ConditionalOnMissingBean

devinsba · 2019-01-15T04:46:48Z

...h-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin2/ZipkinAutoConfiguration.java

 	@ConditionalOnMissingBean
 	public Reporter<Span> reporter(ReporterMetrics reporterMetrics,
-			ZipkinProperties zipkin, Sender sender, BytesEncoder<Span> spanBytesEncoder) {
+			ZipkinProperties zipkin, @Qualifier(SENDER_BEAN_NAME) Sender sender,


I think this needs a doc update associated with it since now overriding requires the correct bean name for a Sender

This whole change needs a doc update because the overall configuration has been updated to support multiple tracers.
I already made the change for spring-cloud-gcp-trace (spring-attic/spring-cloud-gcp#1383).

marcingrzejszczak · 2019-01-15T08:19:39Z

@devinsba

IE: Maybe I bring in 2 different starters that have Reporters in them and neither of them with @ConditionalOnMissingBean

Maybe I don't get it yet, but if neither is @ConditionalOnMissingBean we will get two reporters that are being used. I guess that's the point?

I'm worried that this will result in a hidden mis-configuration. I think this feature should require a flag

I'm for making this opt in - that makes sense.

TYsewyn · 2019-01-15T08:21:39Z

I'm for making this opt in - that makes sense.

Honestly I do not agree, this shouldn't be configured with a property.
If I put 2 separate tracing systems intentionally on my classpath, both should work out of the box without adding more custom configuration.

marcingrzejszczak · 2019-01-15T08:28:29Z

Honestly I do not agree, this shouldn't be configured with a property.
If I put 2 separate tracing systems intentionally on my classpath, both should work out of the box without adding more custom configuration.

Fair enough, makes sense. So let's make it opt out :)

…ation` so other implementations can use the same default

TYsewyn · 2019-01-15T16:41:42Z

Fair enough, makes sense. So let's make it opt out :)

opt-out can be done using eg. spring.zipkin.enabled=false or spring.cloud.gcp.trace.enabled=false :)

TYsewyn · 2019-01-16T11:44:35Z

Pinging @adriancole to have a second pair of eyes.
@marcingrzejszczak and I went through it and it looks good to us. :)

TYsewyn · 2019-01-16T12:26:08Z

I found out there is a breaking change, I'll make some adaptations accordingly.

…"zipkinSender"

…another tracing system extends from spring-cloud-sleuth-zipkin (like spring-cloud-gcp-trace)

…ation`

TYsewyn · 2019-01-16T21:19:59Z

That's weird... It's the second time that SleuthSpanCreatorAspectMonoTests is behaving flakey on CircleCI while everything is passing locally.
@marcingrzejszczak @adriancole can any of you verify this?

codefromthecrypt · 2019-01-21T03:22:28Z

...h-core/src/main/java/org/springframework/cloud/sleuth/autoconfig/TraceAutoConfiguration.java


-	private Reporter<zipkin2.Span> adjustedReporter(Reporter<zipkin2.Span> delegate) {
+	private Reporter<zipkin2.Span> compositeReporter() {
 		return (span) -> {


optional: I would pull this into a static or package private class as it isn't debug friendly. a toString would be nice, and it would also be nice to not have to loop for the common case where there is only one reporter. Another optional would be to catch exceptions in the loop such that one report failure doesn't fail others.

codefromthecrypt · 2019-01-21T03:23:32Z

I approved not as an overruling thing but based on my understanding of things. Those using sleuth should have a stronger opinion

marcingrzejszczak · 2019-01-21T09:41:09Z

Fixed via 490a1d8 and other commits by @TYsewyn

TYsewyn added 2 commits January 13, 2019 17:00

Added support for multiple span reporters

64f24a5

Updated zipkin config to support multiple span reporters/senders

07d2aa8

marcingrzejszczak reviewed Jan 14, 2019

View reviewed changes

marcingrzejszczak added the enhancement label Jan 14, 2019

marcingrzejszczak added this to the 2.1.0.RELEASE milestone Jan 14, 2019

Incorporated PR feedback

5c05186

devinsba reviewed Jan 15, 2019

View reviewed changes

TYsewyn added 2 commits January 15, 2019 17:21

No reason to create a SpanBytesEncoder bean

92c9095

Moved ReporterMetrics sleuthReporterMetrics() to `TraceAutoConfigur…

d7eccee

…ation` so other implementations can use the same default

TYsewyn mentioned this pull request Jan 15, 2019

Updated the auto-configuration to support multiple tracing systems spring-attic/spring-cloud-gcp#1383

Merged

TYsewyn added 5 commits January 15, 2019 22:25

Updated license header

ca2244d

Added @ConditionalOnMissingBean(name = REPORTER_BEAN_NAME)

14700ed

Merge branch 'master' into feature/multiple-reporters

9cb5e27

Updated ZipkinAutoConfiguration for backwards compatibility

7c31475

Updated documentation

8327ecb

TYsewyn added 4 commits January 16, 2019 16:57

Create the restTemplateSender bean when there is no other bean named …

d8381b8

…"zipkinSender"

Adapted new configuration to support backwards compatibility in case …

453d942

…another tracing system extends from spring-cloud-sleuth-zipkin (like spring-cloud-gcp-trace)

Updated conditional for ZipkinBackwardsCompatibilityAutoConfiguration

3b47f77

Updated access modifiers in `ZipkinBackwardsCompatibilityAutoConfigur…

490a1d8

…ation`

codefromthecrypt approved these changes Jan 21, 2019

View reviewed changes

marcingrzejszczak merged commit 490a1d8 into spring-cloud:master Jan 21, 2019

marcingrzejszczak mentioned this pull request Jan 21, 2019

Support multiple tracing systems #1180

Closed

TYsewyn mentioned this pull request Jan 21, 2019

Fixed test spring-attic/spring-cloud-gcp#1387

Closed

TYsewyn deleted the feature/multiple-reporters branch January 21, 2019 17:35

devinsba mentioned this pull request Jan 23, 2019

Disabling Zipkin breaks start up #1191

Closed

Added support for multiple reporters #1181

Added support for multiple reporters #1181

Uh oh!

Conversation

TYsewyn commented Jan 13, 2019

Uh oh!

codecov-io commented Jan 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov-io commented Jan 13, 2019

Codecov Report

Uh oh!

marcingrzejszczak Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

marcingrzejszczak Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

marcingrzejszczak Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

marcingrzejszczak Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

devinsba commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devinsba Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

TYsewyn Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

marcingrzejszczak commented Jan 15, 2019

Uh oh!

TYsewyn commented Jan 15, 2019

Uh oh!

marcingrzejszczak commented Jan 15, 2019

Uh oh!

TYsewyn commented Jan 15, 2019

Uh oh!

TYsewyn commented Jan 16, 2019

Uh oh!

TYsewyn commented Jan 16, 2019

Uh oh!

TYsewyn commented Jan 16, 2019

Uh oh!

codefromthecrypt Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented Jan 21, 2019

Uh oh!

marcingrzejszczak commented Jan 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jan 13, 2019 •

edited

Loading

devinsba commented Jan 15, 2019 •

edited

Loading