-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
NaN metric is not reported #65
Comments
Hi @tomkerkhove Thank you for reporting the problem, but could you please provide some more details about you use-case? We suppress NaN value in Gauge for the reason: it's hard to define predictable behavior for the Gauge in such scenario. What if value was set to NaN and then try to Inc? Should we consider NaN as 0 and result to current value be 1? But NaN + 1 results to NaN in .net. Or should we work with NaN according to .net rules? Than gauge will "not accept" any changes unless user's code explicitly set it to some other value. We should define behavior before make any changes to the library. Let's discuss your use-case and create some proposal. Thanks |
If you are doing this deliberately I think an exception would'be been better rather than supressing as this is a bit strange. My use case is simple - I have a metric and sometimes I need to report |
Ok, so here is my suggestion:
So you can use untyped instead of gauge for your needs. I can create PR tonight if the suggestion works for you. |
Well, in theory, is it still a gauge 🤔 Can we have 2 gauges? Or be able to define a baseline for NaN so you can inc/dec? |
Where I'm going is that there should maybe be 2 gauges - One which only has set and supports NaN, and one which has Inc/Dec? |
What if the Example: var nanGauge = Prometheus.Client.Metrics.CreateGauge("nan", "metric with nan value", suppressNaN: false);
nanGauge.Set(double.NaN); |
It's not a gauge, but untyped which is exported as untyped metric. The main difference that there is Set only, no Inc or Dec methods. |
In our case, we need a gauge - https://prometheus.io/docs/concepts/metric_types/#gauge. Based on this our scenario is still valid as far as I see: https://prometheus.io/docs/practices/instrumentation/#avoid-missing-metrics
Gauges can be set, go up, and go down. They are useful for snapshots of state, such as in-progress requests, free/total memory, or temperature. You should never take a rate() of a gauge. |
I think this is the best approach, personally. It tracks as a Gauge & allows me to use NaN when I don't have a decent metric. |
The problem with NaN for gauge that if you set it's value to NaN, and then use Inc... the current value will be NaN... it might be unexpected and hard to predict behavior. Why can't you use untyped instead? Currently Prometheus server ignores type information at all:
Untyped metric has no such logical problem in work with NaN as Gauge has, because there are no methods except Set. |
Or after all you can implement and register into the CollectorsRegistry you own metric, which will work as you want and reports itself as a Gauge (It's easy after the V3 release by using IMetricWriter). I can create an example for you if you prefer this way. |
The reason for a gauge is that human consumers know what to expect but I get your point, although I'm still not convinced (sorry 😅) Feel free to post a sample, maybe that's a good alternative! |
Ok, so do you need labels support for the metric or just a single unlabeled value? |
We are using labels indeed! But take your time, it's not urgent. |
Hi @tomkerkhove ! Here is example code for custom metric which uses stub as a datasource. All you are need - just register instance of RaceMetric class in your collectors registry (Metrics.DefaultCollectorRegistry if you are using the default one). Custom collector should implement ICollector interface, where:
Please let me know if this helps. |
@tomkerkhove Could you please let us know if the suggestion above helps? Thanks |
Thanks for the sample, I'll have a look at soon. |
Hi @tomkerkhove Is this still actual or we can close the issue? Thanks |
I still need to give it a spin but will close for now. Thanks! |
I've had a look at the sample and makes it a lot more complex for consumers, just to track a NaN on a Gauge. How would it work if I want to have the same experience as the following, but for a custom metric: var gauge = Metrics.CreateGauge("gauge", "help text");
gauge.Set(5.3); I still believe that, if it's opt-in, and I call Inc/Dec on a NaN it's up to me to fix it or just use Set. I tried to extend the current gauge but the factory method makes it a bit more cumbersome. However, if you really think it's best not to remove the check then that's ok for me. Will go with an untyped metric then, unfortunately. |
I'll reopen the issue so we can discuss the changes. Here is new suggestion: @tomkerkhove and @phnx47 does such suggestion works/makes sense? Thanks |
That would be prefect for me! Thanks for considering! |
We are holding off https://github.com/tomkerkhove/promitor v1.0 until this is sorted out. Feel free to let me know what you think of this and if you have the bandwidth. If not, I might be able to contribute this if you want. |
Wow! Sounds like we have to change our plans a little to make this live ASAP. I think it will take day or two to get development build so you can check everything on your side. And then we can cut the production build. I'll keep you informed on the progress. |
Thanks and don't rush it, if we have to wait then that's it - I don't expect you to drop everything and implement this :)
In terms of your suggestion, don't feel obliged to roll this out for everybody perse, if you want to use a |
Pulled in the MyGet package and it works:
Thank you! |
@tomkerkhove, sounds good! Do we need to create a prod package ASAP or you OK for now and it can wait week or two (for other issues in scope of 3.1 to be sorted)? |
I won't ship v1.0 on the preview package but it's ok to wait a week or two, no worries! |
OK. I'll close the issue and let's plan it to be included into upcoming 3.1 release. If you need the functionality to be available earlier - just let us know and we will cut the build. Thanks. |
Just out of curiosity - When are you planning to ship 3.1? |
Hi @tomkerkhove ! I've just finished working on performance optimization. Now validating the changes and testing the library. I think we will be ready to release early next week or so. Is it OK with your release or should we rush the release? |
Sure that's fine - Thanks! |
Not A Number (NaN) is a supported metric according to EXPOSITION FORMATS
However, this is not tracked when doing the following:
Expected Outcome
Actual Outcome
Repro
Pull https://github.com/tomkerkhove/prometheus-nan-repro and consume
/repro
endpoint.The text was updated successfully, but these errors were encountered: