Skip to content
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

Zap doesn't provide a simple way to add a writer #66

Closed
lanzafame opened this issue Oct 7, 2019 · 8 comments · Fixed by #87
Closed

Zap doesn't provide a simple way to add a writer #66

lanzafame opened this issue Oct 7, 2019 · 8 comments · Fixed by #87

Comments

@lanzafame
Copy link
Contributor

Looking at moving go-ipfs to #65 's version of go-log, and one of the sticking points is converting the functionality of the writer.WriterGroup.AddWriter call. Zap provides a way to redirect to another logger with a way of undoing the redirection or to add another SyncWriter or you can open a new output but that requires the output have a URL... I shall come back to this issue once I have dug into it more.

@lanzafame
Copy link
Contributor Author

//cc @Kubuxu @frrist

@Kubuxu
Copy link
Member

Kubuxu commented Oct 7, 2019

@lanzafame I guess you need that for ipfs log tail?

@frrist
Copy link
Member

frrist commented Oct 16, 2019

ipfs log tail was a stream of event logs iirc. Go-log has deprecated event log code, and #65 removes the deprecated code. Given that, I can't see a reason for keeping the log tail command.

@lanzafame
Copy link
Contributor Author

That solves my problem. I will just get that ok'ed by the powers that be.

@Stebalien are you ok with the removal of ipfs log tail?

@Kubuxu
Copy link
Member

Kubuxu commented Oct 22, 2019

Another option would be to allow streaming all logs, which would be much harder (but possible).

@Stebalien
Copy link
Member

Discussed in-person with @lanzafame: I suggested we keep ipfs log tail as it shouldn't be that hard to get working simply by implementing a custom WriteSyncer.

However, I didn't think that through. ipfs log tail spits out structured logs but the WriteSyncer will get serialized logs. Unfortunately, I'm not sure how to get both structured and unstructured logs out of zap.

So, I've changed my mind. Let's just deprecate this command for now.

@Stebalien
Copy link
Member

Actually, I take that back. It turns out zap has a nice NewTee function. We can use this to combine two cores: one that's only enabled when ipfs log tail is running and that spits out json and one that logs to standard error (or the user's configured logging system). That means users can pretty logs in their system log but still query a structured log.

If this turns out to be too much work, I'm fine punting. But it doesn't look difficult.

@Kubuxu
Copy link
Member

Kubuxu commented Oct 22, 2019

I think, we would need to forgo the simplified configuration we do with ZapCfg and create cores manually.

In simplified model (if ipfs log tail can be same format as stderr output):

Cores would be:

  • stderr core that would also be used for ipfs log tail
  • file core (if enabled)

The stderr core would write to: MultiWriteSyncer, that would write into stderr and a custom Writer that would either discard or buffer logs for streaming to cli.

lanzafame added a commit to libp2p/go-libp2p-peerstore that referenced this issue Oct 29, 2019
Part of the work to unblock: ipfs/go-log#66

License: MIT
Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>
iand added a commit to iand/go-log that referenced this issue May 18, 2020
Adds one package level function NewPipeReader which returns a reader
that can be used to listen into the logging output.

Fixes ipfs#66 and allows go-filecoin
to implement go-filecoin log tail using v2 of go-log
iand added a commit to iand/go-log that referenced this issue May 24, 2020
Another take on ipfs#66

This change introduces a multicore implementation that allows
additonal zap cores to be added at runtime. Each core can be
configured with distinct encoders and writers which makes
it possible to tail the logs in a program using a different
log format (e.g. program may log in json but interactive
tailing can be in plaintext). This comes at the cost of an
additional mutex that must be taken on each write since the
destination of the write is mutable.

It reworks (and simplifies) the pipe reader implementation
added in ipfs#87 and adds a
new function NewPipeReaderFormat which creates a pipe reader
using a different output format.

This change also allows us to reconfigure logging after it has been
initialised by swapping out the initial zap core with a
newly configured one. This solves the problem that I attempted
in ipfs#88

Adds a Config struct to enable easier reconfiguration
of the logger. The default config is still read from the
environment in init() but a client of the package can now call
SetupLogging again with a custom config to adjust logging after
initialization.

Note that the fix for ipfs#83
is slightly different. If a file is specified using GOLOG_FILE
then logging to stderr will be disabled but logging can be
configured to write to both file and stderr by calling SetupLogging
with the appropriate configuration.
iand added a commit to iand/go-log that referenced this issue May 25, 2020
Another take on ipfs#66

This change introduces a multicore implementation that allows
additonal zap cores to be added at runtime. Each core can be
configured with distinct encoders and writers which makes
it possible to tail the logs in a program using a different
log format (e.g. program may log in json but interactive
tailing can be in plaintext). This comes at the cost of an
additional mutex that must be taken on each write since the
destination of the write is mutable.

It reworks (and simplifies) the pipe reader implementation
added in ipfs#87 and adds a
new function NewPipeReaderFormat which creates a pipe reader
using a different output format.

This change also allows us to reconfigure logging after it has been
initialised by swapping out the initial zap core with a
newly configured one. This solves the problem that I attempted
in ipfs#88

Adds a Config struct to enable easier reconfiguration
of the logger. The default config is still read from the
environment in init() but a client of the package can now call
SetupLogging again with a custom config to adjust logging after
initialization.

Note that the fix for ipfs#83
is slightly different. If a file is specified using GOLOG_FILE
then logging to stderr will be disabled but logging can be
configured to write to both file and stderr by calling SetupLogging
with the appropriate configuration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants