feat: summary metric #47

Karrq · 2025-11-18T17:25:35Z

Closes #20

This PR also includes some other small improvements:

split the existing metric types in separate modules
add a simple Justfile
move large examples from README.md to dedicated examples files
enforce help string when using prometric-derive
dedicated handling for histograms/summary "partitions" in prometric-derive
prometric-derive now respects user's specified paths for metric types

TODO:

Add examples
Add support in prometric-derive
Properly implement the glue code from metrics-util's Summary for SummaryProvider
Add more tests

deps: metrics-util for Summary logic feat: GenericSummary, converts into prometheus proto's Summary feat: SummaryVecBuilder enabling SummaryVec feat: SummaryVec, mirrors HistogramVec

fix: clippy lints

refactor(derive): `buckets`/`quantiles` attributes and invariant checks

feat(example): add Summary usage

fix(examples): add missing help strings

prometric/src/summary/core.rs

feat(summary): rolling summary

feat(summary): ConcurrentSummaryProvider docs: improve documentation chore: cleanup `core` dead file deps: feature-gate summary deps

chore: cleanup unused MetricType::Summary.1 field test: ensure quantile defaults work chore: remove unused `GenericSummary::observe` refactor: rename `GenericSummary::concurrent_observe` to `observe`

prometric/src/summary/batching.rs

prometric/src/summary/generic.rs

mempirate

First pass, primarily some style stuff. Very nice job

README.md

mempirate · 2025-11-20T10:02:12Z

prometric-derive/src/expand.rs

+        &self,
+        maybe_buckets: Option<syn::Expr>,
+        maybe_quantiles: Option<syn::Expr>,
+    ) -> Result<Partitions> {


This could return Result<Option<Partitions>> and we can remove the Partitions::{None, NotApplicable} variants. Up to you

I thought about it, but decided to go with more explicit variants, to preserve the semantics better, if one day we need to change this I'd rather have it much clearer

Can you elaborate more on how you preserve the semantics better?

To indicate that this parsed item is specifically for this usecase, but also if we want to make hard errors out of "NotApplicable" we could, for example if it was provided and we were processing a Counter

I can rework it to be an option, in the end it's a small difference, it's just that when parsing I prefer to be explicit about the various options, especially since we have an enum already (for buckets/quantiles, which ok could be just the inner syn::Expr)

prometric-derive/src/expand.rs

prometric/src/summary/batching.rs

prometric/src/summary/rolling.rs

prometric/src/summary/batching.rs

fix: accidental Arc clone

Karrq · 2025-11-20T11:53:50Z

Changed to ArcCell but it's still not working correctly (test is failing)
I think I will have to change the approach a bit, I believe those .load calls are actually cloning the Arc sometimes :/

test(arccell): check invariants are upheld

refactor: new batch reserves

prometric/src/summary/batching.rs

merklefruit

Awesome work! Leaving some questions to understand for myself 🙂

prometric/src/summary/batching.rs

Co-authored-by: nicolas <48695862+merklefruit@users.noreply.github.com>

fix: deadlock with cloned GenericSummary (not a full clone) fix(summary:generic): usage with MetricVecBuilder test: summary + macro smoke test refactor: SummaryProvider -> NonConcurrentSummaryProvider refactor: ConcurrentSummaryProvider -> SummaryProvider fix: relax trait bounds where possible

lint: clippy

test: add concurrent Summary metric test

mempirate

LGTM, some minor things but approved. I would like to see this tested in the real world before we do a release, can work on that this week in one of the projects.

mempirate · 2025-11-24T08:29:56Z

prometric/src/summary/batching.rs

+    }
+}
+
+// TODO: switch to FixedVec


FixedVec because we know the number of observations before committing I suppose? Would it be more efficient?

Yes, Fixed in terms of fixed capacity https://docs.rs/orx-concurrent-vec/latest/orx_concurrent_vec/struct.FixedVec.html

The number of measurements will be at most batch size (or a few over those) so we could preallocate the required capacity instead of having the ConcurrentVec grow

prometric/src/summary/generic.rs

prometric/src/summary/mod.rs

prometric/src/summary/rolling.rs

thedevbirb

Minor things

prometric-derive/src/expand.rs

thedevbirb · 2025-11-24T09:24:07Z

prometric-derive/src/expand.rs

+        &self,
+        maybe_buckets: Option<syn::Expr>,
+        maybe_quantiles: Option<syn::Expr>,
+    ) -> Result<Partitions> {


Can you elaborate more on how you preserve the semantics better?

thedevbirb · 2025-11-24T09:40:26Z

prometric/src/summary/simple.rs

+/// A simple Summary metric implementation
+#[derive(Debug, Clone)]
+pub struct SimpleSummary {


What does "Simple" mean here? Overall the distinction between rolling and simple isn't super clear, I have a rough idea but I want to be sure.

Added a comment, but it's to differentiate it from Rolling.
The rolling one has a configured expiry duration so values past that duration are expired and removed, thus not returned from the snapshot, and thus not used for quantile computation

thedevbirb · 2025-11-24T09:40:54Z

prometric/src/summary/mod.rs

+            if matches!(e, prometheus::Error::AlreadyReg) {
+                registry
+                    .unregister(boxed.clone())
+                    .unwrap_or_else(|_| panic!("Failed to unregister metric {id}"));


Is it okay to panic here because this is code running at compile time?

This is not running at compile time, it's run during the metric initialization (when building it)
I copied it from the other metrics, which do the same.

This scenario can happen if the collector was not registered, which is also not possible because we just got "AlreadyReg" error, indicating that it was indeed registered.
In the same way, the register function errors when the collector was already registered, which we just unregistered.

Looking inside the function, it seems like the only other error (from register) other than AlreadyReg would just be a configuration error: descriptor with same name but different help, for example, which is not something recoverable really

refactor: generic extraction method for readability docs: explain RollingSummary docs: explain SimpleSummary fix(rolling): return total count in snapshot

thedevbirb

Great job!

Karrq added 3 commits November 18, 2025 14:38

refactor: split metrics in modules

8d84128

chore: add justfile

07e6a68

feat: summary metric

7387ffa

deps: metrics-util for Summary logic feat: GenericSummary, converts into prometheus proto's Summary feat: SummaryVecBuilder enabling SummaryVec feat: SummaryVec, mirrors HistogramVec

Karrq requested a review from mempirate November 18, 2025 17:25

Karrq self-assigned this Nov 18, 2025

Karrq added 6 commits November 18, 2025 22:58

refactor: export 2 more items

f1ba237

fix: clippy lints

feat(derive): Summary metric support

a1b584c

refactor(derive): `buckets`/`quantiles` attributes and invariant checks

refactor(derive): quote fully qualified imports

b6a51de

refactor(README): move examples under examples/

e17c6ca

feat(example): add Summary usage

fix(derive): enforce metric help string

270c28f

fix(examples): add missing help strings

chore: fmt

8d5d4e0

Karrq marked this pull request as ready for review November 18, 2025 22:41

fix(derive): error with invalid partitions config

c3ab4af

merklefruit reviewed Nov 19, 2025

View reviewed changes

prometric/src/summary/core.rs Outdated Show resolved Hide resolved

Karrq added 6 commits November 19, 2025 15:42

refactor(summary): split into more modules

db9d262

feat(summary): rolling summary

feat(summary): batching summary

9e0dd73

feat(summary): ConcurrentSummaryProvider docs: improve documentation chore: cleanup `core` dead file deps: feature-gate summary deps

Merge remote-tracking branch 'origin/main' into feat/summary

6255729

feat(derive): expand metrics with user path

7e7ea94

chore: cleanup unused MetricType::Summary.1 field test: ensure quantile defaults work chore: remove unused `GenericSummary::observe` refactor: rename `GenericSummary::concurrent_observe` to `observe`

chore: fmt

7c59f9b

fix: clippy

9f9ceef

merklefruit reviewed Nov 20, 2025

View reviewed changes

prometric/src/summary/batching.rs Outdated Show resolved Hide resolved

merklefruit reviewed Nov 20, 2025

View reviewed changes

prometric/src/summary/batching.rs Outdated Show resolved Hide resolved

merklefruit reviewed Nov 20, 2025

View reviewed changes

prometric/src/summary/generic.rs Show resolved Hide resolved

mempirate reviewed Nov 20, 2025

View reviewed changes

mempirate requested changes Nov 20, 2025

View reviewed changes

prometric/src/summary/batching.rs Show resolved Hide resolved

Karrq added 5 commits November 20, 2025 11:51

feat: ArcCell

da0d4d8

fix: accidental Arc clone

fix: BatchOpts, unnecessary const-hack

c618c40

fix: use appropriate parse_quote macro

4a1fbcb

docs(macro): ensure generated docs link to items

745fb80

fix(README): reintroduce basic (smaller) example

103ae8b

test(batch): concurrent_observe

1a4927b

Karrq added 3 commits November 20, 2025 15:31

fix(arccell): account for outstanding loads

85d4946

test(arccell): check invariants are upheld

fix(batching): commit deadlock

da57d6e

refactor: new batch reserves

docs: clarify comment

20ffb66

Karrq requested review from mempirate and merklefruit November 20, 2025 15:29

Karrq added 2 commits November 20, 2025 16:50

test(arccell): avoid concurrency noise

5b6d889

refactor: use existing arc-cell

d570a62

Karrq requested a review from thedevbirb November 21, 2025 08:37

mempirate reviewed Nov 21, 2025

View reviewed changes

prometric/src/summary/batching.rs Outdated Show resolved Hide resolved

merklefruit reviewed Nov 21, 2025

View reviewed changes

prometric/src/summary/batching.rs Outdated Show resolved Hide resolved

prometric/src/summary/batching.rs Show resolved Hide resolved

prometric/src/summary/batching.rs Show resolved Hide resolved

Karrq and others added 6 commits November 21, 2025 15:27

Update prometric/src/summary/batching.rs

890c27d

Co-authored-by: nicolas <48695862+merklefruit@users.noreply.github.com>

test: remove tokio

02ec8a3

test: generic summary smoke tests

0869556

chore: fmt

765a260

lint: clippy

fix: clippy

d91be06

test: add concurrent Summary metric test

Karrq requested a review from mempirate November 21, 2025 17:08

mempirate approved these changes Nov 24, 2025

View reviewed changes

fix: pr review

3c03576

thedevbirb approved these changes Nov 24, 2025

View reviewed changes

Karrq added 2 commits November 24, 2025 17:03

docs: more comments in prometric-derive

aeb342d

refactor: generic extraction method for readability docs: explain RollingSummary docs: explain SimpleSummary fix(rolling): return total count in snapshot

chore: fmt

5837ce5

thedevbirb approved these changes Nov 25, 2025

View reviewed changes

mempirate merged commit 80f8352 into main Nov 25, 2025
4 checks passed

mempirate deleted the feat/summary branch November 25, 2025 08:54

mempirate mentioned this pull request Nov 25, 2025

Perf: use orx FixedVec #48

Open

github-actions bot mentioned this pull request Nov 25, 2025

feat: release 0.2.0 #40

Merged

feat: summary metric #47

feat: summary metric #47

Uh oh!

Conversation

Karrq commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mempirate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Karrq commented Nov 20, 2025

Uh oh!

Uh oh!

merklefruit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mempirate left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thedevbirb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thedevbirb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Karrq commented Nov 18, 2025 •

edited

Loading