Skip to content

fix(metrics): register prometheus histograms - #506

Open
ffelipelimao wants to merge 1 commit into
mainfrom
fix/register-prometheus-histograms
Open

ffelipelimao wants to merge 1 commit into
mainfrom
fix/register-prometheus-histograms

Conversation

@ffelipelimao

Copy link
Copy Markdown
Collaborator

Summary

PrometheusReporter builds every HistogramVec (custom metrics.custom.histograms and the built-in channel_capacity) and stores it in histogramReportersMap, but the final registerMetrics loop only hands counters, gauges and summaries to prometheus.MustRegister. ReportHistogram therefore returns nil while observing into collectors no registry knows about, and the series never appear on /metrics. Reported in #504.

This registers the histogram map along with the other collectors and adds the first test for the Prometheus reporter.

Key Changes

  • pkg/metrics/prometheus_reporter.go: append histogramReportersMap to the collectors passed to prometheus.MustRegister.
  • Remove the built-in response_time_ns histogram. Nothing calls ReportHistogram(ResponseTime) (report.go and agent.go use the summary / ChannelCapacity), and once registered it would collide with the response_time_ns summary: same fully-qualified name and const labels, which Registry.Register rejects, so MustRegister would panic on startup for everyone with metrics.prometheus.enabled. A histogram variant of response time is still possible later under a distinct name.
  • pkg/metrics/prometheus_reporter_test.go: builds the reporter with a custom histogram spec, reports one sample to it and to channel_capacity, and asserts both families are gathered as HISTOGRAM with sample count 1. Fails on main with pitaya_custom_custom_histogram is not registered.
  • go.mod: prometheus/client_model moves from indirect to direct (imported by the test for dto.MetricType).

How to Test

go test ./pkg/metrics/ -run TestPrometheusReporterRegistersHistograms

Manual check: enable metrics.prometheus, declare a histogram under metrics.custom.histograms, call ReportHistogram, and curl /metrics. Before this change only summaries/counters/gauges show up; after it the _bucket, _sum and _count series for the custom histogram and for pitaya_channel_channel_capacity are present.

Closes #504

🤖 Generated with Claude Code

Histogram vectors were created and stored in histogramReportersMap but
never appended to the collectors passed to prometheus.MustRegister, so
ReportHistogram observed into unregistered collectors and neither the
custom histograms nor the built-in channel_capacity ever showed up on
/metrics.

Register the histogram map alongside counters, gauges and summaries.
The unused response_time_ns histogram is removed: nothing calls
ReportHistogram(ResponseTime), and once registered it would collide
with the summary of the same fully-qualified name and panic on startup.

Closes #504

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 34607495191

Coverage increased (+2.8%) to 61.268%

Details

  • Coverage increased (+2.8%) from the base build.
  • Patch coverage: 3 of 3 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 8724
Covered Lines: 5345
Line Coverage: 61.27%
Coverage Strength: 0.67 hits per line

💛 - Coveralls

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.

bug: prometheus histogram metrics is not registered

2 participants