Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# CHANGELOG for `contextual_logger`

Inspired by [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

Note: this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [0.5.1] - 2019-03-10 [diff](https://github.com/Invoca/contextual_logger/compare/v0.5.0...0.5.1)

### Changed

- Refactored debug, info, error... etc methods to call the base class `add(severity, message, progrname)` method since
ActiveSupport::Logger.broadcast reimplements that to broadcast to multiple logger instances, such as
Rails::Server logging to `STDOUT` + `development.log`.
Note that the base class `add()` does not have a `context` hash like our `add()` does.
We use the `**` splat to match the `context` hash up to the extra
`**context` argument, if present. If that argument is not present (such as with `broadcast`), Ruby will instead
match the `**context` up to the `progname` argument.

## [0.5] - 2019-03-06 [diff](https://github.com/Invoca/contextual_logger/compare/v0.4.0...v0.5.0)

Choose a reason for hiding this comment

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

keepachangelog.com shows a neat way of doing this in their CHANGELOG... It would look like this:

Suggested change
## [0.5] - 2019-03-06 [diff](https://github.com/Invoca/contextual_logger/compare/v0.4.0...v0.5.0)
## [0.5] - 2019-03-06
### Added
- Extracted normalize_log_level to a public class method so we can call it elsewhere where we allow log_level to be
configured to text values like 'debug'.
[0.5]: https://github.com/Invoca/contextual_logger/compare/v0.4.0...v0.5.0

i.e. It allows for in-line more readable lines that reference to defined links at the bottom of the file.

Choose a reason for hiding this comment

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

Wouldn't we prefer the version to be [0.5.0] here?

Copy link
Contributor Author

@ColinDKelley ColinDKelley Mar 10, 2020

Choose a reason for hiding this comment

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

Yes, while they're equivalent, I agree that the extra .0 is more clear... so I've added it.

Copy link
Contributor Author

@ColinDKelley ColinDKelley Mar 10, 2020

Choose a reason for hiding this comment

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

i.e. It allows for in-line more readable lines that reference to defined links at the bottom of the file.

I thought diff was pretty readable? No one needs to see the complete github URL--that's irrelevant. And scrolling to the bottom seems like a problem for discoverability. I didn't discover it in their example! And for their github example, it looks like they buried the diff in a hyperlink of the version number. I tried that first, but rejected it for poor discoverability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah i think I might know where confusion is coming in here. The original spec is using reference style linking for the version headers. By making the headers the version number with surrounding brackets (ex. [0.0.1]), it’s using link reference syntax to turn that version number into a link by defining the link at the bottom of the file with [0.0.1]: https//.... The references at the bottom of the file are not rendered by markdown. This exists to help with readability when viewing the actual markdown, like you would when pulling the repo locally and reading.

You can see this in action when you look at the raw markdown of the spec example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok, I didn't know about that notation. That explains why the links are at the bottom of the source. But that seems needlessly complex to manage (add your new version to the top of the file, but don't forget to go near the bottom to add the supporting reference links... and make sure the version numbers are named exactly the same [see 0.5 vs. 0.5.0] so markdown can join them up).
And this still has the same issues around discoverability. I wouldn't expect the version number to link to a raw github diff. The very point of the CHANGELOG is to summarize the changes so you don't have to look at the raw diff! It should be very rare that you'd want to drill into the diff.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But that seems needlessly complex to manage

It's also dry. If for any reason there is the need to reference an older version, and you'd like to specifically link to it, you simply add brackets around the version number and this turns it into the same link.

and make sure the version numbers are named exactly the same [see 0.5 vs. 0.5.0]

The versions in the headers should be following semver, so it shouldn't be too much to keep them in sync. In semver 0.5 and 0.5.0 do actually mean different things. 0.5.0 is an exact version, and 0.5 is a pseudo version meant to reference the latest stable version under 0.5. So if 0.5.1 was released, 0.5 would point semantically to 0.5.1 not 0.5.0. This is why the spec uses the full versions and not pseudo versions.

The very point of the CHANGELOG is to summarize the changes so you don't have to look at the raw diff

I completely agree. And that's exactly why the diff is added as a hyperlink and pushed out of the way in the markdown. The diff is unimportant and supplementary if anything. So it's made sure to be completely out of the way when reading the rendered markdown (through the use of a link), or reading the raw markdown (by using link reference notation).

Copy link
Contributor Author

@ColinDKelley ColinDKelley Mar 12, 2020

Choose a reason for hiding this comment

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

It's also dry. If for any reason there is the need to reference an older version, and you'd like to specifically link to it, you simply add brackets around the version number and this turns it into the same link.

Nah, you can't count hypothetical future cases as a DRY plus today. (YAGNI) Today it's actually less DRY, because you need to repeat the version numbers in order for the join to work. For example, when I renamed 0.5 to 0.5.0 it would have been easy to not remember to go to the bottom of the file and rename there too.

0.5 is a pseudo version meant to reference

I didn't realize that "pseudo versions" were a formal concept. I know about those in the Gemfile version matchers like ~> 0.5, but I was under the impression that 0.5 and 0.5.0 were exactly the same version. Do you have any references you can point me to about "pseudo-versions" in Ruby?

Doesn't this apply one level deeper? Aren't 0.5.1.0 and 0.5.1 the same version?

I don't think you addressed my "discoverability" comment. How would you expect a reader to know that the version hyperlink goes to a raw github diff? I expected it to go to exactly that version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah, you can't count hypothetical future cases as a DRY plus today.
I'm not arguing DRY for the sake of this implementation. I'm stating it as the reasoning for this standard with 76 contributors and used by many many open source projects being the way it is.

I didn't realize that "pseudo versions" were a formal concept
I haven't found any specific documentation for ruby, but I don't know why ruby matters. Semver is for all languages. If you look at Docker, releases work the same way for semver versioning of images. Pulling the ruby:2.5 image will get you the latest patch version of version 2.5. Same with python, ubuntu, etc. The open source community is standardizing around this concept.

All and all, I think that we should follow the standard. If we done, you should remove the reference to the standard from the Changelog. It's more confusing to say "We follow X standard" and then change it.


### Added
- Extracted normalize_log_level to a public class method so we can call it elsewhere where we allow log_level to be
configured to text values like 'debug'.
2 changes: 1 addition & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
contextual_logger (0.5.0)
contextual_logger (0.5.1)
activesupport
json

Expand Down
3 changes: 1 addition & 2 deletions contextual_logger.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@

Gem::Specification.new do |spec|
spec.name = 'contextual_logger'
spec.version = '0.5.0'
spec.version = '0.5.1'
spec.license = 'MIT'
spec.date = '2018-10-12'
spec.summary = 'Add context to your logger'
spec.description = 'A way to add context to the logs you have'
spec.authors = ['James Ebentier']
Expand Down
28 changes: 19 additions & 9 deletions lib/contextual_logger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,38 +59,48 @@ def current_context_for_thread
end

def debug(message = nil, context = {})
add_if_enabled(Logger::Severity::DEBUG, message || yield, context: context)
add(Logger::Severity::DEBUG, message || yield, **context)
end

def info(message = nil, context = {})
add_if_enabled(Logger::Severity::INFO, message || yield, context: context)
add(Logger::Severity::INFO, message || yield, **context)
end

def warn(message = nil, context = {})
add_if_enabled(Logger::Severity::WARN, message || yield, context: context)
add(Logger::Severity::WARN, message || yield, **context)
end

def error(message = nil, context = {})
add_if_enabled(Logger::Severity::ERROR, message || yield, context: context)
add(Logger::Severity::ERROR, message || yield, **context)
end

def fatal(message = nil, context = {})
add_if_enabled(Logger::Severity::FATAL, message || yield, context: context)
add(Logger::Severity::FATAL, message || yield, **context)
end

def unknown(message = nil, context = {})
add_if_enabled(Logger::Severity::UNKNOWN, message || yield, context: context)
add(Logger::Severity::UNKNOWN, message || yield, **context)
end

def log_level_enabled?(severity)
severity >= level
end

def add_if_enabled(severity, message, context:)
def add(init_severity, message = nil, init_progname = nil, **context) # Ruby will prefer to match hashes up to last ** argument
severity = init_severity || UNKNOWN
if log_level_enabled?(severity)
@progname ||= nil
write_entry_to_log(severity, Time.now, @progname, message, context: current_context_for_thread.deep_merge(context))
progname = init_progname || @progname
if message.nil?
if block_given?
message = yield
else
message = init_progname
progname = @progname
end
end
write_entry_to_log(severity, Time.now, progname, message, context: current_context_for_thread.deep_merge(context))
end

true
end

Expand Down
28 changes: 28 additions & 0 deletions spec/lib/contextual_logger_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,34 @@ def expect_log_line_to_be_written(log_line)
expect(log_message_levels).to eq(["unknown"])
end
end

describe 'when used with ActiveSupport::Logger.broadcast' do
let(:log_level) { Logger::Severity::ERROR }
let(:console_log_stream) { StringIO.new }
let(:console_logger) { Logger.new(console_log_stream) }
let(:broadcast_logger) { logger.extend(ActiveSupport::Logger.broadcast(console_logger)) }

it 'properly broadcasts to both logs when level-named method is called' do
log_at_every_level(broadcast_logger, service: 'test_service')
expect(log_message_levels).to eq(["error", "fatal", "unknown"])
# note: context lands in `progname` arg
expect(console_log_stream.string.gsub(/\[[^]]+\]/, '[]')).to eq(<<~EOS)
D, [] DEBUG -- {:service=>\"test_service\"}: debug message
I, [] INFO -- {:service=>\"test_service\"}: info message
W, [] WARN -- {:service=>\"test_service\"}: warn message
E, [] ERROR -- {:service=>\"test_service\"}: error message
F, [] FATAL -- {:service=>\"test_service\"}: fatal message
A, [] ANY -- {:service=>\"test_service\"}: unknown message
EOS
end

it 'properly broadcasts to both logs when add is called' do
broadcast_logger.add(Logger::Severity::ERROR, "error message", service: 'test_service')
expect(log_message_levels).to eq(["error"])
# note: context lands in `progname` arg
expect(console_log_stream.string.gsub(/\[[^]]+\]/, '[]')).to eq("E, [] ERROR -- {:service=>\"test_service\"}: error message\n")
end
end
end

describe 'inline context' do
Expand Down
14 changes: 7 additions & 7 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
Coveralls.wear!

module Helpers
def log_at_every_level(logger)
logger.debug("debug message")
logger.info("info message")
logger.warn("warn message")
logger.error("error message")
logger.fatal("fatal message")
logger.unknown("unknown message")
def log_at_every_level(logger, context = {})
logger.debug("debug message", context)
logger.info("info message", context)
logger.warn("warn message", context)
logger.error("error message", context)
logger.fatal("fatal message", context)
logger.unknown("unknown message", context)
end

def log_message_levels
Expand Down