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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

## 2.3.0

* Change the `observeAndSample` function from the
`System.Metrics.Prometheus.Metric.Histogram` module to return the value of
the sample that was just added, instead of the previous sample.
This change matches similar functions for `Counter`s and `Gauge`s.
[#51](https://github.com/bitnomial/prometheus/pull/51)
4 changes: 2 additions & 2 deletions prometheus.cabal
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
name: prometheus
version: 2.2.4
version: 2.3.0
synopsis: Prometheus Haskell Client
homepage: http://github.com/bitnomial/prometheus
bug-reports: http://github.com/bitnomial/prometheus/issues
license: BSD3
license-file: LICENSE
author: Luke Hoersten
maintainer: luke@bitnomial.com, opensource@bitnomial.com
copyright: Bitnomial, Inc. (c) 2016-2019
copyright: Bitnomial, Inc. (c) 2016-2023
category: Metrics, Monitoring, Web, System
build-type: Simple
cabal-version: >=1.10
Expand Down
2 changes: 1 addition & 1 deletion src/System/Metrics/Prometheus/Metric/Counter.hs
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@
set,
) where

import Control.Applicative ((<$>))

Check warning on line 12 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.0.2 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 12 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.4.8 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 12 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.6.3 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 12 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-8.10.7 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 12 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.2.8 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 12 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.2.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 12 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.0.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 12 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.6.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 12 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-8.10.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 12 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.4.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant
import Control.Monad (when)

Check warning on line 13 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.0.2 / ubuntu-latest

The import of ‘Control.Monad’ is redundant

Check warning on line 13 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.4.8 / ubuntu-latest

The import of ‘Control.Monad’ is redundant

Check warning on line 13 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.6.3 / ubuntu-latest

The import of ‘Control.Monad’ is redundant

Check warning on line 13 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-8.10.7 / ubuntu-latest

The import of ‘Control.Monad’ is redundant

Check warning on line 13 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.2.8 / ubuntu-latest

The import of ‘Control.Monad’ is redundant

Check warning on line 13 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.2.yaml / ubuntu-latest

The import of ‘Control.Monad’ is redundant

Check warning on line 13 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.0.yaml / ubuntu-latest

The import of ‘Control.Monad’ is redundant

Check warning on line 13 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.6.yaml / ubuntu-latest

The import of ‘Control.Monad’ is redundant

Check warning on line 13 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-8.10.yaml / ubuntu-latest

The import of ‘Control.Monad’ is redundant

Check warning on line 13 in src/System/Metrics/Prometheus/Metric/Counter.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.4.yaml / ubuntu-latest

The import of ‘Control.Monad’ is redundant
import Data.Atomics.Counter (AtomicCounter, incrCounter, newCounter, writeCounter)


newtype Counter = Counter {unCounter :: AtomicCounter}
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.



new :: IO Counter
Expand Down
2 changes: 1 addition & 1 deletion src/System/Metrics/Prometheus/Metric/Gauge.hs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@
modifyAndSample,
) where

import Control.Applicative ((<$>))

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Gauge.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.0.2 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Gauge.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.4.8 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Gauge.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.6.3 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Gauge.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-8.10.7 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Gauge.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.2.8 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Gauge.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.2.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Gauge.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.0.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Gauge.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.6.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Gauge.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-8.10.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Gauge.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.4.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant
import Data.IORef (IORef, atomicModifyIORef', newIORef)


newtype Gauge = Gauge {unGauge :: IORef Double}
newtype GaugeSample = GaugeSample {unGaugeSample :: Double}
newtype GaugeSample = GaugeSample {unGaugeSample :: Double} deriving Show


new :: IO Gauge
Expand Down
3 changes: 2 additions & 1 deletion src/System/Metrics/Prometheus/Metric/Histogram.hs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
observeAndSample,
) where

import Control.Applicative ((<$>))

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Histogram.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.0.2 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Histogram.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.4.8 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Histogram.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.6.3 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Histogram.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-8.10.7 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Histogram.hs

View workflow job for this annotation

GitHub Actions / cabal / ghc-9.2.8 / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Histogram.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.2.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Histogram.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.0.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Histogram.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.6.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Histogram.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-8.10.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant

Check warning on line 14 in src/System/Metrics/Prometheus/Metric/Histogram.hs

View workflow job for this annotation

GitHub Actions / stack --stack-yaml ./stack-9.4.yaml / ubuntu-latest

The import of ‘Control.Applicative’ is redundant
import Control.Monad (void)
import Data.Bool (bool)
import Data.IORef (
Expand All @@ -36,6 +36,7 @@
, histSum :: !Double
, histCount :: !Int
}
deriving Show


new :: [UpperBound] -> IO Histogram
Expand All @@ -49,7 +50,7 @@
observeAndSample :: Double -> Histogram -> IO HistogramSample
observeAndSample x = flip atomicModifyIORef' update . unHistogram
where
update histData = (hist' histData, histData)
update histData = (hist' histData, hist' histData)
hist' histData =
histData
{ histBuckets = updateBuckets x $ histBuckets histData
Expand Down
1 change: 1 addition & 0 deletions src/System/Metrics/Prometheus/Metric/Summary.hs
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ data SummarySample = SummarySample
, sumSum :: !Int
, sumCount :: !Int
}
deriving Show
Loading