fix: send and store real sub-second emission durations - #1374
Open
davidberenstein1957 wants to merge 2 commits into
Open
fix: send and store real sub-second emission durations#1374davidberenstein1957 wants to merge 2 commits into
davidberenstein1957 wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1374 +/- ##
=======================================
Coverage 91.43% 91.43%
=======================================
Files 49 49
Lines 5057 5057
=======================================
Hits 4624 4624
Misses 433 433 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
davidberenstein1957
marked this pull request as ready for review
August 13, 2026 05:05
davidberenstein1957
force-pushed
the
fix/subsecond-emission-duration
branch
from
August 19, 2026 09:18
dfe1c2f to
ed11471
Compare
The `duration` field was typed `int` in the pydantic schemas while the DB column and ORM were always `Float`, and `ApiClient.add_emission` dropped any measurement shorter than one second. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
davidberenstein1957
force-pushed
the
fix/subsecond-emission-duration
branch
from
August 19, 2026 14:12
ed11471 to
735be78
Compare
davidberenstein1957
added a commit
that referenced
this pull request
Aug 19, 2026
Adds an ASGI middleware that gives each HTTP request its share of a long-running tracker's energy, plus the attribution model behind it. One tracker runs for the app's lifetime. Each completed sampling window (t_prev, t_now, dE) is split across the requests in flight during it, weighted by their overlap with the window and normalised by the sum of the weights. Windows with nothing in flight are recorded as unattributed. The invariant attributed + unattributed == settled holds exactly after every window, and is what the concurrency test pins down. Why not per-request start/stop energy snapshots: with N requests in flight each request observes the whole machine's delta, so the sum overcounts by roughly N - measured up to 88x at 100 concurrent requests. Fair-share weighting is the only split that conserves the run total. A request's share is only known one or more sampling windows after its response was sent, so results are reported then, via a callback. A request that never covered a completed window reports energy_kwh=None rather than zero: there is no honest number for it. Tracker side: add_energy_window_observer / remove_energy_window_observer expose the sampling windows, and http_request_emissions() scales the run's EmissionsData down to one attributed share using the run's accumulated component ratios and carbon intensity. Depends on #1374 (duration int -> float in the emissions schemas, and dropping the duration < 1 send guard) and #1375 (scheduler pause handling around tasks). Both are carried by their own PRs rather than duplicated here, so this should merge after them. Deliberately left out, to keep the diff reviewable: hardware-tier gating of which backends can resolve a sampling window, include/exclude path filtering (endpoint labelling is two lines inline), idle-baseline subtraction, per-endpoint aggregation, routing per-request rows into the tracker's own CSV/API output handlers, a lifespan helper, and a dedicated docs page. Each is additive on top of this and can follow if there is demand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
davidberenstein1957
added a commit
that referenced
this pull request
Aug 19, 2026
Adds an ASGI middleware that gives each HTTP request its share of a long-running tracker's energy, plus the attribution model behind it. One tracker runs for the app's lifetime. Each completed sampling window (t_prev, t_now, dE) is split across the requests in flight during it, weighted by their overlap with the window and normalised by the sum of the weights. Windows with nothing in flight are recorded as unattributed. The invariant attributed + unattributed == settled holds exactly after every window, and is what the concurrency test pins down. Why not per-request start/stop energy snapshots: with N requests in flight each request observes the whole machine's delta, so the sum overcounts by roughly N - measured up to 88x at 100 concurrent requests. Fair-share weighting is the only split that conserves the run total. A request's share is only known one or more sampling windows after its response was sent, so results are reported then, via a callback. A request that never covered a completed window reports energy_kwh=None rather than zero: there is no honest number for it. Tracker side: add_energy_window_observer / remove_energy_window_observer expose the sampling windows, and http_request_emissions() scales the run's EmissionsData down to one attributed share using the run's accumulated component ratios and carbon intensity. Depends on #1374 (duration int -> float in the emissions schemas, and dropping the duration < 1 send guard) and #1375 (scheduler pause handling around tasks). Both are carried by their own PRs rather than duplicated here, so this should merge after them. Deliberately left out, to keep the diff reviewable: hardware-tier gating of which backends can resolve a sampling window, include/exclude path filtering (endpoint labelling is two lines inline), idle-baseline subtraction, per-endpoint aggregation, routing per-request rows into the tracker's own CSV/API output handlers, a lifespan helper, and a dedicated docs page. Each is additive on top of this and can follow if there is demand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracted from #1203 (
feat/add-fastapi-middleware), which bundled this with unrelated FastAPI middleware work. It stands alone and can be reviewed and merged independently; #1203 will be rebased to drop the duplicated hunks.What
durationbecomesfloatinstead ofintincodecarbon/core/schemas.pyandcarbonserver/carbonserver/api/schemas.py(EmissionBase).ApiClient.add_emissionno longer refuses emissions shorter than one second, and no longer truncates the duration withint(...). It does still skip a non-positive duration: the server declaresdurationasField(..., gt=0), so a zero-length flush would be a 422. Skipping rather than clamping, because clamping would invent a duration that was never measured.ExperimentReport,ProjectReportandOrganizationReportwidendurationfrominttofloat.Why
The DB column and the ORM were always
Column(Float)(carbonserver/carbonserver/api/infra/database/sql_models.py) — only the pydantic models claimedint. So nothing about storage changes here; the schemas are simply being made honest about what the database already holds. On the client side the< 1guard silently dropped any measurement shorter than a second, which is every short-lived task.The report widening is not cosmetic: those endpoints
SUM()a Float column into a field declaredint. Today every stored duration happens to be a whole number, so it validates by luck. The first sub-second duration in the database makes the sum non-integral and the response model raises a validation error — a latent 500 on the experiment/project/organization report endpoints. Widening the reports is therefore part of this fix, not a separate cleanup.No DB migration is needed. The
emissions.durationcolumn is alreadyColumn(Float)incarbonserver/carbonserver/api/infra/database/sql_models.py— this PR only changes the pydantic layer, the storage type is unchanged.Tests
carbonserver/tests/api/test_schema_compatibility.py::test_millisecond_duration_survives_client_to_server— a client payload withduration=0.0042validates against the server schema unchanged.tests/test_api_call.py::TestApi::test_add_emission_sends_millisecond_duration_unchanged— replacestest_add_emission_skips_short_duration; asserts the POST body carries0.0042.tests/test_api_call.py::TestApi::test_add_emission_skips_zero_duration— aduration=0.0flush is dropped client-side and never reaches the server'sgt=0validator.Both fail on
master(ValidationError, andemissions not sent because of a duration smaller than 1) and pass with the fix.uv run pytest tests/ -q --ignore=tests/test_viz_data.py→ 626 passed, 21 skipped. carbonserver unit tests → 108 passed.pre-commit run --all-filesclean.🤖 Generated with Claude Code