discourse / prometheus_exporter

A framework for collecting and aggregating prometheus metrics
MIT License
532 stars 154 forks source link

Allow collecting web metrics as histograms #186

Closed jferris closed 3 years ago

jferris commented 3 years ago

Web metrics are currently reported as summaries, which provides precise data for each controller, action, and node. However, timings from low-throughput actions will cause an average to be wildly inaccurate, which means aggregating across controllers, actions, and nodes is not currently possible.

This adds an option to the middleware which instructs the collector to use histogram metrics rather than summaries, which allows estimating aggregates using histogram_quantile.

Resolves #92.

SamSaffron commented 3 years ago

Hmm, tricky, I think a startup flag is best here, I like the idea of dynamic config but it introduces a new security concern

On Tue, 14 Sep 2021 at 10:50 pm, Joe Ferris @.***> wrote:

@.**** commented on this pull request.

In lib/prometheus_exporter/middleware.rb https://github.com/discourse/prometheus_exporter/pull/186#discussion_r708234789 :

@@ -41,7 +42,8 @@ def call(env) type: "web", timings: info, queue_time: queue_time,

  • default_labels: default_labels(env, result)
  • default_labels: default_labels(env, result),
  • options: { mode: @mode }

Sure; the two ways I can think of to do this would be:

  1. Add a startup flag to prometheus_exporter to set the default aggregation type (ie --histogram)
  2. Add an endpoint to the collector for accepting config changes (ie PATCH /config)

Any preference or other suggestions?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/discourse/prometheus_exporter/pull/186#discussion_r708234789, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABIXKN5GH4S2DHN4GQ2DDUB5AJBANCNFSM5D6AJN3Q .

jferris commented 3 years ago

@SamSaffron - I pushed an update using a --histogram flag rather than an option for the middleware.

SamSaffron commented 3 years ago

This is great, thanks @jferris

svenjr commented 2 years ago

When will this get released? I was trying to implement it using v0.8.1 of the gem but now see this was implemented after that release. Is there a release planned? Thanks