Skip to content

Conversation

@kwasniew
Copy link
Contributor

@kwasniew kwasniew commented Sep 12, 2025

About the changes

Adds a new Histogram metric type to the impact-metrics system, following the same patterns as existing Counter and Gauge metrics.

Key Features

  • observe(value, labels?) method - Records observations with optional labels
  • Prometheus-compatible bucket system - Configurable buckets with automatic Infinity bucket (in prometheus users don't have to specify Infinity bucket themselves and we do the same)
  • Memory-efficient storage - Uses bucket counts instead of storing raw values to handle millions of observations (check Discussion points for the impact of this decision)
  • Full restoration support - Can restore exact histogram state after collection failures (more complex than gauge/counter due to the nature of histograms)
  • Type-safe discriminated unions - NumericMetricSample | BucketMetricSample prevents invalid states (histograms are very different from numeric counters/gauges)
  • Representing infinity as +Inf string similar to prometheus. JS Infinity does not serialize automatically with JSON.stirngify

Important files

Start review by reading tests to understand how histograms work in prometheus.

Discussion points

The backend will need to support batch histogram restoration from collected bucket data, which prom-client doesn't natively support, so we'll likely need to build custom logic to reconstruct prom-client histograms from our bucket counts. I still think it's better than recording millions of samples and then calling observe million times on the server side.

@coveralls
Copy link

coveralls commented Sep 12, 2025

Pull Request Test Coverage Report for Build 17915996843

Details

  • 59 of 65 (90.77%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 89.931%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/impact-metrics/metric-client.ts 10 12 83.33%
src/impact-metrics/metric-types.ts 49 53 92.45%
Totals Coverage Status
Change from base Build 17613865892: -0.05%
Covered Lines: 1267
Relevant Lines: 1348

💛 - Coveralls

@kwasniew kwasniew requested review from Tymek and sighphyre September 12, 2025 13:42
@gastonfournier gastonfournier moved this from New to In Progress in Issues and PRs Sep 23, 2025
@kwasniew kwasniew merged commit 55b711a into main Sep 24, 2025
5 checks passed
@kwasniew kwasniew deleted the histogram-metric branch September 24, 2025 11:57
@github-project-automation github-project-automation bot moved this from In Progress to Done in Issues and PRs Sep 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants