OTLP Exporter: Forklift and internalize Google.Protobuf

[alanwest]

Solution for #4395 by forklifting Google.Protobuf into the OTLP exporter project.

This PR copies the code from Google.Protobuf @ v3.22. The code is modified to suppress Stylecop and mark all public items internal.

Options to consider:

1. Accept this PR as is.
   • Pros: If we ever need to update to a later version of Google.Protobuf, the maintenance burden may be lower. Basically replace the code with newer version, suppress stylecop, and make everything internal.
   • Cons: It's a lot of code. Binary size increases by about 425KB. This might be offset by the fact that consumers may no longer need to also pull the Google.Protobuf dependency.
2. Attempt to trim out unused code.
   • Pros: Makes the binary smaller. I'm not sure by how much without doing the work. I suspect not by much. I've reviewed things a bit and it seems the code generated from the OTLP protos depends on quite a bit from Google.Protobuf.
   • Cons: Tedious. I have not found good tooling that helps automate this much. The maintenance burden would definitely be higher in the event we ever wanted to update to a later version.
3. Reject this PR and pursue the option using ILRepack.
   • Pros: Significantly less code to maintain. ILRepack is a tool that merges the MSIL from two binaries. It allows you to completely internalize a dependency. I have a branch that demonstrates its use. This option also makes updating to a later version trivial.
   • Cons: The binary size increases by about 425KB just as though we had copied the code and compiled it ourselves.
4. Reject internalizing Google.Protobuf altogether. This may limit or hinder the opentelemetry-dotnet-instrumentation project from upgrading to the 1.5.0 SDK.

[codecov]

Codecov Report

Merging #4396 (43655a0) into main (457a9e9) will decrease coverage by 21.26%.
The diff coverage is 24.18%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #4396       +/-   ##
===========================================
- Coverage   84.75%   63.49%   -21.26%     
===========================================
  Files         300      392       +92     
  Lines       12010    18131     +6121     
===========================================
+ Hits        10179    11513     +1334     
- Misses       1831     6618     +4787     

Impacted Files	Coverage Δ
...OpenTelemetryProtocol/Google.Protobuf/ByteArray.cs	0.00% <0.00%> (ø)
...lemetryProtocol/Google.Protobuf/ByteStringAsync.cs	0.00% <0.00%> (ø)
...metryProtocol/Google.Protobuf/Collections/Lists.cs	0.00% <0.00%> (ø)
...ryProtocol/Google.Protobuf/Collections/MapField.cs	0.00% <0.00%> (ø)
...mpatibility/DynamicallyAccessedMembersAttribute.cs	0.00% <0.00%> (ø)
...e.Protobuf/Compatibility/PropertyInfoExtensions.cs	0.00% <0.00%> (ø)
...Compatibility/RequiresUnreferencedCodeAttribute.cs	0.00% <0.00%> (ø)
...ol/Google.Protobuf/Compatibility/TypeExtensions.cs	0.00% <0.00%> (ø)
...atibility/UnconditionalSuppressMessageAttribute.cs	0.00% <0.00%> (ø)
...etryProtocol/Google.Protobuf/Compiler/Plugin.pb.cs	0.00% <0.00%> (ø)
... and 82 more

... and 3 files with indirect coverage changes

[reyang]

5. use https://github.com/protobuf-net/protobuf-net
6. use https://github.com/protobuf-net/protobuf-net and work with the owner to find a proper solution (maybe codegen?)

[reyang]

Two reasons to block the PR for now:

1. Need to figure out whether copying more than one hundred files is the right path forward (considering all the future obligations of security hotfixes and patches).
2. Need to understand the licensing / legal implications.

[alanwest]

5. use https://github.com/protobuf-net/protobuf-net
6. use https://github.com/protobuf-net/protobuf-net and work with the owner to find a proper solution (maybe codegen?)

I suspect these options would present the same concern for the opentelemetry-dotnet-instrumentation project. Though, @Kielek can weigh in.

1. Need to figure out whether copying more than one hundred files is the right path forward (considering all the future obligations of security hotfixes and patches).

For the reason of hotfixes/patches, if we seek to internalize anything - whether it be Google.Protobuf of protobuf-net - I'm beginning to lean more towards looking at ILRepack. Though, I'm not interested in spending much more time on this if I know up front that we're not keen on internalizing anything. Maybe @Kielek can share how this would affect the auto-instrumentation project.

2. Need to understand the licensing / legal implications.

This is likely a concern whether we copy code or use a tool like ILRepack. BSD-3 is a permissive license as I understand it, but I'm not a lawyer. Does the CNCF have a process for this?

[alanwest]

2. Need to understand the licensing / legal implications.

Just a point of reference from opentelemetry-java. They have copied some portions of Java's protobuf library. Albeit much less than what is in this PR, because they've rolled most of their own serialization. Ideally, this is a direction we might consider as well in the future.

[reyang]

7. handcraft a tailored version of serializer, similar to what https://github.com/open-telemetry/opentelemetry-dotnet/blob/9a7bd9f55ad3f643484fe7d62d92c2553c623da1/src/OpenTelemetry.Exporter.Prometheus.HttpListener/Internal/PrometheusSerializer.cs is doing. (and also https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/f9528610ac99eb67f730cbe3ba810026b446b840/src/OpenTelemetry.Exporter.Geneva/MsgPackExporter/MessagePackSerializer.cs)

[alanwest]

7. handcraft a tailored version of serializer

This is actually option 4 😆 - that is, don't internalize anything. My thought behind #4395 was to have the discussion about the viability of internalizing Google.Protobuf out in the open with the intent that it would be an intermediate step towards "option 7". I agree option 7 is where we want to go in the long run.

There's no reason for us to make a hasty decision to internalize or not. Though, I am interested in providing some help to the opentelemetry-dotnet-instrumentation project in the near term. Here's an alternate proposal:

1. Revert [Otlp] Bump Google.Protobuf to 3.22.0 and remove reflection emit code #4201 for now. This will reintroduce Reflection.Emit and take us a step back in AOT support.
2. Do the leg work to figure out how to perform our own protobuf serialization.
3. If step 2 is successful, then problem solved - i.e., no more Google.Protobuf dependency + we're closer to AOT support. If it's not successful by the time .NET 8 is shipped, then maybe we reopen discussion to internalize Google.Protobuf.

@CodeBlanch what do you think about this? My presumption is that doing the work to perform our own serialization will likely not be complete in our timeline for 1.5, so best to revert #4201 for now and see if we can sort this out for 1.6.

[reyang]

7. handcraft a tailored version of serializer

This is actually option 4 😆 - that is, don't internalize anything. My thought behind #4395 was to have the discussion about the viability of internalizing Google.Protobuf out in the open with the intent that it would be an intermediate step towards "option 7". I agree option 7 is where we want to go in the long run.

Here is my suggestion (from most preferred to least preferred):

1. Don't reinvent wheel if there is already a good solution. Seek guidance from the maintainers of https://github.com/protobuf-net/protobuf-net - as the issue we're facing here is not something specific to us, most likely many folks have resolved this issue already.
2. If there is no good solution from 1), we can explore a handcrafted version of serializer, make sure perf is awesome (specifically, make sure we do great job on memory utilization). Protobuf-net Performance protobuf-net/protobuf-net#754
3. If we need to steal code or have package dependency "smoothed out" by ILRepack. The first thing we should be very clear about, is which one to bet on - https://stackoverflow.com/questions/64359801/what-nuget-package-should-i-use-between-protobuf-net-and-google-protobuf-for-a-n.

[reyang]

Some updates from my conversation with the .NET team:

• If we have to choose between Google.Protobuf and protobuf-net, we should use protobuf-net since it is the one that works well with AOT.
• Most likely we'll end up with a handcrafted version, however we might want to seek some guidance from Mark Gravell (I have yet to search his GitHub alias) who is the author of protobuf.net.
• If we need to look into gRPC client lib, we might want to consult @JamesNK who did the ASP.NET Core implementation of gRPC.

[JamesNK]

If we have to choose between Google.Protobuf and protobuf-net, we should use protobuf-net since it is the one that works well with AOT.

??? Google.Protobuf is the library that works with AOT. protobuf-net uses a lot of dynamic code generation

If we need to look into gRPC client lib, we might want to consult @JamesNK who did the ASP.NET Core implementation of gRPC.

gRPC code generation uses Google.Protobuf. I don't know whether it would be possible to use an internal copy.

[reyang]

If we have to choose between Google.Protobuf and protobuf-net, we should use protobuf-net since it is the one that works well with AOT.

??? Google.Protobuf is the library that works with AOT. protobuf-net uses a lot of dynamic code generation

If we need to look into gRPC client lib, we might want to consult @JamesNK who did the ASP.NET Core implementation of gRPC.

gRPC code generation uses Google.Protobuf. I don't know whether it would be possible to use an internal copy.

Appreciate your input @JamesNK 👍 I should clarify that I was relaying messages based on what I heard from several folks, I have no expertise in protobuf so you need to verify what I put above - FYI @alanwest

[mgravell]

If we have to choose between Google.Protobuf and protobuf-net, we should use protobuf-net since it is the one that works well with AOT.

To echo James' point: yes, currently protobuf-net uses ref-emit at runtime, because it was originally designed for code-first scenarios; AOT focus is in progress, via Roslyn analyzers for code-first, and the regular code-gen for contract-first, but: the AOT story is not complete today.

Most likely we'll end up with a handcrafted version, however we might want to seek some guidance from Mark Gravell (I have yet to search his GitHub alias) who is the author of protobuf.net.

That would be @mgravell

[alanwest]

Yes, thank you for both your input, @JamesNK and @mgravell!

AOT focus is in progress, via Roslyn analyzers for code-first, and the regular code-gen for contract-first, but: the AOT story is not complete today.

@mgravell in #4430, I'm using the contract-first approach and using protobuf-net.BuildTools to perform code-gen. Will AOT support be a concern here?

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.