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

Update dependency io.zipkin.reporter2:zipkin-reporter-bom to v3.2.1 #6151

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

codefromthecrypt
Copy link
Contributor

This migrates to the new BytesMessageSender base class used in zipkin-reporter. The old Sender is deprecated for removal.

Notably, this only supports synchronous invocation now. If you need async, you'll have to use your own reporting loop or otherwise.

@codefromthecrypt codefromthecrypt requested a review from a team January 14, 2024 09:34
@codefromthecrypt
Copy link
Contributor Author

obviates #6151

@codefromthecrypt codefromthecrypt changed the title Update dependency io.zipkin.reporter2:zipkin-reporter-bom to v3.2.0 Update dependency io.zipkin.reporter2:zipkin-reporter-bom to v3.2.1 Jan 14, 2024
@codefromthecrypt
Copy link
Contributor Author

@jack-berg fyi I think the link fail is due to sonatype migrating to a central portal. The markdown may need to be adjusted as there's no direct replacement for that 404 https://central.sonatype.org/register/central-portal/#and-publishing-is-easy

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e4b31ce) 91.00% compared to head (8383003) 90.99%.

Files Patch % Lines
...try/exporter/zipkin/ZipkinSpanExporterBuilder.java 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6151      +/-   ##
============================================
- Coverage     91.00%   90.99%   -0.02%     
  Complexity     5646     5646              
============================================
  Files           619      619              
  Lines         16443    16438       -5     
  Branches       1663     1663              
============================================
- Hits          14964    14957       -7     
- Misses         1017     1018       +1     
- Partials        462      463       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codefromthecrypt
Copy link
Contributor Author

raised #6152 to fix markdown

This migrates to the new `BytesMessageSender` base class used in
zipkin-reporter. The old `Sender` is deprecated for removal.

Notably, this only supports synchronous invocation now. If you need
async, you'll have to use your own reporting loop or otherwise.

Signed-off-by: Adrian Cole <[email protected]>
@codefromthecrypt
Copy link
Contributor Author

rebased

});
return result;
try {
sender.send(encodedSpans);
Copy link
Member

Choose a reason for hiding this comment

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

I believe this changes the implementation from being non-blocking to blocking. In practice, this is fine because BatchSpanProcess / SImpleSpanProcessor never calls SpanExporter#export concurrently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep it does make it explicitly blocking (depending on sender some would have sneakily blocked!) Thanks for the notes on impact

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

*/
public ZipkinSpanExporterBuilder setSender(Sender sender) {
@Deprecated
public ZipkinSpanExporterBuilder setSender(zipkin2.reporter.Sender sender) {
Copy link
Member

@jack-berg jack-berg Jan 17, 2024

Choose a reason for hiding this comment

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

The javadoc says that this will be removed in version 4. @open-telemetry/java-approvers we should figure out a plan with how we would like to handle this. If we bump to the next major version of zipkin, we wouldn't be able to support this API without a breaking change.

Might need to start publishing zipkin exporter artifacts which encode the major version of the zipkin dependency in the artifact name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW I'm also not sure when we would do a version 4, but the point remains. The upgrade analysis forced this point, but I think critical thinking could make it easier.

Reporter 3.x isn't compatible with 2.x senders anyway. You haven't yet released the 3.x code yet.... one could argue you could remove this deprecated thing now and then not have to worry about v4.

Copy link
Contributor Author

@codefromthecrypt codefromthecrypt Jan 17, 2024

Choose a reason for hiding this comment

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

for context micrometer and spring boot are waiting for 1.35.0, and that's the first version that has any of the reporter 3 stuff. They will switch all the senders to the floor reporter version in 1.35.0 (which would be 3.2.1 when this lands). I know you have other concerns besides spring, but it is a data point that may help you decide.

Copy link
Member

Choose a reason for hiding this comment

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

You haven't yet released the 3.x code yet.... one could argue you could remove this deprecated thing now and then not have to worry about v4.

Its a good point. We would have to do the same strategy I'm proposing - publish a new artifact id which includes zipkin 3 in the name some how. Deprecate, and eventually stop publishing the existing zipkin exporter artifact.

Added a topic to discuss in the tomorrow's java SIG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep another choice for discussion is to shade it and just expose it as configuration instead of a library hook. anyway thanks for the help!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jack-berg do you have an idea which way you are going to go? If this doesn't merge it isn't dire, just I will need to help split micrometer's classpath as the only place this causes a conflict in is tests. I mean it will be a conflict in real life, too, but only for those simultaneously using otel and also direct zipkin stuff.

Copy link
Member

Choose a reason for hiding this comment

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

I'm inclined to merge this PR and continue to publish the artifact under the coordinates. Then, we can consider publishing new artifact coordinates when zipkin publishes a v4 with the breaking change. Solving the problem now seems like putting the cart ahead of the horse.

@open-telemetry/java-approvers if there are no dissenting additional thoughts on this I'll plan on merging on 1/24/24.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jack-berg sounds good to me, and helps others from having more supporting work than maybe they can budget. v4 was just in the javadoc to tell people to stop using the old stuff as I noticed the senders were used outside of the reporters in that project. I made an issue about v4 and to not do that until it is important, likely with a new groupId/package openzipkin/zipkin-reporter-java#247

Copy link
Member

Choose a reason for hiding this comment

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

I'm inclined to merge this PR and continue to publish the artifact under the coordinates. Then, we can consider publishing new artifact coordinates when zipkin publishes a v4 with the breaking change. Solving the problem now seems like putting the cart ahead of the horse.

👍

@jack-berg jack-berg merged commit 0e4986a into open-telemetry:main Jan 24, 2024
18 checks passed
@codefromthecrypt codefromthecrypt deleted the new-sender branch January 24, 2024 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants