From 27e05af6cb1e1ebca1fd6e8038da290f42db2bdb Mon Sep 17 00:00:00 2001 From: Jeroen van Baarsen Date: Tue, 12 Nov 2024 15:44:26 +0100 Subject: [PATCH 1/5] Add default_attributes to Logger This adds the ability to add default attributes to a logger object. One usecase for this: You create a Logger, for which you already know a specific attribute, you can now set it on the initialize instead of having to pass it around all the time. --- .changesets/add-default-attributes-to-logger.md | 6 ++++++ lib/appsignal/logger.rb | 7 ++++--- spec/lib/appsignal/logger_spec.rb | 10 ++++++++++ 3 files changed, 20 insertions(+), 3 deletions(-) create mode 100644 .changesets/add-default-attributes-to-logger.md diff --git a/.changesets/add-default-attributes-to-logger.md b/.changesets/add-default-attributes-to-logger.md new file mode 100644 index 000000000..f5a0e8d5c --- /dev/null +++ b/.changesets/add-default-attributes-to-logger.md @@ -0,0 +1,6 @@ +--- +bump: patch +type: add +--- + +Add default attributes to Logger diff --git a/lib/appsignal/logger.rb b/lib/appsignal/logger.rb index d882df21e..094321976 100644 --- a/lib/appsignal/logger.rb +++ b/lib/appsignal/logger.rb @@ -20,20 +20,21 @@ class Logger < ::Logger FATAL => 7 }.freeze - attr_reader :level + attr_reader :level, :default_attributes # Create a new logger instance # # @param group Name of the group for this logger. # @param level Log level to filter with # @return [void] - def initialize(group, level: INFO, format: PLAINTEXT) + def initialize(group, level: INFO, format: PLAINTEXT, default_attributes: {}) raise TypeError, "group must be a string" unless group.is_a? String @group = group @level = level @format = format @mutex = Mutex.new + @default_attributes = default_attributes end # We support the various methods in the Ruby @@ -157,7 +158,7 @@ def silence(_severity = ERROR, &block) private def add_with_attributes(severity, message, group, attributes) - Thread.current[:appsignal_logger_attributes] = attributes + Thread.current[:appsignal_logger_attributes] = default_attributes.merge!(attributes) add(severity, message, group) ensure Thread.current[:appsignal_logger_attributes] = nil diff --git a/spec/lib/appsignal/logger_spec.rb b/spec/lib/appsignal/logger_spec.rb index 8f0861f4f..f65ac7a3d 100644 --- a/spec/lib/appsignal/logger_spec.rb +++ b/spec/lib/appsignal/logger_spec.rb @@ -182,6 +182,16 @@ end end + describe "a logger with default attributes" do + it "adds the attributes when a message is logged" do + logger = Appsignal::Logger.new("group", :default_attributes => { :some_key => "some_value" }) + + expect(Appsignal::Extension).to receive(:log).with("group", 6, 0, "Some message", + Appsignal::Utils::Data.generate({ :other_key => "other_value", :some_key => "some_value" })) + logger.error("Some message", { :other_key => "other_value" }) + end + end + describe "#error with exception object" do it "logs the exception class and its message" do error = From 6244731047c72f7cfcc00c6d83f1585d7e783e56 Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:28:56 +0100 Subject: [PATCH 2/5] Rename `default_attributes` parameter Use `attributes` instead of `default_attributes`. --- lib/appsignal/logger.rb | 12 ++++++++---- spec/lib/appsignal/logger_spec.rb | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/appsignal/logger.rb b/lib/appsignal/logger.rb index 094321976..71dc3b5b6 100644 --- a/lib/appsignal/logger.rb +++ b/lib/appsignal/logger.rb @@ -20,21 +20,23 @@ class Logger < ::Logger FATAL => 7 }.freeze - attr_reader :level, :default_attributes + attr_reader :level # Create a new logger instance # # @param group Name of the group for this logger. - # @param level Log level to filter with + # @param level Minimum log level to report. Log lines below this level will be ignored. + # @param format Format to use to parse log line attributes. + # @param attributes Default attributes for all log lines. # @return [void] - def initialize(group, level: INFO, format: PLAINTEXT, default_attributes: {}) + def initialize(group, level: INFO, format: PLAINTEXT, attributes: {}) raise TypeError, "group must be a string" unless group.is_a? String @group = group @level = level @format = format @mutex = Mutex.new - @default_attributes = default_attributes + @default_attributes = attributes end # We support the various methods in the Ruby @@ -157,6 +159,8 @@ def silence(_severity = ERROR, &block) private + attr_reader :default_attributes + def add_with_attributes(severity, message, group, attributes) Thread.current[:appsignal_logger_attributes] = default_attributes.merge!(attributes) add(severity, message, group) diff --git a/spec/lib/appsignal/logger_spec.rb b/spec/lib/appsignal/logger_spec.rb index f65ac7a3d..22121d290 100644 --- a/spec/lib/appsignal/logger_spec.rb +++ b/spec/lib/appsignal/logger_spec.rb @@ -184,7 +184,7 @@ describe "a logger with default attributes" do it "adds the attributes when a message is logged" do - logger = Appsignal::Logger.new("group", :default_attributes => { :some_key => "some_value" }) + logger = Appsignal::Logger.new("group", :attributes => { :some_key => "some_value" }) expect(Appsignal::Extension).to receive(:log).with("group", 6, 0, "Some message", Appsignal::Utils::Data.generate({ :other_key => "other_value", :some_key => "some_value" })) From ae5b83be2fb36ba699955524392f509a1f83c2ca Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:30:07 +0100 Subject: [PATCH 3/5] Ensure the original hashes are not modified The customer might pass a hash that is used someplace else in their application. We should not modify the given hash. (We just saw this issue with appsignal/appsignal-javascript#639) --- lib/appsignal/logger.rb | 2 +- spec/lib/appsignal/logger_spec.rb | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/appsignal/logger.rb b/lib/appsignal/logger.rb index 71dc3b5b6..a0dc2fb4f 100644 --- a/lib/appsignal/logger.rb +++ b/lib/appsignal/logger.rb @@ -162,7 +162,7 @@ def silence(_severity = ERROR, &block) attr_reader :default_attributes def add_with_attributes(severity, message, group, attributes) - Thread.current[:appsignal_logger_attributes] = default_attributes.merge!(attributes) + Thread.current[:appsignal_logger_attributes] = default_attributes.merge(attributes) add(severity, message, group) ensure Thread.current[:appsignal_logger_attributes] = nil diff --git a/spec/lib/appsignal/logger_spec.rb b/spec/lib/appsignal/logger_spec.rb index 22121d290..752b8b860 100644 --- a/spec/lib/appsignal/logger_spec.rb +++ b/spec/lib/appsignal/logger_spec.rb @@ -190,6 +190,17 @@ Appsignal::Utils::Data.generate({ :other_key => "other_value", :some_key => "some_value" })) logger.error("Some message", { :other_key => "other_value" }) end + + it "does not modify the original attribute hashes passed" do + default_attributes = { :some_key => "some_value" } + logger = Appsignal::Logger.new("group", :attributes => default_attributes) + + line_attributes = { :other_key => "other_value" } + logger.error("Some message", line_attributes) + + expect(default_attributes).to eq({ :some_key => "some_value" }) + expect(line_attributes).to eq({ :other_key => "other_value" }) + end end describe "#error with exception object" do From 8be7c7912eea4d3df74f1ca8808b7d6e8802f550 Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:35:14 +0100 Subject: [PATCH 4/5] Ensure the log line attributes take priority This commit does not change the implementation, which already did this, but writes a test to ensure this behaviour and specifies it in the changelog. --- .changesets/add-default-attributes-to-logger.md | 8 +++++++- spec/lib/appsignal/logger_spec.rb | 9 +++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/.changesets/add-default-attributes-to-logger.md b/.changesets/add-default-attributes-to-logger.md index f5a0e8d5c..fa0e4df62 100644 --- a/.changesets/add-default-attributes-to-logger.md +++ b/.changesets/add-default-attributes-to-logger.md @@ -3,4 +3,10 @@ bump: patch type: add --- -Add default attributes to Logger +Allow for default attributes to be given when initialising a `Logger` instance: + +```ruby +order_logger = Appsignal::Logger.new("app", attributes: { order_id: 123 }) +``` + +All log lines reported by this logger will contain the given attribute. Attributes given when reporting the log line will be merged with the default attributes for the logger, with those in the log line taking priority. diff --git a/spec/lib/appsignal/logger_spec.rb b/spec/lib/appsignal/logger_spec.rb index 752b8b860..56af7bdb3 100644 --- a/spec/lib/appsignal/logger_spec.rb +++ b/spec/lib/appsignal/logger_spec.rb @@ -201,6 +201,15 @@ expect(default_attributes).to eq({ :some_key => "some_value" }) expect(line_attributes).to eq({ :other_key => "other_value" }) end + + it "prioritises line attributes over default attributes" do + logger = Appsignal::Logger.new("group", :attributes => { :some_key => "some_value" }) + + expect(Appsignal::Extension).to receive(:log).with("group", 6, 0, "Some message", + Appsignal::Utils::Data.generate({ :some_key => "other_value" })) + + logger.error("Some message", { :some_key => "other_value" }) + end end describe "#error with exception object" do From ce40e8273dd08f54d6a7b82ca4574de63645c0ea Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:54:30 +0100 Subject: [PATCH 5/5] Fix `format` argument in JRuby Logging from JRuby seems to have been subtly broken since the format argument was [added for the C extension, but not for JRuby][commit]. One of the tests added in the previous commits triggered this. [commit]: https://github.com/appsignal/appsignal-ruby/pull/921 --- lib/appsignal/extension/jruby.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/appsignal/extension/jruby.rb b/lib/appsignal/extension/jruby.rb index 0da83ef3e..b97432d8a 100644 --- a/lib/appsignal/extension/jruby.rb +++ b/lib/appsignal/extension/jruby.rb @@ -80,7 +80,7 @@ def self.lib_extension # Logging methods attach_function :appsignal_log, - [:appsignal_string, :int32, :appsignal_string, :pointer], + [:appsignal_string, :int32, :int32, :appsignal_string, :pointer], :void # Transaction methods @@ -273,10 +273,11 @@ def get_server_state(key) make_ruby_string state if state[:len] > 0 end - def log(group, level, message, attributes) + def log(group, level, format, message, attributes) appsignal_log( make_appsignal_string(group), level, + format, make_appsignal_string(message), attributes.pointer )