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

Upgrade to Brave 6 and Zipkin Reporter 3 #39049

Closed

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Jan 7, 2024

This updates to Brave 6 and Zipkin Reporter 3 which are compatible with the classpath of Otel 1.35

Here are some new features for end-users, applicable to both Otel and Brave senders:

  • Use protobuf by setting the property "management.zipkin.tracing.encoding" to "PROTO3"
  • Integrate a dynamic zipkin endpoint source by providing a bean HttpEndpointSupplier.Factory
    • This will allow spring-cloud-loadbalancer or discovery integration like sleuth had, but also for otel
    • The input to this function is ZipkinConnectionDetails.getSpanEndpoint()
    • When absent ZipkinConnectionDetails.getSpanEndpoint() is called once like before.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 7, 2024
Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @codefromthecrypt.

@codefromthecrypt
Copy link
Contributor Author

@wilkinsona I think we can get away from all these deprecated things, sorry about the distraction. I'll mark draft and triple check!

@codefromthecrypt codefromthecrypt marked this pull request as draft January 8, 2024 12:01
@codefromthecrypt codefromthecrypt force-pushed the brave-5.17.1 branch 2 times, most recently from 1f14b97 to 9ff349b Compare January 9, 2024 03:49
@codefromthecrypt codefromthecrypt changed the title deps: bumps to Brave 5.17.1 and moves off internal type deps: bumps to Brave 6.0.0 and moves off internal type Jan 9, 2024
@codefromthecrypt codefromthecrypt marked this pull request as ready for review January 9, 2024 03:54
@codefromthecrypt
Copy link
Contributor Author

should be an easier review, now. I had to add the zipkin-reporter bom not because of code in this change, but because Brave 6 no longer manages zipkin-reporter (Brave 6 has no dependency on zipkin types). spring-boot-actuator-autoconfigure defines reporter types, so needs this for those to compile. When that code updates to Zipkin Reporter 3 is up to you, I'm fine helping soon, but if not this week I can't promise availability as this stuff isn't related to my dayjob anymore.

@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Jan 9, 2024

actually update to zipkin-reporter v3 will need to wait until this is released, as it seems boot is also importing otel exporter for OpenTelemetryConfiguration (note brave doesn't actually use this type). Anyway, migration to v3 is changing packages, so anyone can do it similar to this openzipkin/zipkin-aws#208

@wilkinsona
Copy link
Member

We can't upgrade to Brave 6.0 until Micrometer has done so.

@wilkinsona wilkinsona added status: blocked An issue that's blocked on an external project change and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 9, 2024
@wilkinsona wilkinsona changed the title deps: bumps to Brave 6.0.0 and moves off internal type Upgrade to Brave 6.0.0 Jan 9, 2024
@wilkinsona wilkinsona added the type: dependency-upgrade A dependency upgrade label Jan 9, 2024
@efemoney

This comment was marked as off-topic.

@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Feb 9, 2024

@efemoney I think the current status is micrometer are awaiting https://github.com/open-telemetry/opentelemetry-java to release 1.35.0. Then micrometer can update to that and brave 6 etc per notes here. Once micrometer is converged, then this one can progress. Finally, we can basically not do this again for several years :)

TL;DR; waiting on otel to release and I don't know the schedule of that

@efemoney
Copy link

efemoney commented Feb 9, 2024

Thank you. I checked the micrometer PR but somehow missed that 🙌🏾

@codefromthecrypt
Copy link
Contributor Author

turns out micrometer-metrics/tracing#536 only affected test deps as there was no main code change. So, there's no inter-dependency afaict except maybe if otel latest is needed here. Anyway I will sort this out next!

@codefromthecrypt codefromthecrypt changed the title Upgrade to Brave 6.0.0 Migrate to Brave 6 and Zipkin Reporter 3 Feb 10, 2024
@codefromthecrypt
Copy link
Contributor Author

ok there's probably a lint or some test glitch, but I'm time out for the day.

Effectively, we don't want to depend on zipkin core (zipkin.Reporter) types anymore, so detecting Brave already configured if there is an AsyncZipkinSpanHandler rather wrapping and bringing back a zipkin dep with AsyncReporter

Also, almost everyone uses json and there's no built-in set of encoders in brave. So, I used optional injection of an encoder for both use cases brave and otel. The only known custom user of that encoder would be zipkin-gcp, and again lacking any built-in choice except json, optional is the best way.

Feedback welcome, but I think you can see where I'm going with this and it is a lot simpler to implement.

@mhalbritter
Copy link
Contributor

Thanks a lot for the PR, there are some cool features in there! :)

@codefromthecrypt
Copy link
Contributor Author

rebased with feedback from @mhalbritter accepted! 🤞 on green

@codefromthecrypt
Copy link
Contributor Author

red looks unrelated unless I'm missing something

Adrian Cole added 5 commits February 20, 2024 07:30
Signed-off-by: Adrian Cole <[email protected]>
Signed-off-by: Adrian Cole <[email protected]>
Signed-off-by: Adrian Cole <[email protected]>
Signed-off-by: Adrian Cole <[email protected]>
@codefromthecrypt
Copy link
Contributor Author

rebased. let me know what else is needed here!

@codefromthecrypt
Copy link
Contributor Author

same snapshot download error as before..

@mhalbritter mhalbritter removed the status: blocked An issue that's blocked on an external project change label Feb 20, 2024
@mhalbritter mhalbritter changed the title Migrate to Brave 6 and Zipkin Reporter 3 Upgrade to Brave 6 and Zipkin Reporter 3 Feb 20, 2024
@mhalbritter
Copy link
Contributor

Don't worry. I'll take a look at the PR and merge it. It builds fine on my machine.

mhalbritter pushed a commit that referenced this pull request Feb 20, 2024
mhalbritter added a commit that referenced this pull request Feb 20, 2024
@mhalbritter
Copy link
Contributor

Thanks a lot for the PR!

@mhalbritter mhalbritter added this to the 3.3.0-M2 milestone Feb 20, 2024
@mhalbritter mhalbritter self-assigned this Feb 20, 2024
@codefromthecrypt
Copy link
Contributor Author

thanks @mhalbritter .. one less thing on my nag list, but more importantly a big step forward for those using this stuff. cheers!

@codefromthecrypt codefromthecrypt deleted the brave-5.17.1 branch February 20, 2024 22:39
mhalbritter added a commit that referenced this pull request Feb 21, 2024
Brave can work without zipkin2 on the classpath, OpenTelemetry can't.
To not force users to have zipkin2 on the classpath, move it into the
OpenTelemetry auto-configuration.

See gh-39049
izeye added a commit to izeye/spring-boot that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: dependency-upgrade A dependency upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants