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

feat(promslog): add NewWithWriter() to allow logging other than stderr #686

Closed
wants to merge 1 commit into from

Conversation

tjhop
Copy link
Contributor

@tjhop tjhop commented Sep 2, 2024

Accepting an io.Writer makes it easy to plug-n-play with slog directly while also making the logging output destination configurable. We explicilty make no attempt to manage the underlying writer and leave it to the caller to ensure proper management/cleanup. Example usage to allow writing a JSON file:

file, err := os.OpenFile("/tmp/json.log", os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0600)
if err != nil {
        panic(err)
}
defer file.Close()

logger := promslog.NewWithWriter(&promslog.Config{}, file)
logger.Info("I'm writing to a file!", "hello", "world")
...

Example output:

~ cat /tmp/json.log
time=2024-09-02T16:43:30.925-04:00 level=INFO source=/home/tjhop/go/src/github.com/prometheus/common/promslog/slog_test.go:162 msg="I'm writing to a file!" hello=world

Accepting an `io.Writer` makes it easy to plug-n-play with slog directly
while also making the logging output destination configurable. We
explicilty make no attempt to manage the underlying writer and leave it
to the caller to ensure proper management/cleanup. Example usage to
allow writing a JSON file:

```go
file, err := os.OpenFile("/tmp/json.log", os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0600)
if err != nil {
        panic(err)
}
defer file.Close()

logger := NewWithWriter(&Config{}, file)
logger.Info("I'm writing to a file!", "hello", "world")
...
```

Example output:
```bash
~ cat /tmp/json.log
time=2024-09-02T16:43:30.925-04:00 level=INFO source=/home/tjhop/go/src/github.com/prometheus/common/promslog/slog_test.go:162 msg="I'm writing to a file!" hello=world
```

Signed-off-by: TJ Hoplock <[email protected]>
@tjhop
Copy link
Contributor Author

tjhop commented Sep 2, 2024

@SuperQ @ArthurSens if you don't mind taking a look when you have time, I'd appreciate it!

@roidelapluie
Copy link
Member

Why not exposing ioWriter publicly instead?

@tjhop
Copy link
Contributor Author

tjhop commented Sep 3, 2024

Why not exposing ioWriter publicly instead?

We could do that! My main thought was that setting the output destination for the logger is a one-and-done situation in log/slog, so it made sense to only allow it to be set at logger creation time. If a user sets the io writer field on a config struct after a logger has already been created, it won't do anything, and so leaving it publicly accessible/settable on the promslog.Config struct could cause users to think that updating the output destination of the logger on-the-fly was possible.

@SuperQ
Copy link
Member

SuperQ commented Sep 3, 2024

I'm not sure we want to have this. We're reasonably opinionated about what and how these things should be used.

One major concern, how do we handle log rotations with this file mode?

@tjhop
Copy link
Contributor Author

tjhop commented Sep 3, 2024

One major concern, how do we handle log rotations with this file mode?

I intend to use this to try and dry out some of prometheus' util/logging package -- in particular these bits around json file logging. As far as ALE/grep can find, these utilities are only used for prometheus' query logging, and we already explicitly do not manage file rotation for that log file:

Prometheus will not rotate the query log itself. Instead, you can use external tools to do so.

I'd argue it's the same as current behavior, we're just moving some common functionality out to the lib instead of prometheus helper packages

@tjhop
Copy link
Contributor Author

tjhop commented Sep 3, 2024

Also, while I intend to use it with files, we're obviously accepting an io.Writer -- there's nothing stopping a user from passing in a bytes.Buffer like I do in our current slog tests, etc. I'm not sure of a reasonable way to account for/manage that from within the library here (open to ideas/corrections, though)

@tjhop
Copy link
Contributor Author

tjhop commented Sep 5, 2024

Another use -- more easily mimicing the behavior of go-kit/log's NewNopLogger().

We have lots of log.NewNopLogger() used throughout prometheus itself, and simply replacing this with promslog.New(&promslog.Config{}) is likely to start logging a whole lot more than intended. If we have a way to make the output io.Writer configurable, we could better replicate the nop logger with promslog.NewWithWriter(&promslog.Config{}, io.Discard)

@roidelapluie
Copy link
Member

While we do not rotate the query log, we close/reopen it on SIGHUP .. after the rotation you have to reload prometheus.

@ArthurSens
Copy link
Member

ArthurSens commented Sep 8, 2024

Another use -- more easily mimicing the behavior of go-kit/log's NewNopLogger().

We have lots of log.NewNopLogger() used throughout prometheus itself, and simply replacing this with promslog.New(&promslog.Config{}) is likely to start logging a whole lot more than intended. If we have a way to make the output io.Writer configurable, we could better replicate the nop logger with promslog.NewWithWriter(&promslog.Config{}, io.Discard)

Would it make sense to have a promslog.NewNopLogger() instead?

promslog.NewWithWriter(&promslog.Config{}, io.Discard) looks weird to me since the writer is an object inside Config. At first glance it looks less strange to just expose the writer inside config

@SuperQ
Copy link
Member

SuperQ commented Sep 8, 2024

Apparently we also need this for Windows. 😞

What about this implementation: #694

@jkroepke
Copy link
Contributor

jkroepke commented Sep 8, 2024

Ah, I didn't saw this PR.

One major concern, how do we handle log rotations with this file mode?

I explicitly tell people that log rotation isn't a problem the exporter has to solve. Looking at web servers, they don't care about this either.

At linux, logrotate should care about that.

Another use -- more easily mimicing the behavior of go-kit/log's NewNopLogger().

Why not just directly initiate slog in tests?


If the io.Writer should be passed at construction time as proposed in this PR, I would recommend using the functional options pattern rather than a new constructor function.

@tjhop
Copy link
Contributor Author

tjhop commented Sep 9, 2024

@jkroepke no worries, thanks for helping. 🙃

I'm fine with exposing the writer directly, it seems to be the consensus amongst the group here. I'll close this PR in favor of #694

@tjhop tjhop closed this Sep 9, 2024
tjhop added a commit to tjhop/common that referenced this pull request Sep 27, 2024
Simple convenience function to return an slog.Logger that writes to
io.Discard. Originally suggested by @ArthurSens
[here](prometheus#686 (comment)),
and requested again by @bboreham
[here](prometheus/prometheus#14906 (comment)).
As Bryan points out in the comment, there's 147 instances where a
discard logger is needed, so a consistent utility function to manage
them seems helpful.

Signed-off-by: TJ Hoplock <[email protected]>
SuperQ pushed a commit that referenced this pull request Sep 27, 2024
Simple convenience function to return an slog.Logger that writes to
io.Discard. Originally suggested by @ArthurSens
[here](#686 (comment)),
and requested again by @bboreham
[here](prometheus/prometheus#14906 (comment)).
As Bryan points out in the comment, there's 147 instances where a
discard logger is needed, so a consistent utility function to manage
them seems helpful.

Signed-off-by: TJ Hoplock <[email protected]>
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 this pull request may close these issues.

5 participants