Skip to content

ci: Resolving nginx & prometheus build issue - #667

Open
thompson-tomo wants to merge 38 commits into
open-telemetry:mainfrom
thompson-tomo:ci_fix
Open

thompson-tomo wants to merge 38 commits into
open-telemetry:mainfrom
thompson-tomo:ci_fix

Conversation

@thompson-tomo

@thompson-tomo thompson-tomo commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

NGINX instrumentation build failures have been addressed by explicitly using buildx & switching tests to use php docker image which exists. Have also updated the tests to use nil for remote-parent-id rather than empty string.

Prometheus bazel build resolved by specifying an older bazel version to use.

Prometheus cmake build resolved by changing zlib adding process as well as switching to a native cmake build.

This is a replacement for #633 which i accidentally broke which triggered the closing of it.

@thompson-tomo thompson-tomo changed the title Ci fix ci: Resolving nginx & prometheus build issue + web server consistency- #633 Aug 22, 2026
@thompson-tomo
thompson-tomo marked this pull request as ready for review August 22, 2026 02:57
@thompson-tomo
thompson-tomo requested a review from a team as a code owner August 22, 2026 02:57
@thompson-tomo thompson-tomo changed the title ci: Resolving nginx & prometheus build issue + web server consistency- #633 ci: Resolving nginx & prometheus build issue + web server consistency- Aug 22, 2026
@thompson-tomo thompson-tomo changed the title ci: Resolving nginx & prometheus build issue + web server consistency- ci: Resolving nginx & prometheus build issue + web server consistency Aug 22, 2026
@thompson-tomo thompson-tomo changed the title ci: Resolving nginx & prometheus build issue + web server consistency ci: Resolving nginx & prometheus build issue Aug 23, 2026
Comment thread exporters/prometheus/CMakeLists.txt Outdated
include(FetchContent)

link_directories(${CMAKE_ARCHIVE_OUTPUT_DIRECTORY})
set(ENABLE_TESTING OFF CACHE BOOL "")

@lalitb lalitb Aug 23, 2026

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.

Can you check this once - it seems to be disabling the test for both dependencies and the exporter. Also seen this in CI:

  Updating test list for fixtures
  Added 0 tests to meet fixture requirements
  Checking test dependency graph...
  Checking test dependency graph end
  No tests were found!!! ??

Comment thread exporters/prometheus/CMakeLists.txt
include(CMakePackageConfigHelpers)
file(MAKE_DIRECTORY "${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/cmake/${PROJECT_NAME}")

configure_package_config_file(

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.

With this removal, the downstream CMake projects can no longer discover and link the installed exporter through its existing package target.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Has this been resolved with the newer commits?

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.

When built as a main project these lines are required to install the package. Please revert these removals for that case.

To keep the import target name the same please update the project namespace to be opentelemetry-cpp-contrib::

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These were causing build issues when moving away from using a local version of otel cpp & also other packages didn't have this but did have the add otel_component.

My impression was that is the newer pattern?

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.

Can you share more about the build issues? I'm hopeful we can get the independent install working.

The otel_add_component function is part of the main opentelemtry-cpp project's cmake utilities (added in v1.21) and was used in the user_events and geneva CMake files to allow injecting those contrib components into the main opentelemetry-cpp package when creating the vcpkg port.

I don't think we should propagate that pattern to inject external components into the core package any further in this repo. An alternative approach is to create a top level project for opentelemetry-cpp-contrib with its own utility functions to manage the install of the components. I started prototyping this in #550.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

From memory it was related to when switching to fetching of the libraries as opposed to installing them it caused the generated *-config.cmake to fail or something like that.

I don't think we should propagate that pattern to inject external components into the core package any further in this repo. An alternative approach is to create a top level project for opentelemetry-cpp-contrib with its own utility functions to manage the install of the components.

That sounds like a good solution as it would also enable testing of the non main project path in ci.

@lalitb

lalitb commented Aug 23, 2026

Copy link
Copy Markdown
Member

Thanks for continuing to improve the CI across contrib - these fixes are really appreciated. For this PR, the NGINX changes look good. Please revisit the Prometheus CMake workflow and dependency changes so we preserve the existing test, dependency, and packaging contracts.

@thompson-tomo

Copy link
Copy Markdown
Contributor Author

I am quite confused about the prometheus package purpose on the surface it should be an exporter just like user_events, however when you look at the cmake its name indicates otherwise.

If the name is correct then shouldn't there be a file in the root for the contrib repo & enable the prometheus to mirror other exporters. In the case of the later then the name etc should be updated.

Comment thread exporters/prometheus/CMakeLists.txt Outdated
@thompson-tomo
thompson-tomo requested a review from lalitb August 24, 2026 04:44
Comment thread exporters/prometheus/CMakeLists.txt Outdated
@marcalff

Copy link
Copy Markdown
Member

CI is failing since debian:11 is now EOL. Update to debian 13 ?

@thompson-tomo

Copy link
Copy Markdown
Contributor Author

@marcalff have bumped it to debian 12

Comment thread exporters/prometheus/CMakeLists.txt Outdated
Comment thread exporters/prometheus/CMakeLists.txt Outdated
Comment thread exporters/prometheus/CMakeLists.txt Outdated
Comment thread exporters/prometheus/apt-packages.txt Outdated
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.

5 participants