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

Handle logging #22

Merged
merged 1 commit into from
Nov 1, 2023
Merged

Handle logging #22

merged 1 commit into from
Nov 1, 2023

Conversation

haiks-2382
Copy link
Contributor

@haiks-2382 haiks-2382 commented Oct 25, 2023

Summary

Handle logging

Description

#17
image

Checklist

  • 1. Build successfully on local
  • 2. Format all code files included in the PR
  • 3. No more unconfirmed specs in the PR
  • 4. Tested the normal case and error cases following specs
  • 5. Is there a refactor to the old code? If yes, have you tested the old function and noted the impact yet?
  • 6. Loops have a set length and correct termination conditions
  • 7. There aren't any redundant or duplicate codes in PR (clean code)
  • 8. There aren't any hard-coded in the code in PR
  • 9. Optimization logic handling and optimizations SQL (for example, don't get redundant data or add an index to optimize performance, query only necessary fields...)
  • 10. Set pull-request title by format: Feature new #{id}: {short des}, Bug fix #{id}: {short des}
  • 11. Update issue

Evidence

(Screenshot or Video)

Discussion

Note the part that you are not sure. Comment on your code is OK.

@haiks-2382 haiks-2382 added the type: feature Brand new functionality, features, pages, workflows, endpoints, etc. label Oct 25, 2023
@haiks-2382 haiks-2382 force-pushed the feature.new.17 branch 5 times, most recently from 56550b8 to 149dfa3 Compare October 26, 2023 08:00
} else {
UUID.randomUUID().toString()
}
return clientId
Copy link
Contributor

Choose a reason for hiding this comment

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

Thấy ở đây ngta có override thêm 2 method isAsyncDispatchshouldNotFilterErrorDispatch hình như là để apply cho request lỗi. Hải check thử có cần thêm vào không nhé @haiks-2382
https://github.com/Verdoso/GreenSummer/blob/master/summer-core/src/main/java/org/greeneyed/summer/filter/Slf4jMDCFilter.java#L111

Copy link
Contributor Author

Choose a reason for hiding this comment

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

t thêm rồi nhé,. Xem qua docs thì isAsyncDispatch là kiểu khi filter đc gọi từ 1 async dispatch thì mặc định sẽ ko call func doFilterInternal. shouldNotFilterErrorDispatch thì cũng tương tự mặc định ko call tới doFilterInternal, cụ thể liên quan tới việc response error nếu dùng thymleaf thì phải

@Configuration
class Slf4jMDCFilterConfiguration {
companion object {
const val DEFAULT_RESPONSE_TOKEN_HEADER = "client_id"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const val DEFAULT_RESPONSE_TOKEN_HEADER = "client_id"
const val DEFAULT_RESPONSE_TOKEN_HEADER = "X-Client-Id"

body: Any?
) {
val requestId = httpServletRequest.getAttribute(RequestFilter.REQUEST_ID)
if (requestId != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@haiks-2382 Phần log response time này Hải tách ra thành 1 filter khác như bài này nhé: https://stackoverflow.com/a/42858002/5718493

Để sau này mình dễ dàng bật tắt, đảm bảo các chức năng đc tách bạch không dồn chung vào 1 file

Copy link
Contributor

@khuongln-1346 khuongln-1346 left a comment

Choose a reason for hiding this comment

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

LGTM

@khuongln-1346 khuongln-1346 merged commit ffd01d6 into main Nov 1, 2023
@khuongln-1346 khuongln-1346 deleted the feature.new.17 branch November 1, 2023 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Brand new functionality, features, pages, workflows, endpoints, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants