Skip to content

Backward compatibility for Prometheus metrics names #4955

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Mar 22, 2023
Merged

Backward compatibility for Prometheus metrics names #4955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5881736
cardano-tracer: With metrics compability mapping
jutaro Mar 1, 2023
78d4667
nix: Test configuration for metrics mapping
jutaro Mar 7, 2023
ffe6b4d
cardano-tracer: Fixes for tests
jutaro Mar 8, 2023
6864161
node: Tracer Fixes
jutaro Mar 15, 2023
3f08333
cardano-tracer: Fix typos, better comment
jutaro Mar 16, 2023
a30f5ca
cardano-tracer: Fix bench
jutaro Mar 16, 2023
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
4 changes: 2 additions & 2 deletions cardano-node/src/Cardano/Node/Tracing/Tracers/ChainDB.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ instance MetaTrace (ChainDB.TraceAddBlockEvent blk) where
, " of blocks that could be created over the span of the last @k@ blocks."
]
)
, ( "ChainDB.Slots"
, ( "ChainDB.SlotNum"
, "Number of slots in this chain fragment."
)
, ( "ChainDB.Blocks"
Expand All @@ -629,7 +629,7 @@ instance MetaTrace (ChainDB.TraceAddBlockEvent blk) where
, " of blocks that could be created over the span of the last @k@ blocks."
]
)
, ( "ChainDB.Slots"
, ( "ChainDB.SlotNum"
, "Number of slots in this chain fragment."
)
, ( "ChainDB.Blocks"
Expand Down
1 change: 1 addition & 0 deletions cardano-tracer/bench/cardano-tracer-bench.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ main = do
, logging = NE.fromList [LoggingParams root FileMode format]
, rotation = Nothing
, verbosity = Nothing
, metricsComp = Nothing
}

generate num = replicate num . mkTraceObject <$> getCurrentTime
Expand Down
3 changes: 3 additions & 0 deletions cardano-tracer/src/Cardano/Tracer/Configuration.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ import Data.List.NonEmpty (NonEmpty)
import qualified Data.List.NonEmpty as NE
import Data.List.Extra (notNull)
import Data.Maybe (catMaybes)
import Data.Map.Strict (Map)
import Data.Word (Word16, Word32, Word64)
import Data.Text (Text)
import Data.Yaml (decodeFileEither)
import GHC.Generics (Generic)
import System.Exit (die)
Expand Down Expand Up @@ -91,6 +93,7 @@ data TracerConfig = TracerConfig
, logging :: !(NonEmpty LoggingParams) -- ^ Logging parameters.
, rotation :: !(Maybe RotationParams) -- ^ Rotation parameters.
, verbosity :: !(Maybe Verbosity) -- ^ Verbosity of the tracer itself.
, metricsComp :: !(Maybe (Map Text Text)) -- ^ Metrics compatibility map from metrics name to metrics name
} deriving (Eq, Generic, FromJSON, ToJSON, Show)

-- | Read the tracer's configuration file.
Expand Down
24 changes: 20 additions & 4 deletions cardano-tracer/src/Cardano/Tracer/Handlers/Metrics/Prometheus.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,18 +103,19 @@ runPrometheusServer tracerEnv (Endpoint host port) = forever $ do
Just [nodeName] -> do
liftIO (askNodeId tracerEnv $ decodeUtf8 nodeName) >>= \case
Nothing -> writeText "No such a node!"
Just anId -> writeText =<< liftIO (getMetricsFromNode anId teAcceptedMetrics)
Just anId -> writeText =<< liftIO (getMetricsFromNode tracerEnv anId teAcceptedMetrics)
_ -> writeText "No such a node!"

type MetricName = Text
type MetricValue = Text
type MetricsList = [(MetricName, MetricValue)]

getMetricsFromNode
:: NodeId
:: TracerEnv
-> NodeId
-> AcceptedMetrics
-> IO Text
getMetricsFromNode nodeId acceptedMetrics =
getMetricsFromNode tracerEnv nodeId acceptedMetrics =
readTVarIO acceptedMetrics >>=
(\case
Nothing ->
Expand All @@ -124,7 +125,11 @@ getMetricsFromNode nodeId acceptedMetrics =
) . M.lookup nodeId
where
getListOfMetrics :: Sample -> MetricsList
getListOfMetrics = filter (not . T.null . fst) . map metricsWeNeed . HM.toList
getListOfMetrics =
metricsCompatibility
. filter (not . T.null . fst)
. map metricsWeNeed
. HM.toList

metricsWeNeed (mName, mValue) =
case mValue of
Expand All @@ -143,3 +148,14 @@ getMetricsFromNode nodeId acceptedMetrics =
. T.replace " " "_"
. T.replace "-" "_"
. T.replace "." "_"

