Add integration test for CLI tracer options #1257

rvl · 2020-01-10T08:16:45Z

Relates to #1235.

Overview

758ab28
Add Cardano.Wallet.Logging.stdoutTextTracer
This comes in handy when trying functions in GHCi.
fd4069c
More logging information at program startup
dd22021
tests: Add integration test for per-component tracing

paweljakubas · 2020-01-10T08:51:22Z

lib/jormungandr/exe/cardano-wallet-jormungandr.hs

@@ -396,7 +399,7 @@ helperTracingText = unlines $
    [ "Additional tracing options:"
    , ""
    , "  --log-level SEVERITY     Global minimum severity for a message to be logged."
-    , "                           Defaults to \"INFO\" unless otherwise configured."
+    , "                           Defaults to \"DEBUG\" unless otherwise configured."


Yeah, I had to discard this to make other integration tests happy.

paweljakubas · 2020-01-10T08:56:44Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr.hs

@@ -201,7 +206,7 @@ serveWallet
 serveWallet
    Tracers{..} sTolerance databaseDir hostPref listen lj beforeMainLoop = do
    installSignalHandlers (traceWith applicationTracer MsgSigTerm)
-    traceWith applicationTracer MsgStarting
+    traceWith applicationTracer $ MsgStarting lj


lj is here the only one name not self-descriptive (ie., in 207 line)

OK fixed - I was trying to stay under 80 columns. 😁

paweljakubas · 2020-01-10T08:58:51Z

lib/jormungandr/exe/cardano-wallet-jormungandr.hs

+                                    Logging
+-------------------------------------------------------------------------------}
+
+data Log


we use ApplicationLog in lib/jormungandr/src/Cardano/Wallet/Jormungandr.hs - maybe we could give some prefix to Log ?

I changed it to MainLog.

paweljakubas · 2020-01-10T09:06:37Z

@rvl @KtorZ general question. We have logs datatypes decentralized thought the code. Why we didn't decide to put them in one place - I see this done in cardano-node for logging. Maybe it is better to have them in one place for maintenance (less expansive ripple effect in the codebase when changes happen) or it follows from how monitoring-framework is constructed? Nevertheless, I like that is is scattered in modules that logging concerns (modules take responsibility of their logging)... - it will be easier to decouple them and make them independent what is the path we are taking

rvl · 2020-01-10T09:09:09Z

I asked about this on slack yesterday because I agree with your concern.

My preference would be to have logging types and ToText instances in the same module as the code which uses them, and the other instances (DefineSeverity, ToObject, etc) in an "orphans" module.

paweljakubas · 2020-01-16T13:48:57Z

lib/jormungandr/test/integration/Test/Integration/Jormungandr/Scenario/CLI/Server.hs

+                let componentDebugLogs = grep "stake-pool-db" allDebugLogs
+                let networkDebugLogs = grep "network" allDebugLogs
+                length componentDebugLogs `shouldNotBe` 0
+                length networkDebugLogs `shouldBe` 0


so we here are serving with log-level=debug. after 5 seconds we close log handle. and then look into logs "greping" Debug ones. And we do not see neither stake-pool-db nor network related logs. What I do not undestand is why this test is titled Serve debug logs for one component Don't we want to show that something is present here, rather than absent?

ah, my bad. there is shouldNotBe - yes, that was what I expected to capture!!!! 👍

paweljakubas · 2020-01-16T13:52:34Z

lib/jormungandr/test/integration/Test/Integration/Jormungandr/Scenario/CLI/Server.hs

+                logged <- T.lines <$> TIO.readFile logs
+                let countLogs comp = length $
+                        filter (T.isInfixOf $ "cardano-wallet." <> comp) logged
+                countLogs "network" `shouldBe` 0


indeed, after trace-network is off I expect to see no network related logs 👍

paweljakubas

LGTM! (just slight suggestion to add more tests)

paweljakubas · 2020-01-16T14:02:41Z

lib/jormungandr/test/integration/Test/Integration/Jormungandr/Scenario/CLI/Server.hs

+                        , "--node-port", show (ctx ^. typed @(Port "node"))
+                        , "--random-port"
+                        , "--genesis-block-hash", block0H
+                        , "--log-level", "debug"


this line is not needed according to defaults - that is my expectation. this is nit-picking, but without it we would show that defaults work properly. Maybe it is good idea (in some next PR) to extend this test in such a way that we play with different combinations of general-log level and component log level and see that defaults are as expected. What happens when component log level is Info and general is Debug, or opposite? Also check two component logs with the same, different log levels... What about showing that when Debug is set then we will not have for sure Info related logs?

I needed to change the default global minimum severity back to Info to prevent debug-level MainLog messages from leaking out.

If the global log level is Info then there will be no Debug-level messages whatsoever. There is a check for that.
It the global log level is Debug then there will be Debug, Info, and higher-severity messages.
I added an assertion for showing that when Info is set then we will have for sure Info related logs.
I think that should cover it.

@rvl would you mind making "debug" the default again and adding this very comment as a note in place where we set this default to DEBUG ? This is good to know and have in the source code. I didn't realize this when I changed the global default to be INFO.

I have put that change into #1282. There are are few extra debug severity MainLog messages printed due to this, but I think it's ok.

rvl · 2020-01-17T04:35:46Z

bors r+

1257: Add integration test for CLI tracer options r=rvl a=rvl Relates to #1235. # Overview - 758ab28 Add Cardano.Wallet.Logging.stdoutTextTracer This comes in handy when trying functions in GHCi. - fd4069c More logging information at program startup - dd22021 tests: Add integration test for per-component tracing Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>

iohk-bors · 2020-01-17T04:43:05Z

Build failed

buildkite/cardano-wallet

rvl · 2020-01-17T05:26:36Z

Oops

bors r+

1257: Add integration test for CLI tracer options r=rvl a=rvl Relates to #1235. # Overview - 758ab28 Add Cardano.Wallet.Logging.stdoutTextTracer This comes in handy when trying functions in GHCi. - fd4069c More logging information at program startup - dd22021 tests: Add integration test for per-component tracing Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>

iohk-bors · 2020-01-17T05:34:43Z

Build failed

buildkite/cardano-wallet

KtorZ

LGTM

KtorZ · 2020-01-17T17:50:55Z

lib/core/src/Cardano/Wallet/Logging.hs

+-- | Creates a tracer that prints any 'ToText' log message. This is useful for
+-- debugging functions in the REPL, when you need a 'Tracer' object.
+stdoutTextTracer :: (MonadIO m, ToText a) => Tracer m a
+stdoutTextTracer = Tracer $ liftIO . T.putStrLn . toText


KtorZ · 2020-01-17T17:53:00Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr.hs

+    hostPref
+    listen
+    backend
+    beforeMainLoop = do


That's the kind of moment where I think it's totally fine to not be under 80-lines 😬 ...

This comes in handy when trying functions in GHCi.

rvl · 2020-01-20T04:11:41Z

bors r+

1257: Add integration test for CLI tracer options r=rvl a=rvl Relates to #1235. # Overview - 758ab28 Add Cardano.Wallet.Logging.stdoutTextTracer This comes in handy when trying functions in GHCi. - fd4069c More logging information at program startup - dd22021 tests: Add integration test for per-component tracing Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>

iohk-bors · 2020-01-20T04:21:44Z

Build failed

buildkite/cardano-wallet

rvl · 2020-01-20T05:53:01Z

Needed to update the launcher tests too, and update some tracer names.

bors r+

1257: Add integration test for CLI tracer options r=rvl a=rvl Relates to #1235. # Overview - 758ab28 Add Cardano.Wallet.Logging.stdoutTextTracer This comes in handy when trying functions in GHCi. - fd4069c More logging information at program startup - dd22021 tests: Add integration test for per-component tracing Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>

iohk-bors · 2020-01-20T06:24:40Z

Build succeeded

buildkite/cardano-wallet

1282: cardano-wallet-jormungandr: Set default minimum log-level to Debug r=KtorZ a=rvl Relates to #1235. # Overview - Sets default global minimum log level to DEBUG in the CLI. # Comments From #1257 (comment) Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>

rvl self-assigned this Jan 10, 2020

paweljakubas reviewed Jan 10, 2020

View reviewed changes

