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

Add support for setting date/time format on logrus messages #2413

Closed
tsandall opened this issue May 19, 2020 · 10 comments · Fixed by #5050
Closed

Add support for setting date/time format on logrus messages #2413

tsandall opened this issue May 19, 2020 · 10 comments · Fixed by #5050

Comments

@tsandall
Copy link
Member

Some users have requested ability to override the logrus date/time format to lower the precision--some log aggregation systems do not do well with the number of sub-second digits that are included in the default format. We need to investigate how to set the date/time format via the logrus API and then decide how to pipe the configuration down from the entrypoint.

@timakin
Copy link
Contributor

timakin commented May 20, 2020

@tsandall

Customizing the format of logrus timestamp needs the following two configurations.

  1. FullTimestamp
  2. TimestampFormat
package main

import (
	"github.com/sirupsen/logrus"
)

func main() {
	logrus.Info("before")

	formatter := &logrus.TextFormatter{
		FullTimestamp:   true,
		TimestampFormat: "2006-01-02 15:04:05",
	}
	logrus.SetFormatter(formatter)

	logrus.Info("after")
}
$ INFO[0000] before                                       
$ INFO[2020-05-20 21:23:18] after    

How about adding log-timestamp-format flag to implement the date/time format injection?
It seems a kind of the same level logging option with log-level and log-format in runtime.go.

Of course, it must require the validation of the time format.

@patrick-east
Copy link
Contributor

I like the idea of having a configurable log-timestamp-format option, seems like a pretty easy thing to add on to the opa run CLI too

@tsandall
Copy link
Member Author

👍 a --log-timestamp-format option seems nice. The only other option I could imagine is a less powerful flag that lets users limit the number of sub-second digits. The only benefit I can think of is that if/when we switch to another logging library, it might be easier to continue supporting? I don't feel strongly about this--let's just go with --log-timestamp-format.

@timakin
Copy link
Contributor

timakin commented May 21, 2020

Okay, I’ll give it a try if it isn’t missing the point.
Is it better to wait until you members take this issue in the project TODO list?

@tsandall
Copy link
Member Author

@timakin if you can work on this feature that would be excellent! I think the places that will need to change are:

  • cmd/run.go -- this is where the command-line arguments are processed
  • runtime/runtime.go -- the LoggingConfig struct and setupLogging function will need to be updated

I'm not sure how best to test these changes (aside from doing it manually)--maybe @patrick-east has thoughts.

@patrick-east
Copy link
Contributor

I'm not sure how best to test these changes (aside from doing it manually)--maybe @patrick-east has thoughts.

I'm not 100% sure, but I think you can have logrus write its output into any io.Writer so we should be able to use to start up the runtime (getting some logs emitted) and writing them into a buffer and then parse it and check the timestamp field format.

@ashutosh-narkar
Copy link
Member

WRT testing, we could do something similar to this test.

@rahulchheda
Copy link

Is somebody working on this issue? I would like to contribute!

@ashutosh-narkar
Copy link
Member

ashutosh-narkar commented Jun 19, 2020

@timakin are you planning to work on this ?

@stale
Copy link

stale bot commented Jan 2, 2022

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Jan 2, 2022
SVilgelm added a commit to SVilgelm/opa that referenced this issue Aug 25, 2022
let user to configure the log's timestamp format by:
* cli argument `log-timestamp-format`
* environment variable `OPA_LOG_TIMESTAMP_FORMAT`

Closes open-policy-agent#2413

Signed-off-by: Sergey Vilgelm <[email protected]>
srenatus pushed a commit that referenced this issue Aug 29, 2022
With this, we allow the user to configure the logger's timestamp format by:
* cli argument `log-timestamp-format`
* environment variable `OPA_LOG_TIMESTAMP_FORMAT`

Fixes #2413.

Signed-off-by: Sergey Vilgelm <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants