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

feat(metrics/histogram): 🍪 count() and sum() accessors #242

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cratelyn
Copy link
Contributor

fixes #241.

this commit introduces two new public methods to Histogram; sum() and count() return the sum of all observations and the number of observations made, respectively.

fixes prometheus#241.

this commit introduces two new public methods to `Histogram`; `sum()`
and `count()` return the sum of all observations and the number of
observations made, respectively.

Signed-off-by: katelyn martin <[email protected]>
cratelyn added a commit to linkerd/linkerd2-proxy that referenced this pull request Nov 20, 2024
see:

* prometheus/client_rust#242
* prometheus/client_rust#241

for now, refactor this test so that it gates all use of the (proposed)
`sum()` and `count()` accessors behind a temporary feature gate.

Signed-off-by: katelyn martin <[email protected]>
cratelyn added a commit to linkerd/linkerd2-proxy that referenced this pull request Nov 20, 2024
see:

* prometheus/client_rust#242
* prometheus/client_rust#241

for now, refactor this test so that it gates all use of the (proposed)
`sum()` and `count()` accessors behind a temporary feature gate.

Signed-off-by: katelyn martin <[email protected]>
cratelyn added a commit to linkerd/linkerd2-proxy that referenced this pull request Nov 20, 2024
see:

* prometheus/client_rust#242
* prometheus/client_rust#241

for now, refactor this test so that it gates all use of the (proposed)
`sum()` and `count()` accessors behind a temporary feature gate.

Signed-off-by: katelyn martin <[email protected]>
cratelyn added a commit to linkerd/linkerd2-proxy that referenced this pull request Nov 20, 2024
* feat(app): Backend response frame count metrics

this introduces a new tower middleware for Prometheus metrics, used for
instrumenting HTTP and gRPC response bodies, and observing (a) the
number of frames yielded by a body, and (b) the number of bytes included
in body frames, and (c) the distribution of frame sizes.

this middleware allows operators to reason about how large or small the
packets being served in a backend's response bodies are.

a route-level middleware that instruments request bodies will be added
in a follow-on PR.

 ### 📝 changes

an overview of changes made here:

* the `linkerd-http-prom` has a new `body_data` submodule. it exposes
  `request` and `response` halves, to be explicit about which body is
  being instrumented on a `tower::Service`.

* the `linkerd-http-prom` crate now has a collection of new
  dependencies: `bytes` is added as a dependency in order to inspect the
  data chunk when the inner body yields a new frame. `futures-util` and
  `http-body` are added as dev-dependencies for the accompanying test
  coverage.

* body metrics are affixed to the `RouteBackendMetrics<L>` structure,
  and registered at startup.

Signed-off-by: katelyn martin <[email protected]>

* review: Inline attribute to service passthrough

Signed-off-by: katelyn martin <[email protected]>

* review: Inline attribute to body passthrough

continuing this theme of inlining, we inline the passthrough methods on
`Body` as well.

Signed-off-by: katelyn martin <[email protected]>

* review: Box `<RecordBodyData as Service>::Future` values

Signed-off-by: katelyn martin <[email protected]>

* review: rename `get()` to `metrics()`

Signed-off-by: katelyn martin <[email protected]>

* review: simplify `RecordBodyData<S>` response

Signed-off-by: katelyn martin <[email protected]>

* Update ResponseBody metrics to use a histogram

Signed-off-by: Oliver Gould <[email protected]>

* refactor(tests): feature gate frame size histogram assertions

see:

* prometheus/client_rust#242
* prometheus/client_rust#241

for now, refactor this test so that it gates all use of the (proposed)
`sum()` and `count()` accessors behind a temporary feature gate.

Signed-off-by: katelyn martin <[email protected]>

---------

Signed-off-by: katelyn martin <[email protected]>
Signed-off-by: Oliver Gould <[email protected]>
Co-authored-by: Oliver Gould <[email protected]>
cratelyn added a commit to linkerd/linkerd2-proxy that referenced this pull request Nov 21, 2024
 ### ⛅ overview

this introduces a new tower middleware for Prometheus metrics, used for
instrumenting HTTP and gRPC request bodies, and observing (a) the
number of frames yielded by a body, (b) the number of bytes included
in body frames, and (c) a distribution of the size of frames yielded.

this builds upon the backend-level metrics added in #3308. this
additionally uses the route label extractor, hoisted out of the retry
middleware's Prometheus telemetry in #3337.

 ### 📝 changes

* a `linkerd_http_prom::body_data::request::NewRecordBodyData::NewRecordBodyData`
  middleware is added, which complements the equivalent
  `linkerd_http_prom::body_data::response` middleware.

* this is added to policy routes' metrics layer.

see prometheus/client_rust#241 and prometheus/client_rust#242, which
track upstream proposals to add accessors to `Histogram` that will allow
us to make test assertions that metrics are working properly. for now,
these are feature gated as also done in #3308.

Signed-off-by: katelyn martin <[email protected]>
cratelyn added a commit to linkerd/linkerd2-proxy that referenced this pull request Nov 21, 2024
 ### ⛅ overview

this introduces a new tower middleware for Prometheus metrics, used for
instrumenting HTTP and gRPC request bodies, and observing (a) the
number of frames yielded by a body, (b) the number of bytes included
in body frames, and (c) a distribution of the size of frames yielded.

this builds upon the backend-level metrics added in #3308. this
additionally uses the route label extractor, hoisted out of the retry
middleware's Prometheus telemetry in #3337.

 ### 📝 changes

* a `linkerd_http_prom::body_data::request::NewRecordBodyData::NewRecordBodyData`
  middleware is added, which complements the equivalent
  `linkerd_http_prom::body_data::response` middleware.

* this is added to policy routes' metrics layer.

see prometheus/client_rust#241 and prometheus/client_rust#242, which
track upstream proposals to add accessors to `Histogram` that will allow
us to make test assertions that metrics are working properly. for now,
these are feature gated as also done in #3308.

Signed-off-by: katelyn martin <[email protected]>
cratelyn added a commit to linkerd/linkerd2-proxy that referenced this pull request Nov 21, 2024
### ⛅ overview

this introduces a new tower middleware for Prometheus metrics, used for
instrumenting HTTP and gRPC request bodies, and observing (a) the
number of frames yielded by a body, (b) the number of bytes included
in body frames, and (c) a distribution of the size of frames yielded.

this builds upon the backend-level metrics added in #3308. this
additionally uses the route label extractor, hoisted out of the retry
middleware's Prometheus telemetry in #3337.

 ### 📝 changes

* a `linkerd_http_prom::body_data::request::NewRecordBodyData::NewRecordBodyData`
  middleware is added, which complements the equivalent
  `linkerd_http_prom::body_data::response` middleware.

* this is added to policy routes' metrics layer.

see prometheus/client_rust#241 and prometheus/client_rust#242, which
track upstream proposals to add accessors to `Histogram` that will allow
us to make test assertions that metrics are working properly. for now,
these are feature gated as also done in #3308.

Signed-off-by: katelyn martin <[email protected]>
@cratelyn
Copy link
Contributor Author

cratelyn commented Dec 9, 2024

👋 hello! this pull request in particular would be very helpful for our usage of Histograms. i would love if you could take a look at this soon, i believe it's a small and non-controversial addition to the Histogram interface.

@mxinden
Copy link
Member

mxinden commented Dec 13, 2024

I am hesitant exposing the internals of Histogram just for the use-case of testing. Unless we come up with better use-cases, I don't think we should merge here.

@olix0r
Copy link

olix0r commented Dec 13, 2024

@mxinden I'm sympathetic to maintaining a minimal public API, but it's pretty important to us that we can write tests that ensure that our metrics are properly instrumented. For example:

https://github.com/linkerd/linkerd2-proxy/blob/ea5136268d65aa6a570cb612150875f923067042/linkerd/app/outbound/src/http/logical/policy/route/backend/metrics/tests.rs#L224-L237

Would you be amenable to feature-gating these public helpers behind a test-util feature? This is a pretty common approach we've seen in other crates.

Or are there alternative approaches you recommend? Would it be better to expose a module akin to github.com/prometheus/client_golang/prometheus/testutil to make assertions about a registry?

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.

Histogram does not provide accessors
3 participants