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

Support protobuf 3.22 or upper #2163

Merged
merged 54 commits into from
Aug 11, 2023

Conversation

owent
Copy link
Member

@owent owent commented May 27, 2023

Fixes #2095

Changes

Protobuf depends on abseil-cpp 20230125 from v22/v4.22.

It removed stringpiece.h, google::protobuf::Base64Escape and use absl::Base64Escape for alternative.

We have encountered multiple problems related to the usage of CMake packages in the past (#705 #1359). As a solution, I have added a patch script cmake/patch-imported-config.cmake that allows users to use prebuilt packages with different CONFIG settings when building otel-cpp.

BTW: gRPC v1.55.0 also depend the new protobuf and abseil-cpp, so I also test the new gRPC version in the new CI job.

  • Support protobuf 3.22 or upper
  • Add a CI job (cmake_modern_protobuf_grpc_test) to test building with new protobuf and gRPC.
  • Patch imported target in cmake CONFIG packages to enable users to use prebuilt packages with different CONFIG settings.
  • Remove --experimental_allow_proto3_optional when running protoc when we use protobuf 3.16 or upper. (It's not experimental any more)

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@owent owent requested a review from a team May 27, 2023 08:34
@codecov
Copy link

codecov bot commented May 27, 2023

Codecov Report

Merging #2163 (fdff03d) into main (b7fd057) will decrease coverage by 0.10%.
The diff coverage is 82.40%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2163      +/-   ##
==========================================
- Coverage   87.62%   87.52%   -0.10%     
==========================================
  Files         198      199       +1     
  Lines        5856     5981     +125     
==========================================
+ Hits         5131     5234     +103     
- Misses        725      747      +22     
Files Changed Coverage Δ
sdk/src/common/base64.cc 82.40% <82.40%> (ø)

@owent owent force-pushed the support_protobuf_3.22_or_upper branch from 904b15c to f95fd57 Compare May 27, 2023 11:28
@owent owent force-pushed the support_protobuf_3.22_or_upper branch from ce406cb to 0dc7575 Compare May 27, 2023 13:35
@owent owent force-pushed the support_protobuf_3.22_or_upper branch from be9b3f9 to 6fa4e95 Compare May 27, 2023 15:23
@owent owent force-pushed the support_protobuf_3.22_or_upper branch from a45451a to 8956193 Compare May 29, 2023 10:46
@owent owent changed the title [WIP] Support protobuf 3.22 or upper Support protobuf 3.22 or upper May 31, 2023
@esigo esigo self-assigned this May 31, 2023
@lalitb lalitb added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Aug 3, 2023
@ThomsonTan
Copy link
Contributor

There seems some syntax error for modern protobuf without abseil. Is this expected, or modern protobuf does require abseil?

https://github.com/open-telemetry/opentelemetry-cpp/actions/runs/5753570940/job/15599220018?pr=2163

@@ -63,6 +51,146 @@ namespace otlp
namespace
{

using Base64EscapeChars = const unsigned char[64];
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

There are a lot of codes here, do you think it's better to move it into sdk?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah good point, not required. We can move it later if need to share with other components. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

base64 algorithm is moved into sdk now.

Copy link
Member

Choose a reason for hiding this comment

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

Really sorry if my comment above was not clear. I did realize that this will bloat the sdk, and we should move it only when we need to share it with other components.

@owent owent removed the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Aug 4, 2023
@owent owent changed the title Support protobuf 3.22 or upper [WIP]Support protobuf 3.22 or upper Aug 4, 2023
@owent
Copy link
Member Author

owent commented Aug 4, 2023

There seems some syntax error for modern protobuf without abseil. Is this expected, or modern protobuf does require abseil?

https://github.com/open-telemetry/opentelemetry-cpp/actions/runs/5753570940/job/15599220018?pr=2163

I just tested this CI job and find protobuf can not work when both external and internal abseil-cpp exists.Because they share the same header paths and namespace and will have conflicts with each other.
Maybe we can move headers os internal abseil-cpp into another path, do not inline namespace otel_v1 and replace all usage absl:: in otel-cpp by a macro (OTABSL_NAMESPACE for example) and then we can define OTABSL_NAMESPACE to absl when we have HAVE_ABSEIL, or define it to absl::otel_v1. By this way we can keep ABI compatibility and use the internal and external abseil at the same time without break the ABI.

@lalitb This PR is ready to be reviewed again and thanks,

@owent owent changed the title [WIP]Support protobuf 3.22 or upper Support protobuf 3.22 or upper Aug 5, 2023
@lalitb
Copy link
Member

lalitb commented Aug 5, 2023

Maybe we can move headers os internal abseil-cpp into another path, do not inline namespace otel_v1 and replace all usage absl:: in otel-cpp by a macro (OTABSL_NAMESPACE for example) and then we can define OTABSL_NAMESPACE to absl when we have HAVE_ABSEIL, or define it to absl::otel_v1. By this way we can keep ABI compatibility and use the internal and external abseil at the same time without break the ABI.

Thanks for your analysis @owent. This would be helpful.

@owent
Copy link
Member Author

owent commented Aug 9, 2023

@lalitb @marcalff Could you please review this PR again? I have moved base64 codes into sdk and there are a lot of changes, so I removed od-to-merge label. I think we can add it back after the new changes are reviewed.

@lalitb
Copy link
Member

lalitb commented Aug 9, 2023

@lalitb @marcalff Could you please review this PR again? I have moved base64 codes into sdk and there are a lot of changes, so I removed od-to-merge label. I think we can add it back after the new changes are reviewed.

@owent - Seems you missed my comment here - #2163 (comment) . Actually, I didn't ask for moving base64 from otlp to sdk in my subsequent comments. But if you want, we can keep it in sdk for now, and have a separate PR to move back to otlp. Rest of the PR looks good.

@owent
Copy link
Member Author

owent commented Aug 11, 2023

@lalitb @marcalff Could you please review this PR again? I have moved base64 codes into sdk and there are a lot of changes, so I removed od-to-merge label. I think we can add it back after the new changes are reviewed.

@owent - Seems you missed my comment here - #2163 (comment) . Actually, I didn't ask for moving base64 from otlp to sdk in my subsequent comments. But if you want, we can keep it in sdk for now, and have a separate PR to move back to otlp. Rest of the PR looks good.

Thanks. I can move it back to otlp exporter later in another PR.

@owent owent added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Aug 11, 2023
@lalitb lalitb merged commit 0d1326a into open-telemetry:main Aug 11, 2023
@owent owent deleted the support_protobuf_3.22_or_upper branch September 8, 2023 09:12
@jhl07 jhl07 mentioned this pull request Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CVE Common Vulnerabilities and Exposures ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUILD] Compile WITH_OTLP failed
5 participants