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

TECH-12593:make global_context global, but just for that logger instance #72

Merged
merged 34 commits into from
Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
94f7f24
TECH-12593: store global context as a true global; default to that wh…
ColinDKelley Aug 30, 2023
501d802
TECH-12593: change ContextualLogger to use either Context or Context:…
ColinDKelley Aug 30, 2023
dcb2dcc
TECH-12593: move global_context from process-global to logger-global
ColinDKelley Aug 31, 2023
fff5444
TECH-12593: store global_context in logger; mix Context into LoggerMi…
ColinDKelley Aug 31, 2023
6c23619
TECH-12593: add spec to show that global_context and current_context …
ColinDKelley Aug 31, 2023
de98517
TECH-12593: separate Context from ContextHandler
ColinDKelley Aug 31, 2023
2203516
TECH-12593: bundle update
ColinDKelley Aug 31, 2023
e0f49aa
TECH-12593: add blank line
ColinDKelley Aug 31, 2023
0905e49
TECH-12593: simplify current_context to return nil and current_contex…
ColinDKelley Aug 31, 2023
27b2624
TECH-12593: add spec for .freeze
ColinDKelley Aug 31, 2023
4d3798e
TECH-12593: update CHANGELOG; prep for v1.2.0
ColinDKelley Aug 31, 2023
61552be
TECH-12593: fix LoggerWithContext to delegate global_context
ColinDKelley Aug 31, 2023
5f9f434
TECH-12593: bump version
ColinDKelley Aug 31, 2023
ccd6b3a
TECH-12593: rename [current_context, current_context_for_thread] -> […
ColinDKelley Sep 1, 2023
42446f0
TECH-12593: bump version
ColinDKelley Sep 1, 2023
690d464
TECH-12593: alias current_context_for_thread
ColinDKelley Sep 2, 2023
461c886
TECH-12593: bump version
ColinDKelley Sep 2, 2023
c620d05
use .to change
ColinDKelley Sep 6, 2023
3f514af
TECH-12593: restore code that used context_handler after yield
ColinDKelley Sep 8, 2023
7d1386e
TECH-12593: optimize to skip merge! when empty; test deep merge with …
ColinDKelley Sep 8, 2023
81b2f83
TECH-12593: use ** instead of merge!; keep @deep_merged_context_cache
ColinDKelley Sep 11, 2023
b2d7ba5
TECH-12593: limit @deep_merged_context_cache to 100 entries
ColinDKelley Sep 11, 2023
d3a6971
TECH-12593: remove caching optimizations; add described_class.global_…
ColinDKelley Sep 11, 2023
f2db080
TECH-12593: document Context precedence; refactor LoggerWithContext t…
ColinDKelley Sep 12, 2023
0f92823
TECH-12593: update CHANGELOG
ColinDKelley Sep 12, 2023
6ff10fd
TECH-12593: bump version
ColinDKelley Sep 12, 2023
524c499
TECH-12593: add spec for ContextualLogger::GlobalContextIsLocked
ColinDKelley Sep 12, 2023
7c29ec9
TECH-12593: add trailing newline
ColinDKelley Sep 12, 2023
d4cbaa4
TECH-12593: args -> _args
ColinDKelley Sep 12, 2023
70a4740
TECH-12593: drop redundant self
ColinDKelley Sep 12, 2023
e55ebd2
TECH-12593: remove extra newline
ColinDKelley Sep 12, 2023
80992ab
TECH-12593: move setting ContextualLogger.global_context_lock_message…
ColinDKelley Sep 12, 2023
8597b5c
TECH-12593: bump version
ColinDKelley Sep 12, 2023
1c85c7b
TECH-12593: prep to release v1.2.0
ColinDKelley Sep 14, 2023
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
17 changes: 5 additions & 12 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ 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).

## [1.2.0] - Unreleased
### Changed
- Changed global_context to be stored inside the logger.
- Changed current_context to be stored in a Thread/Fiber-local variable that is unique per instance of the logger.

## [1.1.1] - 2022-11-29
### Added
- Bug fix for registering regex
Expand Down Expand Up @@ -99,15 +104,3 @@ are already passed to the formatter as arguments so that the formatter can decid
### Added
- Extracted `ContextualLogger.normalize_log_level` into a public class method so we can call it elsewhere where we allow log_level to be
configured to text values like 'debug'.

[1.0.0]: https://github.com/Invoca/contextual_logger/compare/v0.11.0...v1.0.0
[0.11.0]: https://github.com/Invoca/contextual_logger/compare/v0.10.0...v0.11.0
[0.10.0]: https://github.com/Invoca/contextual_logger/compare/v0.9.1...v0.10.0
[0.9.1]: https://github.com/Invoca/contextual_logger/compare/v0.9.0...v0.9.1
[0.9.0]: https://github.com/Invoca/contextual_logger/compare/v0.8.0...v0.9.0
[0.8.0]: https://github.com/Invoca/contextual_logger/compare/v0.7.0...v0.8.0
[0.7.0]: https://github.com/Invoca/contextual_logger/compare/v0.6.1...v0.7.0
[0.6.1]: https://github.com/Invoca/contextual_logger/compare/v0.6.0...v0.6.1
[0.6.0]: https://github.com/Invoca/contextual_logger/compare/v0.5.1...v0.6.0
[0.5.1]: https://github.com/Invoca/contextual_logger/compare/v0.5.0...v0.5.1
[0.5.0]: https://github.com/Invoca/contextual_logger/compare/v0.4.0...v0.5.0
24 changes: 13 additions & 11 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
PATH
remote: .
specs:
contextual_logger (1.1.1)
contextual_logger (1.2.0.colin.4)
activesupport
json

GEM
remote: https://rubygems.org/
specs:
activesupport (7.0.4.3)
activesupport (7.0.7.2)
concurrent-ruby (~> 1.0, >= 1.0.2)
i18n (>= 1.6, < 2)
minitest (>= 5.1)
tzinfo (~> 2.0)
appraisal (2.4.1)
appraisal (2.5.0)
bundler
rake
thor (>= 0.14.0)
Expand All @@ -29,18 +29,20 @@ GEM
tins (~> 1.6)
diff-lcs (1.5.0)
docile (1.4.0)
i18n (1.12.0)
i18n (1.14.1)
concurrent-ruby (~> 1.0)
json (2.6.3)
method_source (1.0.0)
minitest (5.18.0)
minitest (5.19.0)
parallel (1.23.0)
parser (3.2.2.0)
parser (3.2.2.3)
ast (~> 2.4.1)
racc
powerpack (0.1.3)
pry (0.14.2)
coderay (~> 1.1)
method_source (~> 1.0)
racc (1.7.1)
rainbow (3.1.1)
rake (13.0.6)
rspec (3.12.0)
Expand All @@ -49,13 +51,13 @@ GEM
rspec-mocks (~> 3.12.0)
rspec-core (3.12.2)
rspec-support (~> 3.12.0)
rspec-expectations (3.12.2)
rspec-expectations (3.12.3)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.12.0)
rspec-mocks (3.12.5)
rspec-mocks (3.12.6)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.12.0)
rspec-support (3.12.0)
rspec-support (3.12.1)
rspec_junit_formatter (0.6.0)
rspec-core (>= 2, < 4, != 2.12.0)
rubocop (0.54.0)
Expand All @@ -79,7 +81,7 @@ GEM
sync (0.5.0)
term-ansicolor (1.7.1)
tins (~> 1.0)
thor (1.2.1)
thor (1.2.2)
tins (1.32.1)
sync
tzinfo (2.0.6)
Expand All @@ -105,4 +107,4 @@ DEPENDENCIES
ruby-prof-flamegraph

BUNDLED WITH
2.2.29
2.3.22
4 changes: 2 additions & 2 deletions contextual_logger.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ Gem::Specification.new do |spec|
"allowed_push_host" => "https://rubygems.org"
}

spec.add_runtime_dependency 'json'
spec.add_runtime_dependency 'activesupport'
spec.add_dependency 'json'
spec.add_dependency 'activesupport'
end
34 changes: 22 additions & 12 deletions lib/contextual_logger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
require 'active_support/core_ext/module/delegation'
require 'json'
require_relative './contextual_logger/redactor'
require_relative './contextual_logger/context/handler'
require_relative './contextual_logger/context'
require_relative './contextual_logger/context_handler'

module ContextualLogger
LOG_LEVEL_NAMES_TO_SEVERITY =
Expand Down Expand Up @@ -43,31 +44,40 @@ def normalize_message(message)
end

module LoggerMixin
include Context

delegate :register_secret, :register_secret_regex, to: :redactor

def global_context
@global_context ||= {}.freeze
end

def global_context=(context)
Context::Handler.new(context).set!
@global_context = context.freeze
jebentier marked this conversation as resolved.
Show resolved Hide resolved
end

def current_context
current_context_override || global_context
end

# TODO: Deprecate current_context_for_thread in v2.0.
alias current_context_for_thread current_context

def with_context(context)
context_handler = Context::Handler.new(current_context_for_thread.deep_merge(context))
context_handler.set!
previous_context_override = current_context_override
self.current_context_override = current_context.deep_merge(context)
jebentier marked this conversation as resolved.
Show resolved Hide resolved
if block_given?
begin
yield
ensure
context_handler.reset!
self.current_context_override = previous_context_override
ColinDKelley marked this conversation as resolved.
Show resolved Hide resolved
end
else
# If no block given, the context handler is returned to the caller so they can handle reset! themselves.
context_handler
# If no block given, return context handler to the caller so they can call reset! themselves.
ContextHandler.new(self, previous_context_override)
end
end

def current_context_for_thread
Context::Handler.current_context
end

# In the methods generated below, we assume that presence of context means new code that is
# aware of ContextualLogger...and that that code never uses progname.
# This is important because we only get 3 args total (not including &block) passed to `add`,
Expand Down Expand Up @@ -117,7 +127,7 @@ def add(arg_severity, arg1 = nil, arg2 = nil, **context) # Ruby will prefer to
message = arg1
progname = arg2 || @progname
end
write_entry_to_log(severity, Time.now, progname, message, context: current_context_for_thread.deep_merge(context))
write_entry_to_log(severity, Time.now, progname, message, context: current_context.deep_merge(context))
end

true
Expand Down
18 changes: 18 additions & 0 deletions lib/contextual_logger/context.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# frozen_string_literal: true

module ContextualLogger
module Context
def thread_context_for_logger_instance
# We include the object_id here to make these thread/fiber locals unique per logger instance.
@thread_context_for_logger_instance ||= "ContextualLogger::Context.context_for_#{object_id}".to_sym
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pretty sneaky. It's the only way I could think to get Thread/Fiber-locals to also be unique for each enclosing Logger. Notice that they won't get cleaned up when the Logger goes out of scope. I don't think it's likely that anyone would create a huge number of Loggers, so we should be fine.

end

def current_context_override
Thread.current[thread_context_for_logger_instance]
end

def current_context_override=(context_override)
Thread.current[thread_context_for_logger_instance] = context_override.freeze
end
end
end
28 changes: 0 additions & 28 deletions lib/contextual_logger/context/handler.rb

This file was deleted.

14 changes: 14 additions & 0 deletions lib/contextual_logger/context_handler.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# frozen_string_literal: true

module ContextualLogger
class ContextHandler
def initialize(instance, previous_context_override)
@instance = instance
@previous_context_override = previous_context_override
end

def reset!
@instance.current_context_override = @previous_context_override
end
end
end
4 changes: 4 additions & 0 deletions lib/contextual_logger/logger_with_context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ def initialize(logger, context, level: nil)
@merged_context_cache = {} # so we don't have to merge every time
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jebentier This is still a bug. I realized it's one of the reasons I had headed down the path of tracking the global_context instance for equals?--that would be necessary in order to fix this bug. But it would be really complicated. And this bug isn't new. The code in this branch works much better in that it handles with_context on both the original logger as well as the LoggerWithContext; it just doesn't handle if you repeat the former after having logged something. That seems very unlikely.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that this seems unlikely and that the code required to fix this limitation is more complex than we want for this logger. Thank you for documenting it so we’re aware moving forward.

def global_context
@logger.global_context
end

def level
@override_level || @logger.level
end
Expand Down
2 changes: 1 addition & 1 deletion lib/contextual_logger/version.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# frozen_string_literal: true

module ContextualLogger
VERSION = '1.1.1'
VERSION = '1.2.0.colin.4'
end
32 changes: 0 additions & 32 deletions spec/lib/contextual_logger/context/handler_spec.rb

This file was deleted.

25 changes: 25 additions & 0 deletions spec/lib/contextual_logger/context_handler_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# frozen_string_literal: true

require 'spec_helper'
require 'contextual_logger'

class ContextualLoggerContextHandlerSpecContainer
include ::ContextualLogger::Context
end

RSpec.describe ContextualLogger::ContextHandler do
let(:context) { { service: { name: 'tts', description: 'TTS' }, integration: "google" } }
let(:context2) { { service: { description: 'Context 2' }, integration: "google" } }

let(:instance) { ContextualLoggerContextHandlerSpecContainer.new }

subject(:handler) { described_class.new(instance, context) }

it { is_expected.to respond_to(:reset!) }

describe '#reset!'
before { instance.current_context_override = context2 }

it { expect { hander.reset! }.to change(instance, :current_context_override).from(context2).to(context) }
end
end
66 changes: 66 additions & 0 deletions spec/lib/contextual_logger/context_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# frozen_string_literal: true

require 'spec_helper'
require 'contextual_logger'

class ContextualLoggerContextSpecContainer
include ::ContextualLogger::Context
end

RSpec.describe ContextualLogger::Context do
let(:context) { { service: { name: 'tts', description: 'TTS' }, integration: "google" } }
let(:context2) { { service: { description: 'Context 2' }, integration: "google" } }
let(:context3) { { service: { name: 'Context 3' } } }

let(:instance) { ContextualLoggerContextSpecContainer.new }

describe 'mixin methods' do
describe 'current_context_override/current_context_override=' do
it 'can be set with current_context_override=, separately per Fiber/Thread' do
instance.current_context_override = context

thread =
Thread.new do
instance.current_context_override = context2
instance.current_context_override
end

fiber =
Fiber.new do
instance.current_context_override = context3
instance.current_context_override
end

fiber_override = fiber.resume
thread_override = thread.value

expect(instance.current_context_override).to eq(context)
expect(thread_override).to eq(context2)
expect(fiber_override).to eq(context3)
end

it 'the current_context_override= values are separate per containing instance' do
instance.current_context_override = context
instance2 = ContextualLoggerContextSpecContainer.new
instance2.current_context_override = context2

expect(instance.current_context_override).to eq(context)
expect(instance2.current_context_override).to eq(context2)
end

it 'freezes the context_override when set' do
instance.current_context_override = {}

expect do
instance.current_context_override[:extra] = 'value'
end.to raise_exception(RuntimeError, /can't modify frozen/)
end

it 'returns nil when set to nil' do
instance.current_context_override = nil

expect(instance.current_context_override).to be_nil
end
end
end
end
Loading
Loading