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 an optional way to override timestamp format in json logs #1330

Merged
merged 9 commits into from
Feb 12, 2024

Conversation

raj-saxena
Copy link
Contributor

@raj-saxena raj-saxena commented Jan 31, 2024

Description

I opened an issue earlier where @jon-whit agreed (#1326 (comment)) that moving towards ISO8601 is a better option.
As such, I raised a PR earlier #1329 where @jon-whit raised a valid point (#1329 (comment)) that we should not break compatibility for any existing users.

I have made the changes accordingly in this PR making ISO8601 as the default format while giving an option to enable the older option too if someone wants that. I understand that this could be a breaking change for some users but in the long run, I believe, this would be a step in the right direction. (adjusted to keep Unix as default after review comments)

Verified visually (since there was no test for this)
image

References

Fixes #1326 while addressing the comment from @jon-whit
#1329 (comment)

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected - provided visual evidence

cmd/run/run.go Outdated Show resolved Hide resolved
pkg/logger/logger.go Outdated Show resolved Hide resolved
.config-schema.json Outdated Show resolved Hide resolved
@jon-whit
Copy link
Member

Going to close #1329 in favor of this.

Copy link
Member

@miparnisari miparnisari left a comment

Choose a reason for hiding this comment

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

Like Jon said, let's keep the backward compatibility. Otherwise lgtm. We'll have to think where to add a test for this. TestCheckLogs already asserts on some fields logged, but I fear that test is becoming too bloated

Copy link
Member

@jon-whit jon-whit left a comment

Choose a reason for hiding this comment

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

Default needs to be Unix.

@raj-saxena raj-saxena force-pushed the add-timeformat-as-optional branch 2 times, most recently from c78f76b to 5513f81 Compare February 1, 2024 10:10
@raj-saxena
Copy link
Contributor Author

raj-saxena commented Feb 1, 2024

@jon-whit @miparnisari I have pushed a commit defaulting to Unix format for json. I have updated the description and the screenshot in the PR too.
Please note that there's no way with the current change to have Unix for text format, but it also isn't something in the scope of the linked issue or being asked for right now.

pkg/logger/logger.go Outdated Show resolved Hide resolved
@jon-whit
Copy link
Member

jon-whit commented Feb 9, 2024

@raj-saxena left one last comment. Looks good otherwise. Thanks!

@raj-saxena
Copy link
Contributor Author

Updated the PR @jon-whit. Let's move this forward 🙏🏼

@jon-whit jon-whit merged commit a0779c4 into openfga:main Feb 12, 2024
9 checks passed
@raj-saxena raj-saxena deleted the add-timeformat-as-optional branch February 13, 2024 09:37
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.

Support ISO8601 time format in JSON logs.
3 participants