Skip to content

[PROMETHEUS] Upgrade otelcpp to 1.28, add prometheus file exporter - #646

Open
owent wants to merge 10 commits into
open-telemetry:mainfrom
owent:prometheus_exporters
Open

[PROMETHEUS] Upgrade otelcpp to 1.28, add prometheus file exporter#646
owent wants to merge 10 commits into
open-telemetry:mainfrom
owent:prometheus_exporters

Conversation

@owent

@owent owent commented Aug 5, 2026

Copy link
Copy Markdown
Member
  • Fixes some thread-safety problems.
  • Implement prometheus file exporter.
  • Upgrade otelcpp to 1.28.

Copilot AI lite review requested due to automatic review settings August 5, 2026 07:19
@owent
owent requested a review from a team as a code owner August 5, 2026 07:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Prometheus exporter module to OpenTelemetry C++ 1.28 and adds a new Prometheus file exporter implementation alongside the existing push exporter.

Changes:

  • Bump Prometheus module versions/references from 1.19.0 to 1.28.0 (CMake/Bazel/GitHub Actions/vcpkg).
  • Add a new Prometheus file exporter (implementation, options, factory) and wire it into CMake + Bazel builds.
  • Extend Prometheus push exporter options/translation path with populate_target_info and without_otel_scope flags.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
exporters/prometheus/vcpkg.json Adds vcpkg manifest for the Prometheus contrib module (now needs metadata aligned with push + file exporters).
exporters/prometheus/tools.cmake Introduces CMake helpers for import/export visibility; contains Windows attribute and SunPro detection issues.
exporters/prometheus/src/push_exporter.cc Plumbs new translation options through push exporter collection/translation.
exporters/prometheus/src/push_exporter_factory.cc Exports the factory Create symbol via the new API macro.
exporters/prometheus/src/file_exporter.cc Adds the new rotating-file exporter implementation (includes time/path formatting, background flush thread).
exporters/prometheus/src/file_exporter_factory.cc Adds factory entry point for constructing the new file exporter.
exporters/prometheus/MODULE.bazel Updates Bazel module + dependency versions to 1.28.0.
exporters/prometheus/include/opentelemetry/exporters/prometheus/push_exporter.h Minor formatting-only change.
exporters/prometheus/include/opentelemetry/exporters/prometheus/push_exporter_options.h Adds new options + API visibility macro definition.
exporters/prometheus/include/opentelemetry/exporters/prometheus/push_exporter_factory.h Exposes factory Create with API macro and includes options header.
exporters/prometheus/include/opentelemetry/exporters/prometheus/file_exporter.h Adds public header for the new file exporter (needs API export annotation).
exporters/prometheus/include/opentelemetry/exporters/prometheus/file_exporter_options.h Adds options + API visibility macro definition for file exporter.
exporters/prometheus/include/opentelemetry/exporters/prometheus/file_exporter_factory.h Adds public factory header for the file exporter.
exporters/prometheus/CMakeLists.txt Updates project version, adds new library target, and adds import/export macro wiring.
exporters/prometheus/BUILD Adds Bazel cc_library target for the file exporter.
.github/workflows/prometheus.yml Updates CI to test against opentelemetry-cpp v1.28.0.
Suppressed comments (3)

exporters/prometheus/tools.cmake:44

  • Same issue as the export path: __attribute__((__dllimport__)) is not the standard GCC/Clang attribute spelling. This likely results in no import decoration on Windows with MinGW/Clang.
  if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang|AppleClang|Intel|XL|XLClang")
    if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
      set(${OUTPUT_VARNAME}
          "__attribute__((__dllimport__))"
          PARENT_SCOPE)

exporters/prometheus/src/file_exporter.cc:693

  • ~PrometheusFileBackend() joins the background flush thread but never sets is_shutdown to stop it. The thread can run for up to 1 minute before exiting (idle timeout), so destroying the exporter can block for a long time if Shutdown() wasn’t called.
  ~PrometheusFileBackend()
  {
    if (file_)
    {
      file_->background_thread_waker_cv.notify_all();
      std::unique_ptr<std::thread> background_flush_thread;

exporters/prometheus/src/file_exporter.cc:1089

  • CheckUpdate() updates rotate_index and calls ResetLogFile() without holding file_lock, even though ResetLogFile() explicitly assumes it is called under lock. This is a data race if AddMetricData() can be invoked concurrently, and can also conflict with OpenLogFile()/RotateLog() which use the same state under file_lock.
    // Reset rotate index when directory changes
    if (new_dir != old_dir)
    {
      file_->rotate_index = 0;
    }

    ResetLogFile();

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread exporters/prometheus/tools.cmake
Comment thread exporters/prometheus/tools.cmake Outdated
Comment thread exporters/prometheus/src/file_exporter.cc Outdated
Comment on lines +26 to +27
class PrometheusFileExporter : public ::opentelemetry::sdk::metrics::PushMetricExporter
{

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.

It's not public header

Comment thread exporters/prometheus/src/file_exporter_factory.cc
Comment thread exporters/prometheus/vcpkg.json
@owent

owent commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

Sorry for it's be a long time to update this package. Could you please review it again when you have time? @ThomsonTan @esigo @lalitb

@proost proost left a comment

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.

Maintainers looks very busy, so i'd like to help out.

Can you add test cases for file exporter?

return 0;
}

file.seekg(std::ios::end);

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.

file.seekg(0, std::ios::end)

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.

Thanks, unit tests are added.

std::string new_dir = FileSystemUtil::DirName(new_file_path);
std::string old_dir = FileSystemUtil::DirName(old_file_path);

// Reset rotate index when directory changes

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.

Looks like we should hold lock in here to call ResetLogFile. Am i correct?

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.

It's not required to be very precise in file size calculation. So I changed all the variables in ResetLogFile tobe atomic.And no lock is reauired neither.

file_->flushed_metric_family_count.store(0);
}

~PrometheusFileBackend()

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.

Are there no need to set "is_shutdown" to true?

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.

Thanks, is_shutdown is set now.

@owent
owent force-pushed the prometheus_exporters branch from 436105b to ac28893 Compare August 17, 2026 06:21
std::lock_guard<std::mutex> lock_guard{file_->background_thread_lock};
if (!file_->background_flush_thread)
{
break;

@proost proost Aug 19, 2026

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.

non-blocking; no background flush thread and "is_shutdown" is true, can we return earlier right?

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, I'm not sure I follow. When there's been no IO for a while, the background thread exits on its own, so it shouldn't matter whether it's explicitly shut down or not.

@owent
owent force-pushed the prometheus_exporters branch from ba10b0e to 4156383 Compare August 20, 2026 06:18
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.

3 participants