Skip to content

Use correct aggregation function for quantities total#8769

Merged
pieterbeulque merged 3 commits intomainfrom
claude/investigate-meter-api-issue-kxEEr
Jan 7, 2026
Merged

Use correct aggregation function for quantities total#8769
pieterbeulque merged 3 commits intomainfrom
claude/investigate-meter-api-issue-kxEEr

Conversation

@pieterbeulque
Copy link
Contributor

Summary

Fixes a regression in the /v1/meters/{id}/quantities API endpoint where the total field was incorrectly calculated for meters with non-summable aggregations (MAX, MIN).

Bug

Users reported seeing incorrect values in the total field:

When I look in the UI, the events show total_enabled_servers: 1. However, when reading from the API with a 30 day lookback, I see almost every org showing 29/30/31.

For meters with max aggregation, the quantities.total value returns the sum of all values (not max across all values).

Root Cause

This is a regression introduced in commit 668ea64 (Dec 18, 2025) titled "meter: Speed up query by avoiding large cartesian join".

Before the regression

The original code correctly used the meter's aggregation function for the total:

func.coalesce(
    meter.aggregation.get_sql_column(Event).filter(
        interval.sql_date_trunc(Event.timestamp) >= interval.sql_date_trunc(start_timestamp),
        interval.sql_date_trunc(Event.timestamp) <= interval.sql_date_trunc(end_timestamp),
    ),
    0,
).label("total"),

After the regression

Commit 668ea64

The performance optimization refactored the query to use pre-computed daily aggregates, but hardcoded func.sum() for the total:

func.coalesce(
    func.sum(daily_metrics.c.quantity).over(order_by=timestamp_column),
    0,
).label("total"),

This works correctly for summable aggregations (count, sum) but is incorrect for non-summable ones (max, min, avg, unique).

Fix

The fix determines the appropriate SQL window function based on whether the meter's aggregation is summable:

if meter.aggregation.is_summable():
    total_agg_func = func.sum
else:
    total_agg_func = meter.aggregation.func.get_sql_function

This is still broken for average & unique aggregation meters.

claude added 2 commits January 5, 2026 20:45
Add test_interval_non_summable_aggregation to verify that the `total`
field in MeterQuantities is computed correctly for non-summable
aggregations (max, min) across multiple days.

This test exposes a regression introduced in commit 668ea64 where the
total was always computed using SUM, even for meters configured with
MAX, MIN, or other non-summable aggregations.

Example of the bug:
- Meter with MAX aggregation over 3 days
- Day 1: max = 20, Day 2: none = 0, Day 3: max = 15
- Expected total: max(20, 0, 15) = 20
- Actual (buggy) total: sum(20 + 0 + 15) = 35
Fix regression introduced in commit 668ea64 where the `total` field in
MeterQuantities was always computed using SUM, even for meters
configured with non-summable aggregations like MAX or MIN.

The fix determines the appropriate SQL window function based on whether
the meter's aggregation is summable:
- For summable aggregations (count, sum): continue using func.sum
- For non-summable aggregations (max, min): use the meter's aggregation
  function (e.g., func.max for MAX aggregation)

Example of the bug this fixes:
- Meter with MAX aggregation tracking "total_enabled_servers"
- Daily values over 30 days: each day reports max of 1
- Expected total: max(1, 1, ..., 1) = 1
- Buggy total: sum(1 + 1 + ... + 1) = 30

Note: avg and unique aggregations require special handling (weighted
averages and deduplication respectively) that is not addressed here.
@vercel
Copy link

vercel bot commented Jan 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Review Updated (UTC)
polar Ignored Ignored Preview Jan 6, 2026 7:33am
polar-sandbox Ignored Ignored Preview Jan 6, 2026 7:33am

@pieterbeulque pieterbeulque requested a review from Yopi January 5, 2026 21:08
Copy link
Contributor

@Yopi Yopi left a comment

Choose a reason for hiding this comment

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

Makes sense, yes

@pieterbeulque pieterbeulque merged commit cde2f46 into main Jan 7, 2026
14 checks passed
@pieterbeulque pieterbeulque deleted the claude/investigate-meter-api-issue-kxEEr branch January 7, 2026 14:55
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