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

Logging format #565

Closed
2 of 6 tasks
tessus opened this issue May 23, 2023 · 4 comments · Fixed by #567
Closed
2 of 6 tasks

Logging format #565

tessus opened this issue May 23, 2023 · 4 comments · Fixed by #567
Labels
question Further information is requested

Comments

@tessus
Copy link
Contributor

tessus commented May 23, 2023

Have you read the documentation?

  • Yes, but it does not include related information regarding my question.
  • Yes, but the steps described in the documentation do not work on my machine.
  • Yes, but I am having difficulty understanding it and wants clarification.

You are setting up gotify in

  • Docker
  • Linux native platform
  • Windows native platform

Describe your problem

There is no discussion tab available, so I hope it is ok to ask in an issue.

I am a bit puzzled by the log format. I have 2 comments:

  1. Why is there a [GIN] in the beginning of the log line? Can it be anything but [GIN]? If not, it is superfluous and a waste of 6 characters.

  2. The date/time format is a bit ambiguous. I am a fan of the ISO 8601 format, or at least as close as possible. Using / in a date can be tricky.
    I suggest to use the following format:

2023-05-23 02:44:06 -04:00

Using a dash in the date, makes it very clear that the format is YYYY-MM-DD. There is no need to put a dash between the date and time. And timezone info is always a nice to have. Especially when you deal with servers across the globe where the systems are not all set to UTC.

By removing the [GIN] and my other suggestion, we'd even save one character:

[GIN] 2023/05/23 - 02:27:57 | 200 |     589.916µs |       127.0.0.1 | GET      "/"
2023-05-23 02:27:57 -04:00 | 200 |     589.916µs |       127.0.0.1 | GET      "/"

What do you think? Would you accept a PR for this? I'd also fix #428 in the same PR.

@tessus tessus added the question Further information is requested label May 23, 2023
@jmattheis
Copy link
Member

Why is there a [GIN] in the beginning of the log line?

Because that's the default for the gin framework

The date/time format is a bit ambiguous. I am a fan of the ISO 8601 format, or at least as close as possible.

I agree, but I'm hesitant to change this, as it could break existing setups, and I'd say it's not that great of a problem.

@tessus
Copy link
Contributor Author

tessus commented May 23, 2023

I thought about that as well, but the chance is almost non-existent that this change will break a setup.

Unless someone wrote a log parser, but in this case it won't break the functionality of gotify, but something else.
One cannot stop to innovate, just because it might break something down the road. If you follow the road long enough you will always find a reason why it might break.

There is a difference between a change where people will encounter a breaking change and a change that could potentially break somehing downstream. While the former must be considered very hard and long, the latter should not be a reason to not implement an improvement.

There are also options to solve the problem. e.g. should there be many bug reports (which I seriously doubt) due to that change, one can:

  • revert the change
  • add an env var to allow the use of the old format
  • add an env var ro allow the use of the new format

And by one, I mean I will be the one, if such a situation really occurs.

Another example: In #428 you and others found a workaround to filter the /health lines from the log. Does this mean you never want it to be fixed properly in the code, because some people might have to revert that workaround?

Please think about it. I am not asking to change the API or other core functionality.

@tessus
Copy link
Contributor Author

tessus commented May 23, 2023

Ok, I have decided to create 2 PRs. One is the fix for #428 and the other will allow people to use the extended log format. I call it "extended" because it shows more info (TZ).

@jmattheis
Copy link
Member

My point about it could break existing setups is because I've remember some fail2ban tickets, which probably depend on the exact format, but yeah this can be mostly ignored as it's probably not widely used like that.

My main point against this change is that I don't think this is a necessary change. Sure, it's a minor improvement, but you could say it's opinionated. There are some more important tickets I rather spend my time on discussing / working than arguing here.

Would you accept a PR for this?

Probably, but I think the log format could be improved further, by using some standard format that is used by nginx or apache. I f.ex really dislike the whitespaces with the pipes in the log message :P.

Does this mean you never want it to be fixed properly in the code, because some people might have to revert that workaround?

No, I'm not against change or breaking stuff. It's only that I don't think that it's a necessary change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Development

Successfully merging a pull request may close this issue.

2 participants