Dimensional counters #32
Conversation
Breaking change: the Sample type now holds dimensions as key-value pairs.
m-matth
force-pushed
the
dimensional-counters2
branch
from
1a16acd to
3ab47b6
Compare
m-matth
force-pushed
the
dimensional-counters2
branch
from
3ab47b6 to
1a77ab4
Compare
System/Metrics.hs
Outdated
| sampleOne (GaugeS m) = Right . Gauge <$> m | ||
| sampleOne (LabelS m) = Right . Label <$> m | ||
| sampleOne (DistributionS m) = Right . Distribution <$> m | ||
| sampleOne (DimensionalS dims m) = Left . (\pairs -> (dims, pairs)) . M.toList <$> m |
There was a problem hiding this comment.
You might want to use a strict <$> to force the result to avoid creating thunks. It's quite important that sampling is efficient otherwise we might be slowing down the application with a background task such as monitoring.
System/Metrics.hs
Outdated
| readAllRefs m = do | ||
| forM ([(name, ref) | (name, Left ref) <- M.toList m]) $ \ (name, ref) -> do | ||
| val <- sampleOne ref | ||
| return (name, val) | ||
| return $ case val of |
|
|
||
| type Name = Text | ||
|
|
||
| type Explanation = Text |
There was a problem hiding this comment.
This is a bit more of an overall comment but I think all metrics should support an explanation (although I haven't given thought to how to retrofit this into the current API) and each dimension could also use an explanation.
In general I think one might want to attach the following metadata to a metric:
- It's type e.g. "seconds" or "megabytes". This helps when automatically creating graphs and UIs for metrics.
- A human-readable explanation of the metrics.
- An explanation of each dimension.
System/Metrics/Dimensional.hs
Outdated
| | not $ matchDimensions (dimensionalDimensions d) pt = | ||
| pure $ Left (UnmatchedDimensions err) | ||
| | otherwise = do | ||
| toLookupResult . HashMap.lookup pt <$> readIORef (dimensionalPoints d) |
There was a problem hiding this comment.
Probably want to force the result.
|
|
||
| type Explanation = Text | ||
|
|
||
| type Dimensions = [Text] |
There was a problem hiding this comment.
It might be convenient to allow a dimension to be a member of some Dimension type class with instances for Text, String, Int, and so forth. Will make the API nicer to use without having to use "to text" conversions at each call site for dimensions that are e.g. naturally ints (e.g. like HTTP status codes). The type class can be used just in the user-facing API. Internally it can all be Text.
PR based on #22, with review taken into account.