Skip to content

task : include userId in the log messages by using MDC. #1103

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
Merged
merged 1 commit into from
Jul 29, 2019
Merged

task : include userId in the log messages by using MDC. #1103

merged 1 commit into from
Jul 29, 2019

Conversation

mukeshk
Copy link
Contributor

@mukeshk mukeshk commented Jul 27, 2019

Addressed to #1102

@mystamps-bot
Copy link

mystamps-bot commented Jul 27, 2019 •
edited
Loading

1 Warning
⚠️ danger check: branch gh_1102_include_userid_in_log_messages does not comply with our best practices. Branch name should use the following scheme: ghXXX_meaningful-name where XXX is an issue number. Next time, please, use this scheme :)

Generated by 🚫 Danger

@mukeshk
Copy link
Contributor Author

mukeshk commented Jul 27, 2019 •
edited
Loading

To Discuss:

  1. Package for the MdcLogEnhancerFilter
  2. URL Pattern for the Filter
  3. Position of the UserId in the logged lines.
  4. Few of the Requests are not able to show userId event after login and few are
a>  2019-07-27 20:09:28.148 <UserID>:</UserID> WARN 26456 --- [qtp985342789-28] r.m.web.feature.site.CspController
     b>2019-07-27 20:24:08.823 <UserID>:2</UserID> INFO 26456 --- [qtp985342789-28] r.m.w.f.c.CollectionServiceImpl          : Series #1 (stamps=1) has been added to collection of user #2

While I am navigating after logging across pages, I am not able to get userId.

  1. Temporary I have put a marker x in the logged.
    we can formulate how we want to represent.

@mukeshk
Copy link
Contributor Author

mukeshk commented Jul 27, 2019

Need your ideas.

@php-coder
Copy link
Owner

Awesome! I'll look in it later.

php-coder
php-coder requested changes Jul 27, 2019
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

Thank you!

See my comments.

src/main/java/ru/mystamps/web/config/MvcConfig.java Outdated


@Bean
public FilterRegistrationBean getMdcLogEnhancerFilter() {
Copy link
Owner

Choose a reason for hiding this comment

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

Given that our filter registration is simple, let's just declare that bean as others. In other wods, we don't need to use FilterRegistrationBean here.

See https://stackoverflow.com/a/19830906/352708

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried @bean, also Tried @filter - did not work.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks! I'll take a look tonight.

Copy link
Owner

Choose a reason for hiding this comment

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

Update: tomorrow :/

@mukeshk
Copy link
Contributor Author

mukeshk commented Jul 28, 2019

2019-07-28 06:12:13.897 2 INFO 25796 --- [tp1370384110-30] m.w.f.i.DatabaseImagePersistenceStrategy : Image #1: preview not found

2019-07-28 06:13:19.085 2 INFO 25796 --- [tp1370384110-34] r.m.w.f.c.CollectionServiceImpl : Series #1 (stamps=1) has been added to collection of user #2

2019-07-28 06:14:28.552 2 INFO 25796 --- [tp1370384110-35] r.m.w.f.c.CollectionServiceImpl : Series #1 has been removed from collection of user #2

Things which go through CSP - dont have it.
2019-07-28 06:15:51.603 WARN 25796 --- [tp1370384110-33] r.m.web.feature.site.CspController

How do I test it further? Check for GET/CREATE/REMOVE actions.

@mukeshk mukeshk force-pushed the gh_1102_include_userid_in_log_messages branch 3 times, most recently from 9408307 to 13a11b7 Compare July 28, 2019 00:58
@mukeshk mukeshk changed the title [WIP]task(area/logging) : Include userId in log messages by using MDC task(area/logging) : Include userId in log messages by using MDC Jul 28, 2019
@mukeshk mukeshk marked this pull request as ready for review July 28, 2019 17:20
@mukeshk mukeshk force-pushed the gh_1102_include_userid_in_log_messages branch from 13a11b7 to 4ecf0a5 Compare July 28, 2019 17:30
php-coder
php-coder reviewed Jul 28, 2019
View reviewed changes
@php-coder
Copy link
Owner

Could you, please, modify commit message?

task: include userId in the log messages by using MDC.

Related links:
- http://logback.qos.ch/manual/mdc.html
- http://docs.spring.io/spring-boot/docs/1.5.x/reference/html/boot-features-logging.html

Part of #388
Fix #1102

php-coder
php-coder reviewed Jul 28, 2019
View reviewed changes
@mukeshk mukeshk force-pushed the gh_1102_include_userid_in_log_messages branch from 4ecf0a5 to 646fd3a Compare July 29, 2019 04:02
@mukeshk mukeshk force-pushed the gh_1102_include_userid_in_log_messages branch from 646fd3a to d93b033 Compare July 29, 2019 09:46
@php-coder
Copy link
Owner

The first line of a commit message still has to be updated to

task: include userId in the log messages by using MDC.

@mukeshk mukeshk force-pushed the gh_1102_include_userid_in_log_messages branch from d93b033 to 0630ebc Compare July 29, 2019 13:40
@mukeshk
Copy link
Contributor Author

mukeshk commented Jul 29, 2019

please review.
I have set the pattern like :
logging.pattern.level: [user:%-2X{userId}] %5p

Is this good?

@php-coder
Copy link
Owner

Is this good?

Yes, thank you! Could you update git commit also? (see my comments above).

@php-coder
Copy link
Owner

There is an issue with a current implementation: this filter doesn't get applied to error pages (try to open //test and you see that userId is always empty). I haven't try it yet, but it seems like we should use setDispatcherType() method. We need to bind to REQUEST and ERROR dispatch types.

Yes, it means that we need to put back FilterRegistrationBean back :)

