-
-
Notifications
You must be signed in to change notification settings - Fork 136
Add child logger with instance named tags support #321
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,7 @@ def self.included(base) | |
|
|
||
| # Returns [SemanticLogger::Logger] class level logger | ||
| def self.logger | ||
| @semantic_logger ||= SemanticLogger[self] | ||
| @semantic_logger ||= SemanticLogger[self].child(**@logger_child_named_tags) | ||
|
||
| end | ||
|
|
||
| # Replace instance class level logger | ||
|
|
@@ -114,6 +114,10 @@ def #{method_name}(*args, &block) | |
| true | ||
| end | ||
|
|
||
| def logger_child(**named_tags) | ||
| @logger_child_named_tags = named_tags | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # Dynamic Module to intercept method calls for measuring purposes. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -153,5 +153,18 @@ def self.call(log) | |
| end | ||
| end | ||
| end | ||
|
|
||
| describe ".child" do | ||
| it "creates a child logger" do | ||
|
||
| tag_data = {tag1: "value1", tag2: "value2"} | ||
| child_logger = logger.child(**tag_data) | ||
| child_logger.info("hello world") | ||
|
|
||
| assert_equal tag_data, child_logger.child_named_tags | ||
| assert log = child_logger.events.first | ||
| assert_equal "hello world", log.message | ||
| assert_equal tag_data, log.payload | ||
| end | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be merging
log.tags?https://github.com/reidmorrison/semantic_logger/blob/master/lib/semantic_logger/log.rb#L19-L20
https://github.com/reidmorrison/semantic_logger/blob/master/lib/semantic_logger/log.rb#L28-L29
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you are saying. Let me think about this a bit, I did not use the original
tagsbecause they come from threads, so I used a separate tag attribute (child_named_tags). I also didn't introduce support for "unnamed tags" in the child tags because I thought the support for "unnamed tags" was to mainly stay compatible with railstaggedmethod, but forchildis not really necessaryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK after reading your comments and going through the PR, I understood the codebase a lot more. I made a change (in a separate commit) that overhauls the approach quite heavily. It touches more files, but I think it does everything in a better way.
I think I need to add a test for the formatters using the named tags, and cover some TODO items.
The additional commit will be squashed if this approach makes more sense.
What do you think?