-
Notifications
You must be signed in to change notification settings - Fork 297
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-net is an unnecessary dependency #1525
Comments
Related to #898 |
It actually looks like this is not used by default, but an optional piece of functionality that needs to be opted in to. But it still always has the Now that HttpClient has metrics by default that seem to cover what PrometheusHandler measures, can PrometheusHandler be removed as well as the |
i am good with removing it and use sys default instead, this would make sdk clean and safe as it is widely used. |
I like moving this out into a separate package. That way if people have dependencies they can still use it, but the main package will be cleaner. |
@tg123 @brendandburns |
@rwkarg can you also add a new nuget package for the promtheus handler so that people who may have used it don't get broken? |
Thinking through the different scenarios to see what the potential impact would be.
To address that last case, we could:
The first option is the least work in the project but does require work from some users. I don't believe there's a good reason to pick option 3 over the other two options. |
Describe the bug
This package references prometheus-net for metrics but that is not a core function of this package.
This can lead to problems where a user has an older version of Prometheus server running on a version that can't accept metrics from the version of prometheus-net reference by this package. This has happened with an older (but not too old) version of Promethus and prometheus-net referenced in KubernetesClient 8.x. The new prometheus-net version includes exemplars which leads to the Prometheus server not accepting the published metrics. Users can't force using an older version of prometheus-net explicitly because it is binary incompatible with the version KubernetesClient was built with and will lead to runtime exceptions and crashing.
Ideally, there would have been a separate package for KubernetesClient.Prometheus that could optionally pulled in to get metrics.
Now however, there are .NET Metrics available without needing to pull in any additional dependency (other than the System package System.Diagnostics.DiagnosticSource) and then users of KubernetesClient are free to use anything that integrates with that common interface (
dotnet-counters
, OpenTelemetry w/ Prometheus exporter, OpenTelemetry w/ OTLP exporter, directly with a MeterListener). This allows for a no-dependency way to collect metrics and a common interface to achieve the existing behavior (Prometheus exposition) or a variety of other exports that the users' applications are already using. This also means that the user is in control of the version of any additional, non-core dependencies like exporting metric data. Additionally, users that do not have a framework to store metrics would not need to pull in any unnecessary packages when all they want to do is work with a Kubernetes cluster.Example impact:
Kubernetes C# SDK Client Version
e.g.
8.x
To Reproduce
Run a Prometheus 2.42.0 server
https://github.com/prometheus/prometheus/releases/tag/v2.42.0
Run a test app using KubernetesClient 8.x+ with any exposed metrics
Scrape metrics from the test app
Expected behavior
Prometheus metrics should be stored by the Prometheus server
Actual behavior
No metrics are stored by the Prometheus server because that version of Prometheus server does not accept metrics that includes exemplars, but the version of prometheus-net that is required to be used by KubernetesClient 8.x+ includes exemplars in the data it exposes.
The text was updated successfully, but these errors were encountered: