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

[WIP] add option for extended logging #567

Merged
merged 1 commit into from May 24, 2023

Conversation

tessus
Copy link
Contributor

@tessus tessus commented May 23, 2023

I believe there is a bug in the config code.

As soon as there is a default value defined in the struct, env vars seem to be ignored.

I have added a debug output for you to see the problem:

$ export GOTIFY_EXTENDEDLOGFORMAT=false
$ ./gotify-server
init() ExtendedLogFormat = true
Starting Gotify version 2.2.4-4-g62a1c99@2023-05-23-17:37:16
Started Listening for plain HTTP connection on :80

If you remove the default default:"true" in config/config.go, it works. (It will heed the value of GOTIFY_EXTENDEDLOGFORMAT.)

I have marked this PR as work in progress, since I ran into this little problem and because you haven't decided whether you want to add this yet.

However, please let me stress this again: a TZ info is rather important. I am pretty sure that most system admins will agree. The "[GIN] " text is also not very useful.
You also mentioned using a standard format like Apache/nginx. I have to say that the default Apache or NCSA log format does not use a proper date format. Your log looks actually quite nice. The pipe symbols are perfectly valid field separators, because they are not part of any of the fields' potential values.

Fixes #565

@tessus tessus requested a review from a team as a code owner May 23, 2023 22:06
config/config.go Outdated Show resolved Hide resolved
- Remove unnecessary prefix
- Use RFC3339 format
Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

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

I've changed the date time format to RFC3339.

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (8f51a2c) 86.09% compared to head (9676fe9) 86.09%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #567   +/-   ##
=======================================
  Coverage   86.09%   86.09%           
=======================================
  Files          45       45           
  Lines        1568     1568           
=======================================
  Hits         1350     1350           
  Misses        137      137           
  Partials       81       81           
Impacted Files Coverage Δ
router/router.go 88.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jmattheis jmattheis merged commit a37afce into gotify:master May 24, 2023
3 checks passed
@tessus
Copy link
Contributor Author

tessus commented May 24, 2023

RFC3339 works too. ;-) I didn't use it, because I thought you might think that it is not as readable because of the T between the date and time.

Thanks for the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Logging format
2 participants