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

Removing protobuf dependency #1008

Merged
merged 17 commits into from
Aug 24, 2023

Conversation

LikeTheSalad
Copy link
Contributor

@LikeTheSalad LikeTheSalad commented Aug 21, 2023

Description:

Bug fix - Since this module has a dependency on OTel Java proto it means that it also gets Protobuf's java lib as a transitive dependency. The protobuf-java library is not ideal for Android projects due to its size which is why there's another version of it, called protobuf-javalite, that aims to be more suitable for Android use cases due to its smaller size.

Given that the protobuf-javalite dependency is better for Android projects, some projects might have it included already which causes a problem if they later add the disk-buffering dependency as well, since it comes with the "regular protobuf dependency". This is an issue because both, the regular and lite versions of the protobuf libs, contain classes that have the exact same fully qualified names, which makes Gradle raise a compilation error due to the "duplicated classes" it finds across different dependencies.

This was discussed in the latest Java SIG meeting where a couple of interesting ideas were proposed to avoid this issue such as:

  • Shadowing the regular protobuf lib's classes to prevent them from clashing with the lite ones later. The problem with this option is the final lib size, as well as the added size across both, the lite version and the "regular" version of the protobuf libraries that would get added into an app that already has the lite version as a dependency.
  • Adding instructions on how to shadow these classes on the consumer project. This would make it a bit complicated to integrate the disk-buffering dependency into a project that already has the protobuf-javalite dependency. Ideally, I'd like this lib to be easy to use.
  • It was briefly mentioned that we could also make the OTel Java proto artifacts to use the lite version only. Though my concern with that approach would be that, in case a project has the "protobuf-java regular" version already as a dependency, then the same kind of issue would happen but the other way around.

The approach I'm proposing to go with in this PR is to not use any of the protobuf dependencies, neither the "regular" nor the "lite" one, in disk-buffering. Instead, we can use Wire, which is a third-party tool (already being used in other OTel Java projects) that takes care of generating the Java sources from the proto files without having any dependency on Google's protobuf libraries. As a bonus side of using this tool, we get smaller generated sources since Wire makes them a bit more android friendly by reducing the number of methods for each generated class.

Existing Issue(s):

#1009

Testing:

Unit, integration, manual tests.

Documentation:

No documentation changes are needed as part of this PR since the changes are only internal.

Outstanding items:

@LikeTheSalad LikeTheSalad marked this pull request as ready for review August 21, 2023 17:23
@LikeTheSalad LikeTheSalad requested a review from a team August 21, 2023 17:23
@zeitlinger
Copy link
Member

are all of those serializer classes hand-written?

@LikeTheSalad
Copy link
Contributor Author

LikeTheSalad commented Aug 22, 2023

are all of those serializer classes hand-written?

Yes. They used to be auto-generated using MapStruct, however, that caused the PR to be bigger since MapStruct couldn't reuse the existing builders/creators for all the implementations of the OTel API in the Java SDK.

The changes made to all of them here are minimal though, it's mostly accessing fields directly instead of through getters (because Wire doesn't create those to reduce the number of methods), and also renaming some builder setters to the style added by Wire.

@trask trask changed the title Disk buffering proto cleanup Removing protobuf dependency Aug 24, 2023
@trask
Copy link
Member

trask commented Aug 24, 2023

@LikeTheSalad no need to add change log in this repo, just update the PR title to what you would like displayed and we'll pull it in semi-automatically before making the next release

@trask trask enabled auto-merge (squash) August 24, 2023 15:04
@trask trask merged commit 8bfad76 into open-telemetry:main Aug 24, 2023
13 checks passed
@LikeTheSalad LikeTheSalad deleted the disk-buffering-proto-cleanup branch August 24, 2023 15:55
zeitlinger pushed a commit to zeitlinger/opentelemetry-java-contrib that referenced this pull request Aug 25, 2023
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
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.

4 participants