Adds method and optional status Labels to PrometheusBackend #1651

kevinmeredith · 2022-12-04T04:32:27Z

Adds method and optional status labels to PrometheusBackend. At work, I was missing these labels out of the box. Having worked on-call in production for 3.5 years, I've found HTTP Methods and Status Codes to be valuable to understand running web services.

As a result, I opened this PR.

I completed the following for the observability/prometheus-backend project. I'm assuming CI/CD will run the overall compile and test.

Before submitting pull request:

Check if the project compiles by running sbt compile
Verify docs compilation by running sbt compileDocs
Check if tests pass by running sbt test
Format code by running sbt scalafmt

It's easier to understand explicit field names with a class than via the hard-to-follow _.X methods.

https://stackoverflow.com/questions/74591438/scala-trait-returning-a-sub-type

…d/or status

adamw · 2022-12-07T11:43:50Z

observability/prometheus-backend/src/main/scala/sttp/client3/prometheus/PrometheusBackend.scala

-            (r: (Request[_, _], Throwable)) => requestToFailureCounterMapper(r._1, r._2),
-            requestToSizeSummaryMapper,
-            (rr: (Request[_, _], Response[_])) => responseToSizeSummaryMapper(rr._1, rr._2),
+            (req: Request[_, _]) => requestToHistogramNameMapper(req).map(addLabelPairs(_, req, None)),


as I understand, this will always add the labels (if they are not present). So it's not possible not to add them - if somebody, for some reason would want that.

Maybe, instead we could modify the default values of parameters in apply so that the user has chance to fully override the behavior?

as I understand, this will always add the labels (if they are not present)

Correct

Maybe, instead we could modify the default values of parameters in apply so that the user has chance to fully override the behavior?

In my 3.5 years of prod on-call, I always found method and status labels to be helpful.

That being said, I'll defer to you and implement this change, i.e. update the default's to consist of method and, optionally, status.

Done - ec2ca4a

adamw · 2022-12-07T11:44:28Z

observability/prometheus-backend/src/main/scala/sttp/client3/prometheus/PrometheusBackend.scala

