Conversation
There was a problem hiding this comment.
Hey Santi,
Thanks for taking this PR upon you and it seems that it is moving in the right direction!
I added some comments.
I would like to test the functionality of the logger by myself. Could you add documentation on how to do this in the description of your PR?
Am I correct that, while you pave the way in this PR of writing the swap-logs to a separate file, in fact, you are not allowing the user to set it?
holisticode
left a comment
There was a problem hiding this comment.
Well done, I think it's good foundation work and definitely a good start!
A couple of comments:
- If we want to change global debug behavior, we need to coordinate with core. I personally don't hink it's needed and will extend this PR longer, but you may have good arguments for it, so if this is the case, let's propose them
- The main idea of this was to have a separate file with pure "financial" information. So everything related to the functioning of the service (booting, deploying, etc.) should remain in the main logger for coherence (debugging). The new logger only would contain pure swap values and the only thing written to a separate file, nothing else should be written to file (no global logs). So everything like "balance changed", "sent cheque for x amount", "received cheque for x amount", etc. I think that this what the current draft largely does, BUT:
- This becomes complex in an error scenario, because for debugging, you would want to have debug information, which we would not have by default because the default swap level would be 3, and it would be difficult to correlate debug information with the other global logs.
- This is why for me, the ideal situation would be that everything is contained in the same global log, but that only swap logs < 3 are also written to the swap log file. Like a T log. I don't know if this is even possible so let's investigate.
- This also means that we need to go through every swap log and think hard about its log level.
I apologize if I did not make any of this clearer beforehand.
swap/protocol.go
Outdated
| // Stop is a node.Service interface method | ||
| func (s *Swap) Stop() error { | ||
| log.Info("Swap service stopping") | ||
| s.logger.Info("Swap service stopping") |
mortelli
left a comment
There was a problem hiding this comment.
this is in good shape. small corrections
swap/swap.go
Outdated
| // Only messages which have a price will be accounted for | ||
| type Swap struct { | ||
| api API | ||
| audit log.Logger // logger for Swap related messages and audit trail |
There was a problem hiding this comment.
@holisticode can you please explain the advantage of having this as a global variable rather than part of the Swap struct?
mortelli
left a comment
There was a problem hiding this comment.
good work!
this time around i only have naming/comment/style notes to make. therefore, i shall not be blocking this PR from being merged in its current status.
just remember to be absolutely sure all the tests are passing, and that the linter and appveyor tasks are successful as well 👍
|
|
||
| // New - swap constructor with integrity checks | ||
| func New(dbPath string, prvkey *ecdsa.PrivateKey, backendURL string, disconnectThreshold uint64, paymentThreshold uint64) (*Swap, error) { | ||
| func New(logpath string, dbPath string, prvkey *ecdsa.PrivateKey, backendURL string, disconnectThreshold uint64, paymentThreshold uint64) (*Swap, error) { |
There was a problem hiding this comment.
place the logpath argument in the same order as it is used in the function body
There was a problem hiding this comment.
I not sure i understood
could you elaborate a bit more?.
There was a problem hiding this comment.
i think it's a good idea to define parameters in the New function in the same order as they are used in its body. but it's not a major gripe
Thank Rinke, for the review. If none is specified it will be redirected to stdout together will all logs generated by swarm. |
|
|
||
| // newLogger returns a new logger | ||
| func newLogger(logpath string) log.Logger { | ||
| swapLogger := log.New("swaplog", "*") |
There was a problem hiding this comment.
is this the standard way of segregating logs?
the * seems a bit like clutter, although i suppose we need to add something that says swap to the context
Eknir
left a comment
There was a problem hiding this comment.
Approving this PR, good work.
For a follow-up PR, I think it might be a good idea to have all logger-related stuff in a separate file under the SWAP package. The advantage of this will be that it improves the general readability of the core-features of SWAP. Additionally, I would like to see the call to newLogger to be not in the new function, but in a completely serperate function (exposed to the outside), such that we can completely optionally call this from outside SWAP by swap.NewLogger (for example).
As you will be taking this issue one step further in a follow-up PR, I would like to request on thing for that PR (please talk to the team about whether everybody thinks it is a good idea):
* 'master' of github.com:ethersphere/swarm: (32 commits) network/stream: refactor cursors tests (ethersphere#1786) network: Add capabilities if peer from store does not have it (ethersphere#1791) Swap logger (ethersphere#1754) network: Add capability filtered depth calculation (ethersphere#1787) travis: remove go1.12 job (ethersphere#1784) cmd/swarm: correct bzznetworkid flag description (ethersphere#1761) network, pss: Capability in pss (ethersphere#1764) network/stream: handle nil peer in TestNodesExchangeCorrectBinIndexes (ethersphere#1779) protocols, retrieval: swap-enabled messages implement Price (ethersphere#1771) cmd/swarm-smoke: fix waitToPushSynced connection closing (ethersphere#1781) cmd/swarm: simplify testCluster.StartNewNodes (ethersphere#1777) build: increase golangci-lint deadline (ethersphere#1778) docker: ignore build/bin when copying files (ethersphere#1780) swap: fix and rename Peer.getLastSentCumulativePayout (ethersphere#1769) network/stream: more resilient TestNodesCorrectBinsDynamic (ethersphere#1776) network: Add Capabilities to Kademlia database (ethersphere#1713) network: add own address to KademliaInfo (ethersphere#1775) pss: Refactor. Step 2. Refactor forward cache (ethersphere#1742) all: configurable payment/disconnect thresholds (ethersphere#1729) network/stream/v2: more resilient TestNodesExchangeCorrectBinIndexes (ethersphere#1760) ...
Resolves issue #1593
swap logs are using a separate logger with the correct identifier.
also added a flag to redirect the swap logs to a file.
Overview of changes
Swap package contains its own audit logger for swap related logs.
new flag --swap-audit-logpath which corresponds to the final location of swap logs during execution.
implements a rotating file handler, so it can split the log file into multiple pieces.
splits the output between regular logs and files
If no flag is provided the logs will be sent to swarm logger.
Tests that logs are being logged to file correctly.