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

Fix bug with observeAndSample function from Histogram #51

Merged
merged 3 commits into from
Dec 19, 2023
Merged

Conversation

cdepillabout
Copy link
Contributor

@cdepillabout cdepillabout commented Dec 19, 2023

The Histogram metric has a function observeAndSample, which lets you add a new observation, and sample it at the same time. Before this commit, this observeAndSample function would return the previous sample, not the sample you just added.

This commit changes the behavior of observeAndSample so that it returns the sample that was just added. This matches the behavior of addAndSample (from Counter), and modifyAndSample (from Gauge), which both return the value of the sample that was just added.

Here's an example program that demonstrates this problem:

{-# LANGUAGE OverloadedStrings #-}

module Main where

import Control.Monad.IO.Class (liftIO)
import System.Metrics.Prometheus.Concurrent.RegistryT
import System.Metrics.Prometheus.Metric.Counter (addAndSample)
import System.Metrics.Prometheus.Metric.Histogram (observeAndSample)
import System.Metrics.Prometheus.MetricId


main :: IO ()
main = runRegistryT $ do
    exampleCounter <- registerCounter "example_counter" mempty
    exampleHist <- registerHistogram "example_hist" mempty [10, 20 .. 100]

    liftIO $ do
      counterResult <- addAndSample 29 exampleCounter
      histResult <- observeAndSample 59 exampleHist
      print counterResult
      print histResult

Running this on master gives output like the following:

CounterSample {unCounterSample = 29}
HistogramSample {histBuckets = fromList [(10.0,0.0),(20.0,0.0),(30.0,0.0),(40.0,0.0),(50.0,0.0),(60.0,0.0),(70.0,0.0),(80.0,0.0),(90.0,0.0),(100.0,0.0),(Infinity,0.0)], histSum = 0.0, histCount = 0}

You can see that while the result of addAndSample for the Counter returns 29 (which was the value just added), the result of observeAndSample for the Histogram returns all zeros (which is the initial state of a histogram).

Running this same program in this PR results in:

CounterSample {unCounterSample = 29}
HistogramSample {histBuckets = fromList [(10.0,0.0),(20.0,0.0),(30.0,0.0),(40.0,0.0),(50.0,0.0),(60.0,1.0),(70.0,1.0),(80.0,1.0),(90.0,1.0),(100.0,1.0),(Infinity,1.0)], histSum = 59.0, histCount = 1}

Now you can see that the resulting Histogram sample correctly has a single observation.

This commit derives `Show` for all of our *Sample types:

- `CounterSample`
- `GaugeSample`
- `HistogramSample`
- `SummarySample`
The Histogram metric has a function `observeAndSample`, which lets you
add a new observation, and sample it at the same time.  Before this
commit, this `observeAndSample` function would return the _previous_
sample, _not_ the sample you just added.

This commit changes the behavior of `observeAndSample` so that it
returns the sample that was just added.  This matches the behavior
of `addAndSample` (from `Counter`), and `modifyAndSample` (from
`Gauge`), which both return the value of the sample that was just
added.
@cdepillabout cdepillabout requested a review from wraithm December 19, 2023 08:23
@cdepillabout cdepillabout marked this pull request as ready for review December 19, 2023 08:23
newtype CounterSample = CounterSample {unCounterSample :: Int}
newtype CounterSample = CounterSample {unCounterSample :: Int} deriving Show
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR also adds Show for all of our *Sample types.

I'm assuming that we want this for ease of debugging. The only problem I can see with this is if we ever want to add a sample type with a value that isn't showable, we might have to figure out what to do in that case. I imagine the likelihood of that is pretty low.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might we want Eq as well? Maybe even Ord. Even though we don't use those typeclasses they seem appropriate for what the data appears to represent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have a strong opinion on this one.

My personal opinion is to derive Show for everything, since it is helpful for debugging. But only derive other typeclasses as-needed, since it can add additional compile time: https://taylor.fausak.me/2017/08/09/deriving-type-classes-in-haskell-is-slow/

But if we have some sort of coding rule here for what to do, I'd be happy to follow it.

@wraithm wraithm changed the base branch from add-ci to master December 19, 2023 15:37
@wraithm wraithm merged commit 6ccbc9d into master Dec 19, 2023
10 checks passed
@cdepillabout cdepillabout deleted the histogram-bug branch December 19, 2023 22:24
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 this pull request may close these issues.

3 participants