[Discussion] Server Metrics API

Thanks for the writeup and APIs Tom!

There's a slight mismatch between the code and proposal text: if there needs to be Metrics.global.makeCounter (as sources indicate) or if we provide the extensions which do this for us and allow Metrics.makeCounter as the proposal contains in the text. I don't see the hop over global change much, as it is visibly global already so it think this was an omission, added missing extensions here: Align proposal code with proposal README, add missing make* on Metrics by ktoso ยท Pull Request #5 ยท tomerd/swift-server-metrics-api-proposal ยท GitHub


As for the proposal itself:

What is your evaluation of the proposal? [added from the questions in the previous Logging proposal, good question IMHO :slight_smile:]

Overall I'm in favor of the proposal :+1:

Some smaller things still need fleshing out though from my perspective, see below.
The types and initialization look fine, and it seems we can express all typical needs and build on top of those core types.

if anything, what does this proposal not cover that you will definitely need

I believe we are missing explicit lifecycle, in the sense that without defining some way to release() even if many implementations would have this as noop we risk having the API be potentially leaky once an impl arrives that needs eager releasing.

I explain the need for explicit lifecycle in Consider a form of "unregister"/"destroy" for metrics ยท Issue #6 ยท tomerd/swift-server-metrics-api-proposal ยท GitHub and the illustrating PR: [reviving the PR today] Showcase release() for metrics #6 by ktoso ยท Pull Request #7 ยท tomerd/swift-server-metrics-api-proposal ยท GitHub If we agree that we need lifecycle I'd stick to the style I proposed here where we can metrics.release(metric) rather what I initially though of which was (metric.release(), which makes the metric objects needlessly heavier).

The rationale is that for systems that poll or cache metrics and may contain heavy structures (high resolution histograms), we really want to be in charge when to release them. And this has to be in the common API, as otherwise 3rd party libraries will not be able to abide to these if a metrics library comes in which needs lifecycle (e.g. prometheus is an example).

if anything, what could we remove from this and still be happy?

I believe we should not ship with a caching proxy (CachingMetricsHandler ) like is done in: swift-server-metrics-api-proposal/Metrics.swift at master ยท tomerd/swift-server-metrics-api-proposal ยท GitHub since it cannot assume things about the metrics types it falls back to very simple caching, and releasing resources is quite heavy then. Real implementations would know their metrics types exactly (and IDs on which they store them), and can therefore implement the cache more efficiently and smartly (e.g. yes, release, but only once the metric has been scraped at least once by a collector etc).

API-wise: what do you like, what don't you like?

I'll avoid the naming bikeshed :slight_smile: The names are good enough.

I think we should align naming of the "MUX" with what the logging proposal has; i.e. MultiplexingLogHandler -> MultiplexingMetricsHandler.

The MetricsHandler interface seems good enough to build all kinds of abstractions on top of it.

The initializing as bootstrap is fine, as it allows to be similar to logging.

Overall I think this is heading towards a solid direction, and I look forward to the last polishing touchups.

2 Likes