Add GaugeFloat64 API #151
Add GaugeFloat64 API #151
Conversation
|
Fixes hashicorp/vault#19594 |
sink.go
Outdated
| SetGaugeFloat64(key []string, val float64) | ||
| SetGaugeFloat64WithLabels(key []string, val float64, labels []Label) | ||
|
|
There was a problem hiding this comment.
Adding methods to an existing public interface is technically a breaking change which would require a whole new major version of the library with new imports etc.
That's because any external implementations of this interface will no longer compile after an upgrade until they also implement these new methods.
I think there are two options here:
- Make this a v2 change which for go mod requires moving all the code to a new /v2 subdir with associated go.mod changes etc.
- Add these to a different optional interface like
Float64Sinkor something (I know, names are hard...) then implement that for any internal sinks you care about. In the place where we call this method on a sink, type check it to see if it implements this interface first and if not do something. The choice on what to do here is somewhat nuanced. The signatures have no error returns for now because metrics are meant to be "fire and forget" and shouldn't hold up application processing, so the choice is eitherpanicand document that it is a developer error to allow a sink that doesn't support this new extended interface to be configured in an app that relies on the new methods you added... or you could just auto-downgrade to float32 which at least results in some metric though if the application bothered to use the 64 bit variant it's still likely to be the wrong thing, or you could just drop the gauge on the floor and maybe log it (though that might require plumbing a logger into the library) and if you do choose this path you probably would want to consider rate-limiting the logs because SetGauge could be called really often in an application and would fill logs really fast if every call logs a warning.
What do you think about those options?
There was a problem hiding this comment.
I prefer option 2. Do you have any preferences?
There was a problem hiding this comment.
2 seems more pragmatic to me. It's a little more "messy" in that ideally we'd have a single nice interface but IMO this isn't a big enough issue to warrant a v2 with all that entails.
The implicit option 3 would be to ask if this is even worth solving at all and choose to do nothing and live with the limitation of float32 for now. It's always worth asking the question! I can't say how important that is for vault though or what the implications of not fixing this might be so assuming it's worth it, I'd go for 2.
biazmoreira
force-pushed
the
biazmoreira/addgaugefloat64
branch
from
02f89f4 to
22d2d08
Compare
There was a problem hiding this comment.
Amazing work Bianca!
I've approved because code-wise this all looks good to me.
I left a few suggestions inline mostly around commenting the new behaviour in doc comments to make the library usage and the footguns more obvious for future consumers or code readers! This library is not well commented in general so it's a bit of a drop in the ocean, but especially the new functions that may panic etc. feel like they should be clearly documented to me!
| @@ -24,6 +24,11 @@ type MetricSink interface { | |||
| AddSampleWithLabels(key []string, val float32, labels []Label) | |||
| } | |||
|
|
|||
| type PrecisionGaugeMetricSink interface { | |||
There was a problem hiding this comment.
I realise the rest of this library does a less-than-ideal job of documenting interfaces already, but I'd be inclined to add a doc comment to explain the purpose of this additional interface. Could be a one liner, could refer to the history of not originally supporting 64bit floats, could even link to this PR for more info but it's probably useful context to a future reader who wonders why this is different to MetricSink!
I really like the name, but without any comment I might assume "precision gauge" is a specific concept/semantic I wasn't aware of and wonder where to look for more info. Just a simple line or two to the effect that not all sinks may support it but if 64 bit precisions is needed this allows that would save me time looking up commit history etc!
metrics.go
Outdated
| } | ||
| sink, ok := m.sink.(PrecisionGaugeMetricSink) | ||
| if !ok { | ||
| panic("Sink does not implement Float64MetricSink.") |
There was a problem hiding this comment.
| panic("Sink does not implement Float64MetricSink.") | |
| panic("Sink does not implement PrecisionGaugeMetricsSink.") |
sink.go
Outdated
| if s64, ok := s.(PrecisionGaugeMetricSink); ok { | ||
| s64.SetPrecisionGaugeWithLabels(key, val, labels) | ||
| } else { | ||
| s.SetGaugeWithLabels(key, float32(val), labels) |
There was a problem hiding this comment.
We chose to panic in the general case of a sink not supporting rather than silently downsample. This is more subtle since there could be valid sinks we can deliver to but I wonder if we should at least document these methods to state that downsampling will occur if an underlying sink doesn't support precision gauges?
There was a problem hiding this comment.
We chose to panic in the general case of a sink not supporting rather than silently downsample.
This wasn't a thought-out decision. Do you think it's worth downsampling the values instead of panicking in case the type check is unsuccessful?
There was a problem hiding this comment.
Looking at the code more closely, I'm leaning towards downsampling. Curious about what you think, though!
There was a problem hiding this comment.
Yeah I liked the panic option for straightforward case because I think that makes it harder to accidentally use the library incorrectly if your sink doesn't support the feature.
In this case I'd maybe lean towards doing neither panic or downsample and instead just not call anything on an unsupported sink for a precision gauge. Possibly wrong data seems more dangerous or likely to cause problems than missing data - if the fact it's missing is noticed then it might prompt some research into why whereas it just being wrong is much less likely to be spotted and could lead to incorrect analysis downstream by humans or automation.
Whatever option we should add a doc comment to call it out as a footgun the developer is responsible for dealing with.
start.go
Outdated
| func SetPrecisionGauge(key []string, val float64) { | ||
| globalMetrics.Load().(*Metrics).SetPrecisionGauge(key, val) | ||
| } | ||
|
|
||
| func SetPrecisionGaugeWithLabels(key []string, val float64, labels []Label) { | ||
| globalMetrics.Load().(*Metrics).SetPrecisionGaugeWithLabels(key, val, labels) | ||
| } | ||
|
|
There was a problem hiding this comment.
This file in general is not well commented but I would lean towards adding doc comments for these functions. Partly because they form part of the libraries core API but specifically because they might panic if used with an incompatible sink and I think callers should be very clear about that before using them!
biazmoreira
force-pushed
the
biazmoreira/addgaugefloat64
branch
from
22d2d08 to
eb30b88
Compare
This adds a new API to send and store gauge metrics as float64s. Previously, they were being stored as float32s which isn't enough precision for storing unix timestamps.
Related issue: hashicorp/vault#19594 and #150