-
Notifications
You must be signed in to change notification settings - Fork 302
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
Updated Prometheus to 1.2.1 #2145
Conversation
summaries.remove(collectorRegistry) | ||
def clear(prometheusRegistry: PrometheusRegistry): Unit = { | ||
unregister(prometheusRegistry, histograms) | ||
histograms.remove(prometheusRegistry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the .remove could be part of the unregister maybe?
cache: mutable.WeakHashMap[CollectorRegistry, ConcurrentHashMap[String, T]], | ||
collectorRegistry: CollectorRegistry | ||
cache: mutable.WeakHashMap[PrometheusRegistry, ConcurrentHashMap[String, T]], | ||
prometheusRegistry: PrometheusRegistry | ||
): ConcurrentHashMap[String, T] = | ||
cache.synchronized { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a follow-up task, we should remove the .synchronized
, as it shouldn't be used in Java21+, and repalce it with some kind of a concurrent data structure (or improve the cache in general)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe we can do it right away, this is invoked only during the backend's construction, so a simple lock should suffice in fact; but we should also protect the cleanup method, then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about separate PR focused on cache I would prefer not to mix it with this update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok :)
@@ -1,8 +1,11 @@ | |||
package sttp.client4.prometheus | |||
|
|||
import io.prometheus.metrics.core.datapoints.{GaugeDataPoint, Timer} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another follow-up task: in tapir, we are using some standardized labels for the http metrics; I suspect there might be some standards for opentelemetry / prometheus as well. If so, we should probably use these by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should get backported to the v3 branch after it's merged, right @adamw?
observability/prometheus-backend/src/main/scala/sttp/client4/prometheus/PrometheusBackend.scala
Show resolved
Hide resolved
...bility/prometheus-backend/src/test/scala/sttp/client4/prometheus/PrometheusBackendTest.scala
Show resolved
Hide resolved
@kciesielski I don't know, it's a breaking change, although in an integration module ... let's see if somebody will need it ;) |
closes #2011