metricsCompatibility :: MetricsList -> MetricsList
metricsCompatibility metricsList =
case metricsComp (teConfig tracerEnv) of
Nothing -> metricsList
Just mmap -> foldl (\ accu p'@(mn,mv) -> case M.lookup mn mmap of
Nothing -> p' : accu
Just rep -> p' : (rep,mv) : accu)
[]
metricsList
Copy link
Contributor

Choose a reason for hiding this comment

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

Has metricsList to be ordered somehow (e.g. identical mvs must be subsequent)?
I was wondering about the fold...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only one mapping per entry is supported, and then order doesn't matter.


1 change: 1 addition & 0 deletions cardano-tracer/test/Cardano/Tracer/Test/Acceptor.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ launchAcceptorsSimple mode localSock dpName = do
, logging = NE.fromList [LoggingParams "/tmp/demo-acceptor" FileMode ForHuman]
, rotation = Nothing
, verbosity = Just Minimum
, metricsComp = Nothing
}

-- | To be able to ask any 'DataPoint' by the name without knowing the actual type,
Expand Down
1 change: 1 addition & 0 deletions cardano-tracer/test/Cardano/Tracer/Test/DataPoint/Tests.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,5 @@ propDataPoint ts@TestSetup{..} rootDir localSock = do
, logging = NE.fromList [LoggingParams rootDir FileMode ForHuman]
, rotation = Nothing
, verbosity = Just Minimum
, metricsComp = Nothing
}
3 changes: 3 additions & 0 deletions cardano-tracer/test/Cardano/Tracer/Test/Logs/Tests.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ propLogs ts@TestSetup{..} format rootDir localSock = do
, rpKeepFilesNum = 10
}
, verbosity = Just Minimum
, metricsComp = Nothing
}

propMultiInit :: TestSetup Identity -> LogFormat -> FilePath -> FilePath -> FilePath -> IO Property
Expand Down Expand Up @@ -107,6 +108,7 @@ propMultiInit ts@TestSetup{..} format rootDir localSock1 localSock2 = do
, logging = NE.fromList [LoggingParams root FileMode format]
, rotation = Nothing
, verbosity = Just Minimum
, metricsComp = Nothing
}

propMultiResp :: TestSetup Identity -> LogFormat -> FilePath -> FilePath -> IO Property
Expand Down Expand Up @@ -134,6 +136,7 @@ propMultiResp ts@TestSetup{..} format rootDir localSock = do
, logging = NE.fromList [LoggingParams root FileMode format]
, rotation = Nothing
, verbosity = Just Minimum
, metricsComp = Nothing
}

checkMultiResults :: FilePath -> IO Property
Expand Down
1 change: 1 addition & 0 deletions cardano-tracer/test/Cardano/Tracer/Test/Restart/Tests.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,5 @@ mkConfig TestSetup{..} rootDir p = TracerConfig
, logging = NE.fromList [LoggingParams rootDir FileMode ForMachine]
, rotation = Nothing
, verbosity = Just Minimum
, metricsComp = Nothing
}
8 changes: 8 additions & 0 deletions nix/nixos/cardano-tracer-service.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ let serviceConfigToJSON =
epHost = "127.0.0.1";
epPort = 3200; ## supervisord.portShiftPrometheus
} // (cfg.prometheus or {});
# Just an example for metrics compatibility mapping.
# An entry means the first entry has the second entry as alias.
# The Metrics is then avalable, both with the original and the mapped name.
# Only one mapping per message is supported.
# metricsComp = {
# "Mempool.TxsInMempool" = "Mempool.TxsInMempool.Mapped";
# "ChainDB.SlotNum" = "ChainDB.SlotNum.Mapped";
Copy link
Contributor

Choose a reason for hiding this comment

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

@jutaro, just one thing -- can we change names in this example to make it more clear -- what metric already exists, and what will be created? If I didn't have the context, I'd be a bit confused by it.

Also, another question -- can we flip the order?
It's a bit weird that the created metrics are on the right of what reads like an assignment operator.

Copy link
Contributor Author

@jutaro jutaro Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What names to you think make it clearer. I thought adding the ".Mapped" postfix makes it clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a mapping from current name to new name. I think this is pretty straightforward, and reversing it would not make it clearer in my view ?!?

Copy link
Contributor

@mgmeier mgmeier Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It all hinges on how you read the assignment operator; its meaning for ("X" = "Y") being

"X" has alias "Y"

right?
Without knowing how the resulting Map Text Text is applied in code, it's not straightforward; it would be nice to have that clarification in the comment, for someone just wanting to adjust the .nix service definition.

Copy link
Contributor

@deepfire deepfire Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There is a lot of mathematical baggage, where we define:

  • something on the left
  • to be equal to something on the right

..which, on the surface might feel like it's imperative PL bullshit, but it actually runs deeper.

Can you think of a single definition where the "new thing" appears on the right?

Copy link
Contributor

@deepfire deepfire Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There are further reasons:

  • maps are asymmetric -- the thing on the left is unique, the thing on the right not so
  • it makes sense to allow multiple aliases for the same source metric
  • it doesn't make sense to have a single alias to be related to several source metrics

The status quo allows the strange thing and disallows the silly-but-meaningful thing.

# };
};
in pkgs.commonLib.defServiceModule
(lib: with lib;
Expand Down