paweljakubas added this to the (WB-31) Allow logs to be filtered by components milestone Jan 14, 2020

rvl force-pushed the rvl/1235/logging-test branch from 6fd4a60 to dd22021 Compare January 16, 2020 08:38

rvl marked this pull request as ready for review January 16, 2020 08:47

paweljakubas reviewed Jan 16, 2020

View reviewed changes

paweljakubas approved these changes Jan 16, 2020

View reviewed changes

paweljakubas reviewed Jan 16, 2020

View reviewed changes

rvl force-pushed the rvl/1235/logging-test branch from dea37f4 to d0a92a8 Compare January 17, 2020 04:51

KtorZ approved these changes Jan 17, 2020

View reviewed changes

KtorZ mentioned this pull request Jan 18, 2020

Log of severity "DEBUG" shows up as "INFO" #1273

Closed

rvl added 3 commits January 19, 2020 16:37

Add Cardano.Wallet.Logging.stdoutTextTracer

8052fbe

This comes in handy when trying functions in GHCi.

More logging information at program startup

053100b

tests: Add integration test for per-component tracing

967b995

rvl force-pushed the rvl/1235/logging-test branch from d0a92a8 to 967b995 Compare January 19, 2020 06:41

rvl mentioned this pull request Jan 20, 2020

cardano-wallet-jormungandr: Set default minimum log-level to Debug #1282

Merged

tests: update tracer name in CLI test

a919273

rvl force-pushed the rvl/1235/logging-test branch from 64b6f3f to 80731aa Compare January 20, 2020 05:49

tests: also update launcher cli tests

2281dc5

rvl force-pushed the rvl/1235/logging-test branch from 80731aa to 2281dc5 Compare January 20, 2020 05:50

iohk-bors bot merged commit 2281dc5 into master Jan 20, 2020

rvl deleted the rvl/1235/logging-test branch January 20, 2020 07:29

rvl mentioned this pull request Jan 29, 2020

Allow logs to be filtered by components #1235

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration test for CLI tracer options #1257

Add integration test for CLI tracer options #1257

rvl commented Jan 10, 2020 •

edited

paweljakubas Jan 10, 2020

rvl Jan 16, 2020

paweljakubas Jan 10, 2020

rvl Jan 16, 2020

paweljakubas Jan 10, 2020

rvl Jan 16, 2020

paweljakubas commented Jan 10, 2020 •

edited

rvl commented Jan 10, 2020

paweljakubas Jan 16, 2020

paweljakubas Jan 16, 2020

paweljakubas Jan 16, 2020

paweljakubas left a comment •

edited

paweljakubas Jan 16, 2020 •

edited

rvl Jan 17, 2020

KtorZ Jan 17, 2020 •

edited

rvl Jan 20, 2020

rvl commented Jan 17, 2020

iohk-bors bot commented Jan 17, 2020

rvl commented Jan 17, 2020

iohk-bors bot commented Jan 17, 2020

KtorZ left a comment

KtorZ Jan 17, 2020

KtorZ Jan 17, 2020

rvl commented Jan 20, 2020

iohk-bors bot commented Jan 20, 2020

rvl commented Jan 20, 2020

iohk-bors bot commented Jan 20, 2020

Add integration test for CLI tracer options #1257

Add integration test for CLI tracer options #1257

Conversation

rvl commented Jan 10, 2020 • edited

Overview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Jan 10, 2020 • edited

rvl commented Jan 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas left a comment • edited

Choose a reason for hiding this comment

paweljakubas Jan 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ Jan 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl commented Jan 17, 2020

iohk-bors bot commented Jan 17, 2020

Build failed

rvl commented Jan 17, 2020

iohk-bors bot commented Jan 17, 2020

Build failed

KtorZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl commented Jan 20, 2020

iohk-bors bot commented Jan 20, 2020

Build failed

rvl commented Jan 20, 2020

iohk-bors bot commented Jan 20, 2020

Build succeeded

rvl commented Jan 10, 2020 •

edited

paweljakubas commented Jan 10, 2020 •

edited

paweljakubas left a comment •

edited

paweljakubas Jan 16, 2020 •

edited

KtorZ Jan 17, 2020 •

edited