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

Provide a CMake target for prometheus_exporter_utils #2566

Closed

Conversation

dufferzafar
Copy link

Fixes #2565

@dufferzafar dufferzafar requested a review from a team February 28, 2024 06:18
Copy link

linux-foundation-easycla bot commented Feb 28, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: dufferzafar / name: Shadab Zafar (eef4fcf)
  • ✅ login: esigo / name: Ehsan Saei (fca523a)

target_link_libraries(
opentelemetry_exporter_prometheus_utils
PUBLIC opentelemetry_metrics prometheus-cpp::core)

Copy link
Member

Choose a reason for hiding this comment

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

Targets are also added in cmake config -

set(_OPENTELEMETRY_CPP_LIBRARIES_TEST_TARGETS
But I am not sure of the implications in that case - as the symbols from exporter_utils.cc would be part of two target belonging to same package OPENTELEMETRY_CPP_LIBRARIES. Will let @owent comment on that :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that should be an issue, resolving it. Sorry for confusion :)

Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove src/exporter_utils.cc from and link opentelemetry_exporter_prometheus_utils into opentelemetry_exporter_prometheus if we want to split this file into a standalone target.Or there may be conflict when some components link both opentelemetry_exporter_prometheus_utils and opentelemetry_exporter_prometheus.

BTW: The documents and the component name for OPENTELEMETRY_CPP_LIBRARIES should also be added into cmake/opentelemetry-cpp-config.cmake.in.

@dufferzafar
Copy link
Author

@lalitb Does the patch look okay apart from your original comment?

@esigo
Copy link
Member

esigo commented Mar 11, 2024

The PR should not break existing builds for the users and this has to be investigated before merge.

@esigo esigo added the pr:do-not-merge This PR is not ready to be merged. label Mar 11, 2024
@marcalff
Copy link
Member

Thanks for the PR.

Currently, opentelemetry-cpp maintains the following builds:

  • CMake, with various combinations for optional features (WITH_OTLP_HTTP, etc), and with various combinations for technical dependencies (C++ STL, abseil, etc)
  • Bazel, also with complexity related to all the dependencies used.

On top of the build systems supported by opentelemetry-cpp,
some third parties are also making their own builds, for example:

  • Vcpkg
  • Conan
  • bazel central

Maintaining all this ecosystem, and making sure various combinations of existing features are working properly is already a challenge for opentelemetry-cpp, and a major drain on development resources.

What this PR asks for makes sense, but the problem is that this new build combination would not be tested and covered in the existing CI, since the new library only makes sense when used by external code not available in opentelemetry-cpp.

Adding even more complexity to an already difficult to maintain area is not desirable at this point.

Given how this combination is not covered in our CI, there is a higher risk of this build to be broken by later changes or refactoring done in the opentelemetry-cpp make files, and the break can go unnoticed.

As a result, if we (opentelemetry-cpp) accept this PR, we would just be setting expectations (to support it and maintain it) that realistically will not be met.

Closing this PR.

Overall, the need to cut the dependency from the prometheus exporter to a given web server is valid.

The best way to achieve this is to extend the prometheus exporter to support alternate web servers instead, but this is a much bigger feature and not just a makefile change to split a library.

@marcalff marcalff closed this Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:do-not-merge This PR is not ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a CMake prometheus_exporter_utils library target
5 participants