-
Notifications
You must be signed in to change notification settings - Fork 71
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
Prometheus client metrics support #711
Conversation
@jjaakola-aiven @aiven-anton could You review this PR too? |
@aiven-anton done. Review please. |
BTW. Is the usage of .gauge() and .timing() in the following functions acceptable?
|
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.
Can we have some tests?
We can certainly add some tests. However, the current version of Karapace Stats lacks test coverage, so we have no existing framework to guide us. At this point, it seems that only unit tests are feasible. I'm unsure how to implement integration tests for this part task. |
Let's kick off with the unit tests. I'll provide an example soon on how to execute an integration test. It's crucial to have a tangible test that ensures our output its compliant to the standard format. Without integration tests, even if the feature is technically sound, we lack a mechanism preventing us from the introduction of code that might break the consumers |
karapace/metrics.py
Outdated
|
||
|
||
class Metrics(metaclass=Singleton): | ||
stats_client: StatsClient |
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.
stats_client: StatsClient | None = None
, I've run this locally with metrics_extended
set to False
and its failing because the stats_client
its not going to be set
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.
Fixed and unit test for this case is added.
Thoughts:
I will continue the work by @eliax1996 and try to add a sample metric and also some integration test. |
This PR introduces the Metrics class, aiming to implement metrics similar to those found in comparable products. We intend to support both StatsD and Prometheus. Are you suggesting that we should enable the concurrent use of both? |
I do not think it is necessary to have both StatsD and Prometheus within the application itself, especially if there are tools to integrate one on top of the other. Nevertheless, we already provide statsd support within the code, this is something to discuss internally, we'd go ahead and add prometheus metrics and then reason around what to do with the current stats implementation which both calls statsd and sentry at the same time. |
@libretto I will close this PR once #902 is merged. |
This code introduces the Metrics class, which allows clients migrating to Karapace from alternative products to collect metrics similar to those they are accustomed to. It supports using Prometheus as an alternative to StatsD for collecting statistics in Karapace.