Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API for creating metrics with specific buckets or quantiles #76

Open
ktoso opened this issue Sep 23, 2020 · 10 comments
Open

API for creating metrics with specific buckets or quantiles #76

ktoso opened this issue Sep 23, 2020 · 10 comments
Assignees
Labels
kind/enhancement Improvements to existing feature.
Milestone

Comments

@ktoso
Copy link
Member

ktoso commented Sep 23, 2020

Some more users often need to pre-configure their metrics with pre-determined bucket sizes (and numbers) or quantiles they want to report. This matters most for metrics which do some form of aggregation client side, like prometheus (e.g. https://github.com/MrLotU/SwiftPrometheus/blob/master/Sources/Prometheus/Prometheus.swift#L210 ).

We should make it possible to use the SwiftMetrics API without dropping to raw APIs and still be able to configure bucket sizes etc. These are a common and not specific-backend bound configuration parameter.

It could look something like:

Timer(
    label: "hello", 
    dimensions: [("a": "aaa")], 
    buckets: [.005, .01, .025, .05, .075, .1, .25, .5, .75, 1, 2.5, 5, 7.5, 10]
)
Timer(
    label: "hello",
    dimensions: [("a": "aaa")], 
    buckets: .linear([.100, .100, 100)]
)
Timer(
    label: "hello",
    dimensions: [("a": "aaa")], 
    buckets: .exponential([1, 2, 64)]
)

Timer(
    label: "hello",
    dimensions: [("a": "aaa")], 
    quantiles: [0.5, 0.05, 0.9, 0.01]
)

Use case discussed with @avolokhov, we'll iterate on the exact needs and shapes of this soon

@ktoso ktoso added the kind/enhancement Improvements to existing feature. label Sep 23, 2020
@ktoso ktoso added this to the 2.1.0 milestone Sep 23, 2020
@ktoso
Copy link
Member Author

ktoso commented Sep 23, 2020

FYI @MrLotU as well :-)

@MrLotU
Copy link
Contributor

MrLotU commented Sep 23, 2020

Big +1! This would be useful for both Timer as well as Recorder (Summary and Histogram in Prometheus land)

@ktoso
Copy link
Member Author

ktoso commented Sep 23, 2020

Right, sounds good Recorder as well 👍

@ktoso ktoso self-assigned this Sep 23, 2020
@tomerd
Copy link
Member

tomerd commented Sep 23, 2020

thanks for bringing this up. what would backends that do not support such feature do with this information?

@avolokhov
Copy link

I'm not sure if it sounds crazy, but I have this thought in mind:
Imagine you want to instrument you, say, http client library with a request payload size histogram.
With the old library, you create a bunch of metrics and feed the data to it.
With a new library you see an api that expects quantiles and histograms. Being a library creator you have 0 knowledge about how this library is going to be used (and it may vary a lot - average payload may be as low as a few bytes or as high as a few gigabytes). So you have to either guess, or drop the idea in a fear that misconfigured metrics may hurt.

@ktoso
Copy link
Member Author

ktoso commented Sep 24, 2020

I think there's really only two ways out, one of them being as Anton suggests.

Option 1) backends may ignore this information silently

So given the example:

let timer = Timer(
    label: "hello", 
    dimensions: [("a": "aaa")], 
    buckets: [.005, .01, .025, .05, .075, .1, .25, .5, .75, 1, 2.5, 5, 7.5, 10]
)

Say, statsd client would simply ignore it. It directly emits values without aggregating, so the setting has no effect.
Silently ignoring config is a bit meh, but not the end of the world here actually... c'est la vie, backends to the best they can with the passed in information. 🤔

Option 2) optional initializers

We could make those new initializers failable, then they'd return nil if unable to support this feature, usage becomes more verbose then:

guard let timer = Timer(
    label: "hello", 
    dimensions: [("a": "aaa")], 
    buckets: [.005, .01, .025, .05, .075, .1, .25, .5, .75, 1, 2.5, 5, 7.5, 10]
) else { fatalError("OH NO, I DEFINITELY NEED BUCKETS") }

or, falling back to non buckets if not necessary

let timer = Timer(
    label: "hello", 
    dimensions: [("a": "aaa")], 
    buckets: [.005, .01, .025, .05, .075, .1, .25, .5, .75, 1, 2.5, 5, 7.5, 10]
) ?? Timer(label: "hello", dimensions: [("a": "aaa")]) // "ok, i don't actually care/need buckets"

The option 2 is technically more correct but seems like a large burden and could accidentally make some libraries not usable with some backends, only because they took the "oh well, lets crash" approach.

We could try to make option 2 spell somewhat nicer, if we really believe in it?

Personally I think Option 1 is fine... I'll check what similar APIs do in the Java ecosystem though

@ktoso
Copy link
Member Author

ktoso commented Sep 24, 2020

  • OpenTelemetry Metrics do not define ways to configure such details.
    • otel discussions I've found are pretty inconclusive and ignoring such details to be honest.
  • APIs like dropwizard metrics don't expose this configuration detail in the metric types.

Alternatively, we could double down and design some form of Metric(..., configured: ...) which would allow specific instrumentations to "add" and "pass through" the APIs but that looks pretty nasty to be honest.

@tomerd
Copy link
Member

tomerd commented Oct 13, 2020

@ktoso did you want to do anything here for 2.1.0, or should we release 2.1.0 sooner?

@ktoso
Copy link
Member Author

ktoso commented Oct 13, 2020

We can cut at any time as far as I'm concerned - this does not have to be in 2.1.0 :) you can cut or I'll do tomorrow

@ktoso ktoso modified the milestones: 2.1.0, 2.2.0 Oct 14, 2020
@ktoso ktoso modified the milestones: 2.2.0, 2.3.0 Aug 16, 2021
@ktoso ktoso modified the milestones: 2.3.0, 2.3.3 Jul 21, 2022
@ktoso ktoso modified the milestones: 2.3.3, 2.3.4 Dec 7, 2022
@ktoso ktoso modified the milestones: 2.3.4, 2.3.5 Feb 2, 2023
@ktoso ktoso modified the milestones: 2.4.0, 2.5.0 May 30, 2023
@blindspotbounty
Copy link

I believe this feature would be nice to have.
Though, if some backends (like swift prometheus) which actually supports that, can be used by providing implementation handlers to Recorder/Timer instead of having it in swift-metrics frontend api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements to existing feature.
Projects
None yet
Development

No branches or pull requests

5 participants