Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CBR-275] stack logging ontop of 'katip', provides structured logging #3533

Merged
merged 17 commits into from
Sep 15, 2018

Conversation

CodiePP
Copy link
Contributor

@CodiePP CodiePP commented Sep 1, 2018

Description

we adapt our abstract interface Pos.Util.Wlog and direct output to 'katip'.
this should allow a transition from 'log-warper' to 'katip' with minimal changes to the overall codebase.

  • this PR only contains changes to logging related code in 'util' and changes to other modules which need to be applied at the same time
  • especially, startup code and logging initialization in tests
  • PR [CBR-275] cleanup interface required for structured logging #3534 alrady adapted "minimally" the codebase
  • all references to 'log-warper' library have been removed; now, we only use 'katip' for logging

Linked issue

CBR-97 (user story)
CBR-275

Type of change

  • [~] 🐞 Bug fix (non-breaking change which fixes an issue)
  • [~] 🛠 New feature (non-breaking change which adds functionality)
  • [~] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • [~] ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • [~] I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • [~] CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

  • all tests in CI
  • benchmark in scripts/bench/runbench.sh
  • block syncing test

@CodiePP CodiePP requested review from njd42 and erikd September 1, 2018 08:16
@CodiePP CodiePP force-pushed the adiemand/CBR-275/mimic-logging-interface branch 3 times, most recently from 1f63bbf to badf129 Compare September 2, 2018 09:41
Copy link

@njd42 njd42 left a comment

Choose a reason for hiding this comment

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

Can we split this into a "clean up interface" PR and a "first steps to Katip" PR?

This makes the changes (and importantly the potential interactions) clearer - the "clean up" should just be reducing the "surface area" - the replacement should just be the interface module (plus the essential changes to things like logging initialisation) as the PR sequence progresses

@CodiePP
Copy link
Contributor Author

CodiePP commented Sep 2, 2018

yes, moved interface adaptation to PR #3534

@CodiePP CodiePP force-pushed the adiemand/CBR-275/mimic-logging-interface branch 3 times, most recently from 7d35904 to 9fea606 Compare September 2, 2018 14:15
util/src/Pos/Util/Wlog.hs Outdated Show resolved Hide resolved
util/src/Pos/Util/Wlog.hs Outdated Show resolved Hide resolved
erikd
erikd previously approved these changes Sep 3, 2018
Copy link
Member

@erikd erikd left a comment

Choose a reason for hiding this comment

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

Approved as is, but I do think it can be improved by removing the use of RecordWildCards there.

@CodiePP CodiePP force-pushed the adiemand/CBR-275/mimic-logging-interface branch 2 times, most recently from 954c5ea to e8c06c0 Compare September 5, 2018 15:37
util/src/Pos/Util/Wlog.hs Outdated Show resolved Hide resolved
util/src/Pos/Util/Wlog.hs Outdated Show resolved Hide resolved
@CodiePP CodiePP force-pushed the adiemand/CBR-275/mimic-logging-interface branch 2 times, most recently from 5809c0a to 0bd6ee0 Compare September 6, 2018 00:53
@erikd erikd dismissed their stale review September 6, 2018 04:55

Large number of commits added since my review.

@365andreas 365andreas force-pushed the adiemand/CBR-275/mimic-logging-interface branch 3 times, most recently from b9c0fdb to 5b208bb Compare September 7, 2018 12:48
@CodiePP
Copy link
Contributor Author

CodiePP commented Sep 7, 2018

next steps:

  • cleanup this PR by squashing commits
  • testing, testing

@@ -197,7 +200,7 @@ withClient streamWindow transport logic serverAddress@(Node.NodeId _) k = do
, fdcLastKnownBlockVersion = blockVersion
, fdcConvEstablishTimeout = 15000000 -- us
, fdcStreamWindow = streamWindow
, fdcTrace = wlogTrace ("client" <> "diffusion")
, fdcTrace = wlogTrace ("client.diffusion")
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@erikd erikd left a comment

Choose a reason for hiding this comment

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

LGTM!

@CodiePP CodiePP force-pushed the adiemand/CBR-275/mimic-logging-interface branch from c26567e to acd8c64 Compare September 14, 2018 08:46
@CodiePP
Copy link
Contributor Author

CodiePP commented Sep 14, 2018

thank you @erikd for reviewing
rebased on develop again

@CodiePP CodiePP force-pushed the adiemand/CBR-275/mimic-logging-interface branch 2 times, most recently from d684388 to 468b27d Compare September 14, 2018 14:24
Andreas Triantafyllos and others added 16 commits September 15, 2018 10:50
Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
… to 'cardano-sl'

Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
@CodiePP CodiePP force-pushed the adiemand/CBR-275/mimic-logging-interface branch from 468b27d to 77f2e11 Compare September 15, 2018 12:17
@CodiePP
Copy link
Contributor Author

CodiePP commented Sep 15, 2018

rebased on develop

Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
@CodiePP CodiePP force-pushed the adiemand/CBR-275/mimic-logging-interface branch from 77f2e11 to 9365bcb Compare September 15, 2018 12:25
@CodiePP CodiePP merged commit c1c3729 into develop Sep 15, 2018
@CodiePP CodiePP deleted the adiemand/CBR-275/mimic-logging-interface branch October 26, 2018 08:55
KtorZ pushed a commit that referenced this pull request Nov 9, 2018
…logging-interface

[CBR-275] stack logging ontop of 'katip', provides structured logging
KtorZ pushed a commit to input-output-hk/cardano-wallet-legacy that referenced this pull request Nov 9, 2018
…hk/adiemand/CBR-275/mimic-logging-interface

[CBR-275] stack logging ontop of 'katip', provides structured logging
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants