Skip to content
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

Closed
rwkarg opened this issue Feb 17, 2024 · 8 comments · Fixed by #1526
Closed

Prometheus-net is an unnecessary dependency #1525

rwkarg opened this issue Feb 17, 2024 · 8 comments · Fixed by #1526

Comments

@rwkarg
Copy link
Contributor

rwkarg commented Feb 17, 2024

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.

@rwkarg
Copy link
Contributor Author

rwkarg commented Feb 17, 2024

Related to #898

@rwkarg
Copy link
Contributor Author

rwkarg commented Feb 17, 2024

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 prometheus-net reference.
Add a Prometheus handler. #591

Now that HttpClient has metrics by default that seem to cover what PrometheusHandler measures, can PrometheusHandler be removed as well as the prometheus-net package reference?
https://learn.microsoft.com/en-us/dotnet/core/diagnostics/built-in-metrics-system-net

@tg123
Copy link
Member

tg123 commented Feb 17, 2024

i am good with removing it and use sys default instead, this would make sdk clean and safe as it is widely used.
could you please send a PR and update README also

cc @brendanburns

@brendandburns
Copy link
Contributor

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.

@rwkarg
Copy link
Contributor Author

rwkarg commented Feb 26, 2024

@tg123 @brendandburns
Does PR #1526 adequately address this issue?

@brendandburns
Copy link
Contributor

@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?

@rwkarg
Copy link
Contributor Author

rwkarg commented Feb 27, 2024

@brendandburns

Thinking through the different scenarios to see what the potential impact would be.

  • Without explicitly configuring the PrometheusHandler, there will be no difference after the change.
  • Configuring the PrometheusHandler without also configuring a MetricServer or some other way to consume the metrics wouldn't have any observable difference
  • Configuring the PrometheusHandler with a MetricServer would already be getting the new .NET metrics (the version of prometheus-net referenced now pulls in the .NET metrics). Here, the difference would be that the old, now duplicate, metric names that PrometheusHandler recorded would not be there and any visualization/alerting would need to be updated to the naming of the built in .NET metrics.

To address that last case, we could:

  1. Provide migration guidance on how to update to the new metric names
  2. Re-implement PrometheusHandler in the existing project to publish the same named metrics but using the .NET metrics (removing the need to reference prometheus-net)
  3. Re-implement PrometheusHandler in a new project to exactly match the prior implementation and pull in the prometheus-net package just in that optional project

The first option is the least work in the project but does require work from some users.
Option two is probably the most reasonable approach as it maintains behavior, doesn't require publishing a separate package or a user to need to pull in a new package, and also removes the dependency on prometheus-net completely.

I don't believe there's a good reason to pick option 3 over the other two options.

@scout719
Copy link

@rwkarg I've noticed that the Handler was added back to keep backwards compatibility here: #1534

Is there any ETA to have a new release with this change?
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants