Skip to content

[BUILD] Allow shared opentelemetry_proto on non-Windows platform.#2097

Merged
marcalff merged 13 commits into
open-telemetry:mainfrom
owent:fix_2096
May 17, 2023
Merged

[BUILD] Allow shared opentelemetry_proto on non-Windows platform.#2097
marcalff merged 13 commits into
open-telemetry:mainfrom
owent:fix_2096

Conversation

@owent

@owent owent commented Apr 15, 2023

Copy link
Copy Markdown
Member

Fixes #2096
Fixes #2139

Changes

Please provide a brief description of the changes here.

  • Allow build shared opentelemetry_proto on non-Windows platform.
  • Add opentelemetry_proto_grpc which contains generated grpc files (Which depends gRPC) to make OTLP Http exporter do not depends gRPC when otel-cpp is built as dynamic libraries.

Sorry for push a wrong repo by mistake before, I have moved the fix_2096 branch into my fork.

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 April 15, 2023 10:52
@codecov

codecov Bot commented Apr 15, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2097 (bfbe345) into main (c9f14e9) will increase coverage by 0.03%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2097      +/-   ##
==========================================
+ Coverage   87.15%   87.17%   +0.03%     
==========================================
  Files         166      166              
  Lines        4777     4777              
==========================================
+ Hits         4163     4164       +1     
+ Misses        614      613       -1     

see 1 file with indirect coverage changes

owent added 6 commits April 20, 2023 11:28
Signed-off-by: WenTao Ou <admin@owent.net>
Signed-off-by: WenTao Ou <admin@owent.net>
Signed-off-by: WenTao Ou <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
if(WITH_OTLP_GRPC)
if(WITH_ABSEIL)
find_package(absl CONFIG)
if(TARGET absl::synchronization)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the target absl::synchronization optional, or error out if it doesn't exist?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure if absl::synchronization can be turn off.I'm not familar with abseil-cpp.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just curious the motive of this change - was there any build error with WITH_ABSEIL?

Comment thread cmake/opentelemetry-proto.cmake Outdated
list(APPEND OTELCPP_PROTO_TARGET_OPTIONS STATIC)
endif()

list(APPEND OTELCPP_PROTO_TARGET_OPTIONS)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure if I am missing something, but how are we using OTELCPP_PROTO_TARGET_OPTIONS

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry, it's a mistake after merging.

@lalitb

lalitb commented May 9, 2023

Copy link
Copy Markdown
Member

@owent As I understand, this PR adds two new shared libs - opentelemetry_proto and opentelemetry_proto_grpc. And these shared libraries would be used by otlp_recordable, otlp_http_client and otlp_grpc_client targets. How much reduction in total otel lib size we are seeing with this change ? As this could be a breaking change if users are hardcoding the required otel libraries in their build, we should have a sufficient total size reduction to reason for a breaking change.

…ing.

Signed-off-by: owent <admin@owent.net>
@owent

owent commented May 9, 2023

Copy link
Copy Markdown
Member Author

@owent As I understand, this PR adds two new shared libs - opentelemetry_proto and opentelemetry_proto_grpc. And these shared libraries would be used by otlp_recordable, otlp_http_client and otlp_grpc_client targets. How much reduction in total otel lib size we are seeing with this change ? As this could be a breaking change if users are hardcoding the required otel libraries in their build, we should have a sufficient total size reduction to reason for a breaking change.

Spliting opentelemetry_proto into opentelemetry_proto and opentelemetry_proto_grpc is used to avoid to depend on gRPC when users only use OTLP HTTP exporters. The generated gRPC sources in opentelemetry_proto_grpc depend gRPC.
If users use both OTLP HTTP and gRPC exporters, I think the total size will not decreased(at lease in my tests it do not decrease), but we can allow users use only one of them to reduce the final package size.

Comment thread CHANGELOG.md Outdated
[#2123](https://github.com/open-telemetry/opentelemetry-cpp/pull/2123)
* [BUILD] Build break with old curl, macro CURL_VERSION_BITS unknown
[#2102](https://github.com/open-telemetry/opentelemetry-cpp/pull/2102)
* [BUILD] Allow build shared opentelemetry_proto on non-Windows platform.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you think we should mentioned this PR potential breaking change here in changelog, as we have are introducing a new library?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you think we should mentioned this PR potential breaking change here in changelog, as we have are introducing a new library?
Sure, the changelog is added now.

Just curious the motive of this change - was there any build error with WITH_ABSEIL?

Yes, the related CI job is here https://github.com/open-telemetry/opentelemetry-cpp/actions/runs/4707416429/jobs/8349260349 .

if(WITH_OTLP_GRPC)
if(WITH_ABSEIL)
find_package(absl CONFIG)
if(TARGET absl::synchronization)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just curious the motive of this change - was there any build error with WITH_ABSEIL?

Signed-off-by: owent <admin@owent.net>

@lalitb lalitb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks

@marcalff marcalff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for the fix for 2139 as well.

@marcalff marcalff merged commit 23dfc20 into open-telemetry:main May 17, 2023
@marcalff marcalff changed the title Allow build shared opentelemetry_proto on non-Windows platform. [BUILD] Allow shared opentelemetry_proto on non-Windows platform. May 23, 2023
@owent owent deleted the fix_2096 branch May 27, 2023 06:11
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.

[BUILD] OTLP GRPC exporter fails to build with non default GRPC install Allow to build proto as dynamic on non-Windows platform.

4 participants