+    *   BaseCollectorConfig sub-type. Note that PrometheusBackend#apply and PrometheusListener's constructor reference
+    *   the sub-types of BaseCollectorConfig.
+    */
+  def addLabelPairs(config: BaseCollectorConfig, req: Request[_, _], maybeResp: Option[Response[_]]): config.T = {


Maybe a better name would be sth like addMethodStatusLabels? It would reflect more clearly what it does

Good call - will add.

Done - d802a5d

adamw · 2022-12-07T11:45:15Z

observability/prometheus-backend/src/main/scala/sttp/client3/prometheus/PrometheusBackend.scala

@@ -90,7 +143,7 @@ object PrometheusBackend {
      cache.getOrElseUpdate(collectorRegistry, new ConcurrentHashMap[String, T]())
    }

-  type RequestCollectors = (Option[Histogram.Timer], Option[Gauge.Child])
+  final class RequestCollectors(val maybeTimer: Option[Histogram.Timer], val maybeGauge: Option[Gauge.Child])


maybe a case class? also, the optionality is already expressed in the types, so I'd just use timer / gauge

Side comment: I tend to avoid case if there's no need for equality, i.e. class w/ val's meets the bare minimum.

But, adding case is a trivial addition, so I'm happy to add.

Done - 7870222

adamw · 2022-12-07T11:46:07Z

observability/prometheus-backend/src/main/scala/sttp/client3/prometheus/PrometheusBackend.scala

+    *   BaseCollectorConfig sub-type. Note that PrometheusBackend#apply and PrometheusListener's constructor reference
+    *   the sub-types of BaseCollectorConfig.
+    */
+  def addLabelPairs(config: BaseCollectorConfig, req: Request[_, _], maybeResp: Option[Response[_]]): config.T = {


also, maybe a different signature, without the type member: def addLabelPairs[T <: BaseCollectorConfig](cfg: T, ...)?

Yep - let me add that. config.T is a bit ugly, I agree.

Done - 2e97286. Not sure the T#T is any better. Let me know, please.

adamw · 2022-12-07T11:48:27Z

Thanks! I left some comments inline. We should probably make similar changes to the opentelemetry backend (either here or in a separate follow-up issue)

Address softwaremill#1651 (comment)

…ethod and/or status" This reverts commit 1585239.

kevinmeredith · 2023-01-08T05:38:52Z

A few tests are still failing locally for me:

[info] PrometheusBackendTest:
[info] prometheus
[info] - should use default histogram name (120 milliseconds)
[info] - should allow creating two prometheus backends (2 milliseconds)
[info] - should use mapped request to histogram name (4 milliseconds)
[info] - should use mapped request to histogram name with labels and buckets (5 milliseconds)
[info] - should use mapped request to gauge name with labels (6 milliseconds)
[info] - should disable histograms (1 millisecond)
[info] - should use default gauge name *** FAILED *** (15 seconds, 111 milliseconds)
[info]   The code passed to eventually never returned normally. Attempted 107 times over 15.070412125 seconds. Last failure message: The Option on which value was invoked was not defined.. (PrometheusBackendTest.scala:165)
[info] - should use mapped request to gauge name *** FAILED *** (15 seconds, 95 milliseconds)
[info]   The code passed to eventually never returned normally. Attempted 106 times over 15.081156 seconds. Last failure message: The Option on which value was invoked was not defined.. (PrometheusBackendTest.scala:196)
[info] - should disable gauge *** FAILED *** (15 seconds, 94 milliseconds)
[info]   The code passed to eventually never returned normally. Attempted 107 times over 15.075386708 seconds. Last failure message: The Option on which value was invoked was not defined.. (PrometheusBackendTest.scala:228)
[info] - should use default counter name (46 milliseconds)
[info] - should not override user-supplied 'method' and 'status' labels (9 milliseconds)
[info] - should use default summary name (14 milliseconds)
[info] - should use error counter when http error is thrown (23 milliseconds)
[info] - should use failure counter when other exception is thrown (4 milliseconds)
[info] - should use success counter on success response (1 millisecond)

I'm suspicious that they are flaky, but I'll review again another day.

kevinmeredith · 2023-01-08T05:46:37Z

If anything stands out to you, @adamw, on the cause(s) of the failures on #1651 (comment), let me know, please! Thanks

adamw · 2023-01-09T17:09:30Z

@kevinmeredith thanks!

Convert type synonym (tuple) to a class

23435c9

It's easier to understand explicit field names with a class than via the hard-to-follow _.X methods.

kevinmeredith marked this pull request as draft December 4, 2022 04:32

kevinmeredith added 3 commits December 3, 2022 23:36

Refactor BaseCollectorConfig to support 'builder' method

cfd49d5

https://stackoverflow.com/questions/74591438/scala-trait-returning-a-sub-type

add (if not present) label key-value pairs for method and status

c96598b

update PrometheusBackendTest since all metrics will include method an…

1585239

…d/or status

kevinmeredith force-pushed the prometheus-addition branch from 519efb3 to 1585239 Compare December 4, 2022 04:38

kevinmeredith added 2 commits December 3, 2022 23:44

add code comment

580da36

add test verifying user-supplied 'method' and 'status' labels are kept

786ad40

kevinmeredith marked this pull request as ready for review December 4, 2022 04:54

kevinmeredith changed the title ~~draft/WIP: Prometheus addition~~ Adds method and optional status Labels to PrometheusBackend Dec 4, 2022

adamw reviewed Dec 7, 2022

View reviewed changes

kevinmeredith added 7 commits January 7, 2023 23:25

move adding status and method labels to defaults - not always

ec2ca4a

Address softwaremill#1651 (comment)

don't add method or status label if already present, case insensitive

e282a4b

class -> case class for RequestCollectors

7870222

re-name: addLabelPairs -> addMethodStatusLabels

d802a5d

refactor addMethodStatusLabels

2e97286

Revert "update PrometheusBackendTest since all metrics will include m…

2c8aefc

…ethod and/or status" This reverts commit 1585239.

fix up PrometheusBackendTest per refactor

c5ebc5f

adamw added 2 commits January 9, 2023 16:17

Fix compile for Scala3

41efc89

Split addMethodLabel and addStatusLabel method

19a2feb

adamw merged commit 212a79c into softwaremill:master Jan 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds method and optional status Labels to PrometheusBackend #1651

Adds method and optional status Labels to PrometheusBackend #1651

kevinmeredith commented Dec 4, 2022 •

edited

Loading

adamw Dec 7, 2022

kevinmeredith Dec 8, 2022

kevinmeredith Jan 8, 2023

adamw Dec 7, 2022

kevinmeredith Dec 8, 2022

kevinmeredith Jan 8, 2023

adamw Dec 7, 2022

kevinmeredith Dec 8, 2022

kevinmeredith Jan 8, 2023

adamw Dec 7, 2022

kevinmeredith Dec 8, 2022

kevinmeredith Jan 8, 2023

adamw commented Dec 7, 2022

kevinmeredith commented Jan 8, 2023

kevinmeredith commented Jan 8, 2023

adamw commented Jan 9, 2023

Adds method and optional status Labels to PrometheusBackend #1651

Adds method and optional status Labels to PrometheusBackend #1651

Conversation

kevinmeredith commented Dec 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamw commented Dec 7, 2022

kevinmeredith commented Jan 8, 2023

kevinmeredith commented Jan 8, 2023

adamw commented Jan 9, 2023

kevinmeredith commented Dec 4, 2022 •

edited

Loading