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

Configure log timestamp format #5050

Merged
merged 3 commits into from Aug 29, 2022

Conversation

SVilgelm
Copy link
Contributor

let user to configure the log's timestamp format by:

  • cli argument log-timestamp-format
  • environment variable OPA_LOG_TIMESTAMP_FORMAT

Closes #2413

Signed-off-by: Sergey Vilgelm sergey@vilgelm.com

@ashutosh-narkar
Copy link
Member

@SVilgelm thanks for the contribution. It would be great if you could add some unit tests. Thanks.

@SVilgelm
Copy link
Contributor Author

if you could add some unit tests

I would be glad to do it, but need some hints. What do you think is the best place for testing new cli argument? I did not find any tests for the log-format or log-level args

@ashutosh-narkar
Copy link
Member

I would have imagined a simple test which collects the log output with the log-timestamp-format flag set and then verifying the result. There may be some other tests like this but not sure.

@philipaconrad
Copy link
Contributor

@SVilgelm You might be able to build off of the existing end-to-end logging tests.

I think there's at least one test case there that checks to make sure the decision log JSON has the correct keys present-- that can almost certainly be upgraded into a check for the timestamp matching a regex for a custom format.

@SVilgelm
Copy link
Contributor Author

SVilgelm commented Aug 25, 2022

@ashutosh-narkar @philipaconrad I added the additional unit test in cmd/run_test.go, unfortunately it is impossible to write the tests in the e2e/logging because the test logger does not add the time field, but the timestamp field , whichis encoded by `json.Marshal. See here https://github.com/open-policy-agent/opa/blob/main/plugins/logs/plugin.go#L918

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 <sergey@vilgelm.com>
Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

checkLogTimeStampFormat(t, params, format)
})
t.Run("environment variable", func(t *testing.T) {
t.Setenv("OPA_LOG_TIMESTAMP_FORMAT", format)
Copy link
Contributor

@srenatus srenatus Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat. I didn't know that function. Pretty handy. https://pkg.go.dev/testing#T.Setenv

@srenatus srenatus merged commit 5e51af9 into open-policy-agent:main Aug 29, 2022
@SVilgelm SVilgelm deleted the log-timestamp-format branch August 29, 2022 16:45
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.

Add support for setting date/time format on logrus messages
4 participants