Use system libraries for abseil, protobuf, and curl #5305
Conversation
Remove abseil, protobuf, and curl from local build dependencies and use the system's libraries instead. This significantly reduces build size and build time while using system-maintained, security-patched libraries. This commit includes all changes from otel_2 branch: - System library integration (abseil, protobuf, curl) - OpenTelemetry implementation fixes - Documentation reorganization - Build improvements and cleanup Changes: - Set ABSL_IDIR, ABSL_LDIR to /usr/include and /usr/lib - Set PROTOBUF_IDIR, PROTOBUF_LDIR to /usr/include and /usr/lib - Set CURL_IDIR, CURL_LDIR to /usr/include and /usr/lib - Remove 'abseil', 'protobuf', 'curl' from targets list - Remove abseil, protobuf, curl build targets - Update OpenTelemetry to use source include directories - Update linking to use dynamic linking for system libs - Add OpenTelemetry documentation in otel/ folder - Add OpenTelemetry tests - Fix JSON include issues in tap tests System packages required: - libabsl-dev (abseil) - libprotobuf-dev, protobuf-compiler (protobuf) - libcurl4-openssl-dev (curl)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
Summary of ChangesHello @renecannao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the project's dependency management by transitioning from locally building Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes a significant improvement by switching from local builds of abseil, protobuf, and curl to using system-provided libraries. This simplifies the build process, reduces compilation time, and leverages system-maintained packages. Additionally, the PR includes a substantial and valuable refactoring of the OpenTelemetry integration, greatly improving memory safety, exception safety, and robustness through the use of RAII patterns and better error handling. The introduction of unit tests for the OpenTelemetry components is also a great addition.
My review includes two main points:
- A suggestion to improve portability in the build files by using
pkg-configinstead of hardcoded library paths. - A critical fix for a race condition in the management of the global OpenTelemetry tracer instance.
Overall, this is a high-quality contribution that modernizes the dependency management and hardens the tracing implementation.
| void OTelTracer::SetGlobalTracer(OTelTracer* tracer) { | ||
| pthread_rwlock_t rwlock_local; | ||
| pthread_rwlock_init(&rwlock_local, nullptr); | ||
| pthread_rwlock_wrlock(&rwlock_local); | ||
| g_global_tracer = tracer; | ||
| pthread_rwlock_unlock(&rwlock_local); | ||
| pthread_rwlock_destroy(&rwlock_local); | ||
| } |
There was a problem hiding this comment.
The locking mechanism for the global tracer g_global_tracer is not thread-safe. This function uses a local pthread_rwlock_t, which is created and destroyed within this function call. It does not protect against concurrent calls to SetGlobalTracer, GetGlobalTracer, or the OTelTracer constructor/destructor from different threads.
To fix this, you should use a single static pthread_rwlock_t to protect all accesses to the static g_global_tracer pointer.
Here's a suggested implementation approach:
-
Define a static lock for the global tracer near
g_global_tracer:namespace { OTelTracer* g_global_tracer = nullptr; pthread_rwlock_t g_global_tracer_lock = PTHREAD_RWLOCK_INITIALIZER; }
-
Update
SetGlobalTracerto use this static lock:void OTelTracer::SetGlobalTracer(OTelTracer* tracer) { pthread_rwlock_wrlock(&g_global_tracer_lock); g_global_tracer = tracer; pthread_rwlock_unlock(&g_global_tracer_lock); }
-
Similarly,
GetGlobalTracershould use a read lock, and the constructor/destructor should use a write lock ong_global_tracer_lockwhen modifyingg_global_tracer.
| ABSL_IDIR := /usr/include | ||
| ABSL_LDIR := /usr/lib | ||
|
|
||
| PROTOBUF_PATH := $(DEPS_PATH)/protobuf/protobuf | ||
| PROTOBUF_ROOT := $(PROTOBUF_PATH)/local | ||
| PROTOBUF_IDIR := $(PROTOBUF_ROOT)/include | ||
| PROTOBUF_LDIR := $(PROTOBUF_ROOT)/lib | ||
| PROTOBUF_IDIR := /usr/include | ||
| PROTOBUF_LDIR := /usr/lib |
There was a problem hiding this comment.
Hardcoding library paths to /usr/include and /usr/lib reduces portability. On many systems, these libraries might be installed in different locations like /usr/local or other custom paths. It's better to use a tool like pkg-config to discover these paths dynamically at build time. This will make the build system more robust and portable across different environments.
For example, you could define the paths like this:
ABSL_IDIR := $(shell pkg-config --cflags-only-I absl_base)
ABSL_LDIR := $(shell pkg-config --libs-only-L absl_base)
PROTOBUF_IDIR := $(shell pkg-config --cflags-only-I protobuf)
PROTOBUF_LDIR := $(shell pkg-config --libs-only-L protobuf)Note that you might need to adjust the pkg-config package names (absl_base, protobuf) depending on the system. This also applies to CURL_IDIR and CURL_LDIR defined earlier in this file.
Clean up the deps directory by removing: - deps/protobuf/ directory (using system protobuf now) - Extracted opentelemetry-cpp and opentelemetry-proto folders - These will be extracted during build when needed Only the tarballs and patches remain in deps/opentelemetry/.
|
1 participant
Summary
This PR removes abseil, protobuf, and curl from local build dependencies and configures the project to use the system's libraries instead. This significantly reduces build size and build time while using system-maintained, security-patched libraries.
Changes
System Library Integration
libabsl-devfrom the systemlibprotobuf-devandprotobuf-compilerfrom the systemlibcurl4-openssl-devfrom the systemBuild Configuration (deps/Makefile)
Path Configuration (include/makefiles_paths.mk)
ABSL_IDIRandABSL_LDIRto/usr/includeand/usr/libPROTOBUF_IDIRandPROTOBUF_LDIRto/usr/includeand/usr/libCURL_IDIRandCURL_LDIRto/usr/includeand/usr/libOTEL_IDIRto point to actual source include directories (api, sdk, ext, exporters)*_ROOTand*_PATHvariables for system packagesLinking Updates (src/Makefile)
-lcurland-lprotobuffrom static to dynamic linking-labsl,-lutf8_range,-lutf8_validityfrom static linking-lidn2for curl)OpenTelemetry Implementation
otel/foldertests/otel/)Benefits
System Requirements
The following system packages are now required:
libabsl-dev(abseil)libprotobuf-dev(protobuf development files)protobuf-compiler(protoc compiler for OpenTelemetry)libcurl4-openssl-dev(curl development files)These can be installed on Ubuntu/Debian with:
Testing
make build_depscompletes successfullymake(regular build) completes successfullymake debug(debug build) completes successfullylibprotobuf.so.32 => /lib/x86_64-linux-gnu/libprotobuf.so.32libcurl.so.4 => /lib/x86_64-linux-gnu/libcurl.so.4