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.

Sign up for GitHub

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

Jump to bottom

Refactor Router Logger #17308

Merged
merged 58 commits into from
Jan 20, 2022
Merged

Refactor Router Logger #17308

merged 58 commits into from
Jan 20, 2022

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Oct 14, 2021 •
edited by zeripath
Loading

Background:

The Router logger was very basic and always emitted started and completed logs which were of limited utility. It was difficult from the router log alone to determine which routing function was responsible for creating the response seen.

Further the routing involved reflection at runtime to determine where to route to - this is inefficient and slow, and there was a bug in that the logging level was not previously logged.

This PR

  • Started logging is now at Trace level
  • Long polling handlers are recognised and slow executing routes are logged.
  • All logging lines (except started) state the name of the function that is currently running.
  • This PR prewraps the router functions meaning that reflection no longer occurs a routing time, as all reflection is precalculated.

⚠️ BREAKING ⚠️

This PR substantially changes the logging format of the router logger. If you use this logging for monitoring e.g. fail2ban you will need to update this to match the new format.

Screenshots

Before

gitea-log1

After

image

zeripath
zeripath reviewed Oct 14, 2021
View reviewed changes
cmd/web.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 14, 2021
@wxiaoguang wxiaoguang force-pushed the optimize-logger branch 2 times, most recently from 44f0fd5 to 20d4018 Compare October 14, 2021 14:58
zeripath
zeripath reviewed Oct 14, 2021
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

I don't completely understand what you're trying to do here but I really dislike the "panic occurred " changes.

cmd/web.go Outdated Show resolved Hide resolved
routers/web/base.go Outdated Show resolved Hide resolved
modules/setting/log.go Show resolved Hide resolved
routers/common/assets.go Outdated Show resolved Hide resolved
routers/common/logger.go Outdated Show resolved Hide resolved
routers/common/logger.go Outdated Show resolved Hide resolved
routers/common/middleware.go Outdated Show resolved Hide resolved
routers/install/routes.go Outdated Show resolved Hide resolved
routers/install/routes.go Outdated Show resolved Hide resolved
@zeripath

This comment has been minimized.

Sign in to view
@wxiaoguang

This comment has been minimized.

Sign in to view
@wxiaoguang wxiaoguang force-pushed the optimize-logger branch 2 times, most recently from 73788b1 to f3a1f40 Compare October 18, 2021 07:13
@wxiaoguang

This comment has been minimized.

Sign in to view
@lunny lunny added the type/enhancement An improvement of existing functionality label Oct 18, 2021
@lunny lunny added this to the 1.16.0 milestone Oct 18, 2021
@lunny lunny added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Oct 18, 2021
@codecov-commenter

This comment has been minimized.

Sign in to view
@zeripath
Add some comments and a test
22b13f2 
Signed-off-by: Andrew Thornton <[email protected]>
@zeripath
Copy link
Contributor

I've sent a small PR to do a few changes but I'll need to think a bit more about this.

@zeripath

This comment has been minimized.

Sign in to view
@6543 6543 modified the milestones: 1.16.0, 1.17.0 Jan 13, 2022
@6543

This comment has been minimized.

Sign in to view
@wxiaoguang

This comment has been minimized.

Sign in to view
zeripath and others added 7 commits January 20, 2022 09:37
@zeripath
Remove logger v1
4cb7e03 
Now 1.16.0 is out and we're at the start of the v1.17 cycle we can make
more radical breaking changes. In which case we can simply remove the
logger v1 changes.

Signed-off-by: Andrew Thornton <[email protected]>
@wxiaoguang
Merge pull request #5 from zeripath/optimize-logger-no-logger-v1
f0d5559 
Remove logger v1
@wxiaoguang
Merge branch 'main' into optimize-logger
00da10b 
# Conflicts:
#	modules/web/route.go
@zeripath
fix broken conflicts resolution
8d143fe 
Signed-off-by: Andrew Thornton <[email protected]>
@wxiaoguang
Merge branch 'main' into optimize-logger
3e77481
@wxiaoguang
Use TRACE level to show router started message
f2cfdcf
@wxiaoguang
Fix document
5a39871
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 20, 2022
@zeripath zeripath changed the title Optimize logger Refactor Router Logger Jan 20, 2022
@wxiaoguang wxiaoguang merged commit 5bf8d54 into go-gitea:main Jan 20, 2022
@wxiaoguang wxiaoguang deleted the optimize-logger branch January 20, 2022 11:41
@zeripath
Copy link
Contributor

Although this is potentially an extremely breaking change - we are making this change right at the start of the 1.17 cycle therefore we have time for people to spot this and complain.

If necessary we can implement the changes in wxiaoguang#4 and rediscuss the log levels for started etc.

@zeripath zeripath added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Jan 20, 2022
@zeripath zeripath mentioned this pull request Jan 20, 2022
Closed
zjjhot pushed a commit to zjjhot/gitea that referenced this pull request Jan 21, 2022
Merge remote-tracking branch 'giteaoffical/main'
0e80795 
* giteaoffical/main:
  [skip ci] Updated translations via Crowdin
  Refactor jwt.StandardClaims to RegisteredClaims (go-gitea#18344)
  format with gofumpt (go-gitea#18184)
  Enable deprecation error for v1.17.0 (go-gitea#18341)
  Use correct translation key for errors (go-gitea#18342)
  Refactor Router Logger (go-gitea#17308)
  Updated Chroma to v0.10.0 (go-gitea#18270)
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@wxiaoguang
Refactor Router Logger (go-gitea#17308)
2ca5578 
Make router logger more friendly, show the related function name/file/line.

[BREAKING]
This PR substantially changes the logging format of the router logger. If you use this logging for monitoring e.g. fail2ban you will need to update this to match the new format.
@wxiaoguang wxiaoguang mentioned this pull request Mar 31, 2022
Open
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath zeripath approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
6 participants
@wxiaoguang @zeripath @codecov-commenter @lunny @6543 @GiteaBot