[CSL-1585] Namespace cardano EKG metrics #1464
Conversation
|
@adinapoli-iohk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @neongreen, @pva701 and @gromakovsky to be potential reviewers. |
This commit: * Switch to use a transient non-master revision of timewarp-nt which removes the `Node.Util.Monitor` module; * Adds the `Pos.Util.Monitor` module as part of Cardano; * Gives better names to the metrics in `setupMonitor` together with units of measure; * Drops `ekg` in favour of `ekg-wai`, to avoid transitive deps on snap-server and io-streams.
|
Note This is currently using a non-master, non-upstream revision of Now we have namespaced metrics for everything IOHK related, with units of measure:
|
|
I suggest we use ASCII just to be sure - it's quicker than verifying this :) |
Haha, I was sure you guys were going to prefer the safe route 😁 . I think we are fine though. StatsD spec explicitly says tags can be UTF-8: https://github.com/b/statsd_spec#metric-types--formats Th DD agent should support utf-8 as well: But if you guys don't feel confident anyway I'm happy to go with ASCII 😀 |
|
We can either take the risk of figuring out only on next deploy or spend a few minutes taking the safe route :) Both are fine with me though - my experience with python shows that few programmers write code that works well with non-ASCII encodings. |
I won't be the guy responsible for degrading the velocity of the project 😀 Safe route will be! Fix incoming. |
The TW PR is merged, you can update. |
core/Pos/System/Metrics/Constants.hs
Outdated
| {-# LANGUAGE OverloadedStrings #-} | ||
| module Pos.System.Metrics.Constants ( | ||
| withCardanoNamespace | ||
| ) where |
There was a problem hiding this comment.
Is this spacing in our style guide? :-)
core/Pos/System/Metrics/Constants.hs
Outdated
| @@ -0,0 +1,13 @@ | |||
| {-# LANGUAGE OverloadedStrings #-} | |||
There was a problem hiding this comment.
This extension (among many others) is enabled by default.
There was a problem hiding this comment.
Ah, I must have been taken in by flycheck or something 😉
node/src/Pos/Launcher/Runner.hs
Outdated
| ekgStore' <- setupMonitor | ||
| (runProduction . runToProd JsonLogDisabled oq) node' nrEkgStore | ||
| let ekgStore' = nrEkgStore | ||
| registerMetrics (Just cardanoNamespace) (runProduction . runToProd JsonLogDisabled oq) node' ekgStore' |
There was a problem hiding this comment.
You don't need ekgStore' any more. It was only there because the old setupMonitor returned a possibly modified store, and no it doesn't, so there's no need to name the result.
infra/Pos/Network/Types.hs
Outdated
| {-# LANGUAGE ExistentialQuantification #-} | ||
| {-# LANGUAGE CPP #-} | ||
| {-# LANGUAGE OverloadedStrings #-} |
There was a problem hiding this comment.
In infra (and I guess in all other packages) this extension is enabled by default too.
node/src/Pos/Util/Monitor.hs
Outdated
| @@ -0,0 +1,47 @@ | |||
| {-# OPTIONS_GHC -fno-warn-name-shadowing #-} | |||
There was a problem hiding this comment.
I find it much better to avoid name shadowing than to suppress warnings. If there are fundamental reasons why it can't be avoided, it should be justified in comments.
node/src/Pos/Util/Monitor.hs
Outdated
| import Control.Monad.IO.Class | ||
| import Mockable.Class | ||
| import qualified Mockable.Metrics as Metrics | ||
| import Node |
There was a problem hiding this comment.
Imports should be qualified or contain explicit import list, according to style-guide.
adinapoli-iohk
changed the title
adinapoli-iohk
deleted the
alfredo/csl-1585-correctly-namespace-ekg-metrics
branch
This PR correctly namespace IOHK's metrics by the "cardano" namespace, for which I have added a smart "constructor" inside a new module called
Pos.System.Metrics.Constantswithincore.Note this PR is not complete as we need to modify
timewarp-ntso thatregisterQueueMetricscan take an optional namespace (will do this in parallel):https://github.com/serokell/time-warp-nt/blob/40f4235cda65c746c6edc2a3455242d96d7175bf/src/Network/Broadcast/OutboundQueue.hs#L607
Last but not least, now the time-based queues are parametrised by their unit of measures, microseconds in
txpcase. (maybe one of the devops like @deepfire or @domenkozar could tell us ifstatsdwill choke if non-ASCII chars are passed to it). This is the result so far: