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

feat: format gin logger #904

Merged
merged 6 commits into from
Mar 9, 2022

Conversation

Belyenochi
Copy link
Contributor

@Belyenochi Belyenochi commented Mar 7, 2022

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What this PR does / why we need it:

Resolves #899

pkg/log/gin_logger.go Outdated Show resolved Hide resolved
pkg/log/gin_logger.go Show resolved Hide resolved
@lingsamuel
Copy link
Member

Simply run make update-license && make update-gofmt to update license header and imports.

@Belyenochi
Copy link
Contributor Author

Simply run make update-license && make update-gofmt to update license header and imports.

Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2022

Codecov Report

Merging #904 (8cf2648) into master (8348d01) will decrease coverage by 0.18%.
The diff coverage is 69.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #904      +/-   ##
==========================================
- Coverage   32.05%   31.86%   -0.19%     
==========================================
  Files          71       72       +1     
  Lines        7775     7895     +120     
==========================================
+ Hits         2492     2516      +24     
- Misses       5009     5103      +94     
- Partials      274      276       +2     
Impacted Files Coverage Δ
pkg/log/gin_logger.go 68.57% <68.57%> (ø)
pkg/api/server.go 82.27% <100.00%> (ø)
pkg/ingress/ingress.go 6.59% <0.00%> (-0.18%) ⬇️
pkg/ingress/secret.go 0.00% <0.00%> (ø)
pkg/ingress/status.go 0.00% <0.00%> (ø)
pkg/ingress/gateway.go 0.00% <0.00%> (ø)
pkg/ingress/endpoint.go 0.00% <0.00%> (ø)
pkg/ingress/apisix_tls.go 0.00% <0.00%> (ø)
pkg/ingress/apisix_route.go 0.00% <0.00%> (ø)
pkg/ingress/endpointslice.go 0.00% <0.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8348d01...8cf2648. Read the comment docs.

@lingsamuel
Copy link
Member

Please fix this:

Error: ../../pkg/log/gin_logger.go:26:2: missing go.sum entry for module providing package github.com/gin-gonic/gin (imported by github.com/apache/apisix-ingress-controller/pkg/log);

Thanks!

@Belyenochi
Copy link
Contributor Author

Please fix this:

Error: ../../pkg/log/gin_logger.go:26:2: missing go.sum entry for module providing package github.com/gin-gonic/gin (imported by github.com/apache/apisix-ingress-controller/pkg/log);

Thanks!

Done

@Belyenochi
Copy link
Contributor Author

@lingsamuel, hi lingsamuel, I observed ci error, check the ingress lb status is updated, I don't know what to do to fix it.

@lingsamuel
Copy link
Member

It's a flaky test #869, just ignore it

c.Next()
cost := time.Since(start)

logger.Infof("path: %s, status: %d, method: %s, query: %s, ip: %s, user-agent: %s, errors: %s, cost: %s",
Copy link
Member

@lingsamuel lingsamuel Mar 8, 2022

Choose a reason for hiding this comment

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

I am not familiar with gin-gonic, does it have similar concept like log level (like report error when 4XX, 5XX)? wdyt @tao12345666333 ? (this is not a change request)

Copy link
Member

Choose a reason for hiding this comment

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

IIRC it does not provide this ability by default, but it can be achieved by customizing the Log output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lingsamuel @tao12345666333 I used error level alone for the return codes of 4xx and 5xx.

Copy link
Member

@lingsamuel lingsamuel left a comment

Choose a reason for hiding this comment

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

LGTM

@tao12345666333
Copy link
Member

Please check the CI errors

@Belyenochi
Copy link
Contributor Author

Please check the CI errors

@tao12345666333, I have some doubts about the CI error. I can't reproduce it locally. It seems to be an unstable CI unit test.

@tao12345666333
Copy link
Member

@Belyenochi let me re-run CI.

@Belyenochi
Copy link
Contributor Author

@Belyenochi let me re-run CI.

It seems that everything is ok.

@tao12345666333 tao12345666333 merged commit 4025151 into apache:master Mar 9, 2022
@tao12345666333
Copy link
Member

merged!

@tao12345666333 tao12345666333 added this to the 1.5.0 milestone Mar 9, 2022
@tao12345666333 tao12345666333 added this to In progress in v1.5 Planning via automation Mar 9, 2022
@tao12345666333 tao12345666333 moved this from In progress to Done in v1.5 Planning Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Unified structured log (Gin)
4 participants