-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
feature: machine-readable logging #326
Conversation
@whyrusleeping @jbenet @mappum At this point, there's one particular design decision I would like some feedback on: It's important to be standardize the keys used to logged metadata. In the ideal case, when logging peers in metadata, they're logged under the same key system-wide. This is a subset of the signifiers in use today:
We only really need two keys:
To make this work with minimal effort, it would be ideal to be able to generate log events without specifying remote/local. One idea is to let peers carry a type (Unspecified (default), Local, Remote). This way, by implementing the
Please critique early and often. Thanks. ps. NB: I've added methods to the Peer interface. I have a proposal for cutting down on the Peer interface and making global peer-management safer, but implementation of this is out of the scope of this PR. (In short, we would perform create, get, and modify operations on the Peerstore itself. The Peer interface would become read-only) |
61b0b1c
to
0693af5
Compare
|
||
var testIdentity = config.Identity{ | ||
PeerID: "QmNgdzLieYi8tgfo2WfTUzNVH5hQK9oAYGVf6dxN12NrHt", | ||
PrivKey: "CAASrRIwggkpAgEAAoICAQCwt67GTUQ8nlJhks6CgbLKOx7F5tl1r9zF4m3TUrG3Pe8h64vi+ILDRFd7QJxaJ/n8ux9RUDoxLjzftL4uTdtv5UXl2vaufCc/C0bhCRvDhuWPhVsD75/DZPbwLsepxocwVWTyq7/ZHsCfuWdoh/KNczfy+Gn33gVQbHCnip/uhTVxT7ARTiv8Qa3d7qmmxsR+1zdL/IRO0mic/iojcb3Oc/PRnYBTiAZFbZdUEit/99tnfSjMDg02wRayZaT5ikxa6gBTMZ16Yvienq7RwSELzMQq2jFA4i/TdiGhS9uKywltiN2LrNDBcQJSN02pK12DKoiIy+wuOCRgs2NTQEhU2sXCk091v7giTTOpFX2ij9ghmiRfoSiBFPJA5RGwiH6ansCHtWKY1K8BS5UORM0o3dYk87mTnKbCsdz4bYnGtOWafujYwzueGx8r+IWiys80IPQKDeehnLW6RgoyjszKgL/2XTyP54xMLSW+Qb3BPgDcPaPO0hmop1hW9upStxKsefW2A2d46Ds4HEpJEry7PkS5M4gKL/zCKHuxuXVk14+fZQ1rstMuvKjrekpAC2aVIKMI9VRA3awtnje8HImQMdj+r+bPmv0N8rTTr3eS4J8Yl7k12i95LLfK+fWnmUh22oTNzkRlaiERQrUDyE4XNCtJc0xs1oe1yXGqazCIAQIDAQABAoICAQCk1N/ftahlRmOfAXk//8wNl7FvdJD3le6+YSKBj0uWmN1ZbUSQk64chr12iGCOM2WY180xYjy1LOS44PTXaeW5bEiTSnb3b3SH+HPHaWCNM2EiSogHltYVQjKW+3tfH39vlOdQ9uQ+l9Gh6iTLOqsCRyszpYPqIBwi1NMLY2Ej8PpVU7ftnFWouHZ9YKS7nAEiMoowhTu/7cCIVwZlAy3AySTuKxPMVj9LORqC32PVvBHZaMPJ+X1Xyijqg6aq39WyoztkXg3+Xxx5j5eOrK6vO/Lp6ZUxaQilHDXoJkKEJjgIBDZpluss08UPfOgiWAGkW+L4fgUxY0qDLDAEMhyEBAn6KOKVL1JhGTX6GjhWziI94bddSpHKYOEIDzUy4H8BXnKhtnyQV6ELS65C2hj9D0IMBTj7edCF1poJy0QfdK0cuXgMvxHLeUO5uc2YWfbNosvKxqygB9rToy4b22YvNwsZUXsTY6Jt+p9V2OgXSKfB5VPeRbjTJL6xqvvUJpQytmII/C9JmSDUtCbYceHj6X9jgigLk20VV6nWHqCTj3utXD6NPAjoycVpLKDlnWEgfVELDIk0gobxUqqSm3jTPEKRPJgxkgPxbwxYumtw++1UY2y35w3WRDc2xYPaWKBCQeZy+mL6ByXp9bWlNvxS3Knb6oZp36/ovGnf2pGvdQKCAQEAyKpipz2lIUySDyE0avVWAmQb2tWGKXALPohzj7AwkcfEg2GuwoC6GyVE2sTJD1HRazIjOKn3yQORg2uOPeG7sx7EKHxSxCKDrbPawkvLCq8JYSy9TLvhqKUVVGYPqMBzu2POSLEA81QXas+aYjKOFWA2Zrjq26zV9ey3+6Lc6WULePgRQybU8+RHJc6fdjUCCfUxgOrUO2IQOuTJ+FsDpVnrMUGlokmWn23OjL4qTL9wGDnWGUs2pjSzNbj3qA0d8iqaiMUyHX/D/VS0wpeT1osNBSm8suvSibYBn+7wbIApbwXUxZaxMv2OHGz3empae4ckvNZs7r8wsI9UwFt8mwKCAQEA4XK6gZkv9t+3YCcSPw2ensLvL/xU7i2bkC9tfTGdjnQfzZXIf5KNdVuj/SerOl2S1s45NMs3ysJbADwRb4ahElD/V71nGzV8fpFTitC20ro9fuX4J0+twmBolHqeH9pmeGTjAeL1rvt6vxs4FkeG/yNft7GdXpXTtEGaObn8Mt0tPY+aB3UnKrnCQoQAlPyGHFrVRX0UEcp6wyyNGhJCNKeNOvqCHTFObhbhO+KWpWSN0MkVHnqaIBnIn1Te8FtvP/iTwXGnKc0YXJUG6+LM6LmOguW6tg8ZqiQeYyyR+e9eCFH4csLzkrTl1GxCxwEsoSLIMm7UDcjttW6tYEghkwKCAQEAmeCO5lCPYImnN5Lu71ZTLmI2OgmjaANTnBBnDbi+hgv61gUCToUIMejSdDCTPfwv61P3TmyIZs0luPGxkiKYHTNqmOE9Vspgz8Mr7fLRMNApESuNvloVIY32XVImj/GEzh4rAfM6F15U1sN8T/EUo6+0B/Glp+9R49QzAfRSE2g48/rGwgf1JVHYfVWFUtAzUA+GdqWdOixo5cCsYJbqpNHfWVZN/bUQnBFIYwUwysnC29D+LUdQEQQ4qOm+gFAOtrWU62zMkXJ4iLt8Ify6kbrvsRXgbhQIzzGS7WH9XDarj0eZciuslr15TLMC1Azadf+cXHLR9gMHA13mT9vYIQKCAQA/DjGv8cKCkAvf7s2hqROGYAs6Jp8yhrsN1tYOwAPLRhtnCs+rLrg17M2vDptLlcRuI/vIElamdTmylRpjUQpX7yObzLO73nfVhpwRJVMdGU394iBIDncQ+JoHfUwgqJskbUM40dvZdyjbrqc/Q/4z+hbZb+oN/GXb8sVKBATPzSDMKQ/xqgisYIw+wmDPStnPsHAaIWOtni47zIgilJzD0WEk78/YjmPbUrboYvWziK5JiRRJFA1rkQqV1c0M+OXixIm+/yS8AksgCeaHr0WUieGcJtjT9uE8vyFop5ykhRiNxy9wGaq6i7IEecsrkd6DqxDHWkwhFuO1bSE83q/VAoIBAEA+RX1i/SUi08p71ggUi9WFMqXmzELp1L3hiEjOc2AklHk2rPxsaTh9+G95BvjhP7fRa/Yga+yDtYuyjO99nedStdNNSg03aPXILl9gs3r2dPiQKUEXZJ3FrH6tkils/8BlpOIRfbkszrdZIKTO9GCdLWQ30dQITDACs8zV/1GFGrHFrqnnMe/NpIFHWNZJ0/WZMi8wgWO6Ik8jHEpQtVXRiXLqy7U6hk170pa4GHOzvftfPElOZZjy9qn7KjdAQqy6spIrAE94OEL+fBgbHQZGLpuTlj6w6YGbMtPU8uo7sXKoc6WOCb68JWft3tejGLDa1946HAWqVM9B/UcneNc=", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have good context for this, but is it a good idea to commit a private key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's fine since its just for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Yeah. This is a unit-test mock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I was just browsing through and wanted to make sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cleichner thanks for checking. btw, lots of security stuff in IPFS is not yet done correctly. (like we store the keys we generate in your config file). no one should assume ipfs is secure yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbenet , how should the keys be stored then ideally?
This branch is pretty far behind master at the moment. I'd like to rebase. Anyone working on this actively? |
i havent touched it, youre good to go as far as im concerned |
SGTM. Rebasing now. @whyrusleeping here's a preview of the logging output:
|
554a87e
to
fb1829a
Compare
I will document the API so it's more accessible. Here's a brief outline of my intentions for this PR.
backend packages |
650e41c
to
538426e
Compare
@whyrusleeping @mappum @jbenet ready for code review |
to aid in testing
test(time) expose time format var rename time format misc(util/time) don't need this anymore License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
a wrapper around the util.Logger metadata is loggable License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
Experimenting with LD JSON logging License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
TODO use a writer to get the command output resulting appearance: ``` go-ipfs (feat/machine-readable-logging) λ. make install_2; ipfs2 init -f cd cmd/ipfs2 && go install initializing ipfs node at /Users/btc/.go-ipfs generating key pair... peer identity: %s QmcPLgrqfvnneJ3vr3oGpkTiS9Psx8p3HxevjnoZYToDRm ``` License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
now, logrus does not automatically inject time, msg, level metadata License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@jbenet we may want to consider setting the default type to Remote. I left the default as Unspecified just for safety. License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
aa42baa
to
6291c25
Compare
// TODO need to decide whether to expose the override as a file or a | ||
// directory. That is: should we allow the user to also specify the | ||
// name of the file? | ||
// TODO cmds.StringOption("event-logs", "l", "Location for machine-readable event logs"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can cross that bridge when people ask for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@maybebtc a few random comments but everything here LGTM!! |
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
…splaying messages to the user @jbenet this commit re-introduces the peer identity line. It's very useful. I understand it may have been removed because of the clashing of the IDs. To alleviate this, this commit places some negative space between the two lines. 'to test' -> 'to get started' as a stronger call to action ``` λ. ipfs2 init -f initializing ipfs node at /Users/btc/.go-ipfs generating key pair...done. peer identity: QmWzjxNEYKjDAxuHJqvtLP1dZTDjreBSUsArWoHai1v9yP to get started, enter: ipfs cat QmYpv2VEsxzTTXRYX3PjDg961cnJE3kY1YDXLycHGQ3zZB ``` License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
2fec66a
to
c8c062f
Compare
@jbenet i presume you prefer it without the period License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
rebased and merged #movefastandfixthings |
feature: machine-readable logging
}) | ||
if err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This snuck in. i only caught it in my email. Let's not do this pattern. Please pass in a writer, if you want to redirect (or silence) the ouput
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not to silence the output. It's to protect the messages from being accidentally modified by someone making changes to the identity code. Otherwise we'll face regressions.
@maybebtc , cudos, so cool!! 👍 @maybebtc , you are saying you are going to work on API documentation for this now? I should stay in tune to make sure it can be picked up by In this example: {"event":"sentMessage","localPeer":{"id":"EiCsOiFNw0RDqkoNuVeGtGoz4JrHVdLoZhOBhU7dbaSOCg==","latency":0},"message":{"type":"PING"},"system":"dht","time":"2014-11-16T14:04:45.554426994Z","remotePeer":{"id":"EiCwSlfUDsoTiAnxOadrEgRDM8N0A5HJvxzp2OIaeSEL/Q==","latency":0}} does |
Yes. |
And it's okay if we have to feed ipfs log output through an |
providers: run datastore GC concurrently
updated:
This PR adds support for machine-readable logging. Here's the skinny:
The
eventlog
package extends our existing logging functionality to provide structured, machine-readable logs. It exposes theeventlog.Logger
which wraps theutil.Logger
, making it a drop-in replacement.By implementing the
Loggable
interface, objects marshal themselves intomap[string]interface{}
with keys that are consistent across the entire project.By default, logs are written to
~/.go-ipfs/logs/events.log
as line-delimited JSON. This can be overridden at config initialization or updated from the CLI withipfs config
. The init override flag has not yet been exposed.Log initialization is performed as a pre-command hook. We take care to not read the config when the executing command will not read the config.
Now we can grep our logs and transform them for protocol-agnostic analysis.
future work
dependencies
logrus
(MIT) aims to provide functionality similar to oureventlog
package. While leveraginglogrus
allows us to keep our package small while we iterate on the design, I recommend we move off of this once we make decisions about 1) leveled logging and 2) alternatives to statically importingvar log = Logger(package)
in client packages.lumberjack
(MIT) provides log file rotation. I predict it will be quite useful as a medium/long-term dependency.additional info