@mukeshk mukeshk force-pushed the gh_1102_include_userid_in_log_messages branch from 0630ebc to bf42149 Compare July 29, 2019 14:55
@mukeshk mukeshk changed the title task(area/logging) : Include userId in log messages by using MDC task : include userId in the log messages by using MDC. Jul 29, 2019
@mukeshk mukeshk force-pushed the gh_1102_include_userid_in_log_messages branch from bf42149 to 94e87d5 Compare July 29, 2019 15:39
@mukeshk
Copy link
Contributor Author

mukeshk commented Jul 29, 2019

I have put the @bean public FilterRegistrationBean userMdcLoggingFilter() {
still for few request user is not recorded.

2019-07-29 20:34:53.224 [user: ] WARN 23944 --- [tp1635327890-33] r.m.web.feature.site.CspController
2019-07-29 20:35:18.972 [user: ] WARN 23944 --- [tp1635327890-31] o.eclipse.jetty.servlet.ServletHandler : //test

Other requests like adding, and remove collection - event - userId is recorded.

php-coder
php-coder approved these changes Jul 29, 2019
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

LGTM!

@php-coder php-coder merged commit 742ca80 into php-coder:master Jul 29, 2019
@php-coder
Copy link
Owner

Things which go through CSP - dont have it.

I've played with it and here is what I found out: it works when report-uri in CSP header matches with a site that is open in a browser. When you run the app locally, report-uri is set to http://127.0.0.1:8080, so if you open this page in a browser you will see that userId is logged. But it won't work if you open http://locahost:8080.

@php-coder
Copy link
Owner

There is an issue with a current implementation: this filter doesn't get applied to error pages (try to open //test and you see that userId is always empty). I haven't try it yet, but it seems like we should use setDispatcherType() method. We need to bind to REQUEST and ERROR dispatch types.

I've created an issue for tracking it: #1106

@php-coder
Copy link
Owner

@mukeshk Good work, thank you! 👍

@mukeshk mukeshk deleted the gh_1102_include_userid_in_log_messages branch July 30, 2019 05:03
@mukeshk mukeshk restored the gh_1102_include_userid_in_log_messages branch July 30, 2019 05:03
@mukeshk mukeshk deleted the gh_1102_include_userid_in_log_messages branch July 30, 2019 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@php-coder php-coder php-coder approved these changes

Assignees
No one assigned
Labels
resolution/merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mukeshk @mystamps-bot @php-coder