-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add Logging #1291
Add Logging #1291
Changes from all commits
100146f
e5c7969
3ad2197
ac473c7
b0ef4d7
981c229
2a80cdb
67c7db1
e28e2b5
ca4139e
2c4a028
49c8f69
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 |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# Instrumentation | ||
|
||
AMS uses the instrumentation API provided by Active Support this way we | ||
can choose to be notified when AMS events occur inside our application. | ||
|
||
## render.active_model_serializers | ||
|
||
|key | value | | ||
|-------------|----------------------| | ||
|:serializer | The serializer class | | ||
|:adapter | The adapter instance | | ||
|
||
```ruby | ||
{ | ||
serializer: PostSerializer, | ||
adapter: #<ActiveModel::Serializer::Adapter::Attributes:0x007f96e81eb730> | ||
} | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
# Logging | ||
|
||
If we are using AMS on Rails app by default the `Rails.logger` will be used. | ||
|
||
On a non Rails enviroment by default the `ActiveSupport::TaggedLogging` will be | ||
used. | ||
|
||
If we need to customize the logger we can define this in an initializer: | ||
|
||
```ruby | ||
ActiveModel::Serializer.logger = Logger.new(STDOUT) | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,15 @@ | |
module ActiveModel | ||
class SerializableResource | ||
ADAPTER_OPTION_KEYS = Set.new([:include, :fields, :adapter, :meta, :meta_key, :links]) | ||
extend ActiveModel::Callbacks | ||
|
||
define_model_callbacks :render | ||
|
||
around_render do |_, block, _| | ||
notify_active_support do | ||
block.call | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I think you're right. This doesn't add anything but complexity. Sorry for having you extra work. I meant it as a thought if it made sense.. and it's more indirection than helpful |
||
|
||
# Primary interface to composing a resource with a serializer and adapter. | ||
# @return the serializable_resource, ready for #as_json/#to_json/#serializable_hash. | ||
|
@@ -11,7 +20,23 @@ def initialize(resource, options = {}) | |
options.partition { |k, _| ADAPTER_OPTION_KEYS.include? k }.map { |h| Hash[h] } | ||
end | ||
|
||
delegate :serializable_hash, :as_json, :to_json, to: :adapter | ||
def serializable_hash(*args) | ||
run_callbacks :render do | ||
adapter.serializable_hash(*args) | ||
end | ||
end | ||
|
||
def as_json(*args) | ||
run_callbacks :render do | ||
adapter.as_json(*args) | ||
end | ||
end | ||
|
||
def to_json(*args) | ||
run_callbacks :render do | ||
adapter.to_json(*args) | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. call me crazy, but I think this might be a good use case for an around filter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
def serialization_scope=(scope) | ||
serializer_opts[:scope] = scope | ||
|
@@ -64,5 +89,13 @@ def serializer? | |
protected | ||
|
||
attr_reader :resource, :adapter_opts, :serializer_opts | ||
|
||
def notify_active_support | ||
event_name = 'render.active_model_serializers'.freeze | ||
payload = { serializer: serializer, adapter: adapter } | ||
ActiveSupport::Notifications.instrument(event_name, payload) do | ||
yield | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did you need to use the we should also freeze the string here or reduce object allocations on what would be frequently called method, even if the notifications are turned off. otherwise, 💯 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I made these changes 👍 |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,13 +6,15 @@ | |
require 'active_model/serializer/configuration' | ||
require 'active_model/serializer/fieldset' | ||
require 'active_model/serializer/lint' | ||
require 'active_model/serializer/logging' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be required in lib/active_model_serializers, I think. It's more of a global concern than local to this file. feel free to disagree |
||
|
||
# ActiveModel::Serializer is an abstract class that is | ||
# reified when subclassed to decorate a resource. | ||
module ActiveModel | ||
class Serializer | ||
include Configuration | ||
include Associations | ||
include Logging | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👎 does not belong here |
||
require 'active_model/serializer/adapter' | ||
|
||
# Matches | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
module ActiveModel | ||
class Serializer | ||
module Logging | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm 👎 on adding new namespaces to what I hope to become the legacy namespace, #1310 It either needs to be in lib/active_model_serializers.rb, in lib/active_model_serializers/logging.rb or have 2 👍 by other maintainers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bf4 are you saying everything should be switched to the new namespace before adding anything? wouldn't that only matter depending on the order of merges? waiting on stuff like that causes needless delays, imo but maybe I misunderstand? idk |
||
extend ActiveSupport::Concern | ||
|
||
class LogSubscriber < ActiveSupport::LogSubscriber | ||
def render(event) | ||
logger.tagged('AMS') do | ||
info do | ||
serializer = event.payload[:serializer] | ||
adapter = event.payload[:adapter] | ||
duration = event.duration.round(2) | ||
"Rendered #{serializer.name} with #{adapter.class} (#{duration}ms)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The more work you can do in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not understand this line:
Could you please clarify? |
||
end | ||
end | ||
end | ||
|
||
def logger | ||
ActiveModelSerializers.logger | ||
end | ||
end | ||
end | ||
end | ||
end | ||
|
||
ActiveModel::Serializer::Logging::LogSubscriber.attach_to :active_model_serializers | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be configured in the railtie There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? We still want logging on other environments right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you mean like non-Rails? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, was referring to notifications above. logging should be configured in rails in the railtie. outside rails, manually There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the review, I will update next time I work on the project. I do not understand what you want here? Do you want that I move Thanks |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
require 'test_helper' | ||
|
||
module ActiveModel | ||
class Serializer | ||
class LoggingTest < Minitest::Test | ||
class TestLogger < ActiveSupport::Logger | ||
def initialize | ||
@file = StringIO.new | ||
super(@file) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just use your own logger. that's kind of the point, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so. I know that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I think that makes sense. I would have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re naming, to me
and
|
||
|
||
def messages | ||
@file.rewind | ||
@file.read | ||
end | ||
end | ||
|
||
def setup | ||
@author = Author.new(name: 'Steve K.') | ||
@post = Post.new(title: 'New Post', body: 'Body') | ||
@comment = Comment.new(id: 1, body: 'ZOMG A COMMENT') | ||
@post.comments = [@comment] | ||
@comment.post = @post | ||
@post.author = @author | ||
@author.posts = [@post] | ||
@post_serializer = PostSerializer.new(@post, custom_options: true) | ||
|
||
@old_logger = ActiveModelSerializers.logger | ||
@logger = ActiveSupport::TaggedLogging.new(TestLogger.new) | ||
logger @logger | ||
end | ||
|
||
def teardown | ||
logger @old_logger | ||
end | ||
|
||
def logger(logger) | ||
ActiveModelSerializers.logger = logger | ||
end | ||
|
||
def test_uses_ams_as_tag | ||
ActiveModel::SerializableResource.new(@post).serializable_hash | ||
assert_match(/\[AMS\]/, @logger.messages) | ||
end | ||
|
||
def test_logs_when_call_serializable_hash | ||
ActiveModel::SerializableResource.new(@post).serializable_hash | ||
assert_match(/Rendered/, @logger.messages) | ||
end | ||
|
||
def test_logs_when_call_as_json | ||
ActiveModel::SerializableResource.new(@post).as_json | ||
assert_match(/Rendered/, @logger.messages) | ||
end | ||
|
||
def test_logs_when_call_to_json | ||
ActiveModel::SerializableResource.new(@post).to_json | ||
assert_match(/Rendered/, @logger.messages) | ||
end | ||
|
||
def test_logs_correct_serializer | ||
ActiveModel::SerializableResource.new(@post).serializable_hash | ||
assert_match(/PostSerializer/, @logger.messages) | ||
end | ||
|
||
def test_logs_correct_adapter | ||
ActiveModel::SerializableResource.new(@post).serializable_hash | ||
assert_match(/ActiveModel::Serializer::Adapter::Attributes/, @logger.messages) | ||
end | ||
|
||
def test_logs_the_duration | ||
ActiveModel::SerializableResource.new(@post).serializable_hash | ||
assert_match(/\(\d+\.\d+ms\)/, @logger.messages) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks 😊 |
||
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.
Could you explain why you made that change?
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.
seconded
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.
Take a look on this commit message maurogeorge@981c229
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.
Didn't clear it up for me
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.
The main purpose of this is to the instrumentation be called on the Rails controller, so if I remove, this test will be broken.
As you can see here the instrumentation is made on the
ActiveModel::SerializableResource
what I think is the canonical way to use a serializer and as you can see we simply delegate the method to the adapter. If we call the adapter directly over use theActiveModel::SerializableResource
we will need to change all adapters to call instrumentation on they methodsserializable_hash
,as_json
andto_json
.Keeping the
ActiveModel::SerializableResource
we define this once and all adapter, including new ones, will have instrumentation.Let me know if you have some doubt.