Instrumenting rendering of resources #1322

bf4 · 2015-11-08T21:54:54Z

Building on the very excellent PR by @maurogeorge (all squashed)

bf4 · 2015-11-08T22:12:51Z

lib/active_model/serializer/railtie.rb

-      ActiveSupport.on_load(:action_controller) do
-        ActiveModelSerializers.logger = ActionController::Base.logger
+      ActiveSupport.on_load(:active_model_serializers) do
+        self.logger = ActionController::Base.logger
      end


bonus code, out of scope... but shouldn't be that controversial.. pattern copied from active job

Pretty cool 👍

beauby · 2015-11-08T22:16:52Z

Looks cool 👍

maurogeorge · 2015-11-09T12:44:23Z

lib/active_model_serializers.rb

 require 'active_model'
 require 'active_support'
+require 'active_support/tagged_logging'
+require 'active_support/logger'


I think we can remove these 2 requires, since we already require active_support

So, for this I followed the pattern in active job

It's always nice to be explicit

You're right that it just so happens require 'active_support' requires logging and autoloads TaggedLogging, but there's no guarantee https://github.com/rails/rails/blob/4-0-stable/activesupport/lib/active_support.rb#L27

That said, if you feel strongly I'll take it out

I think if we have tests covering this we are good to go, to remove.

Squashed commits: Add Logging Generates logging when renders a serializer. Tunning performance on notify_active_support - Use yield over block.call - Freeze the event name string Organize the logger architeture * Keep only the `ActiveModel::Serializer.logger` to follow the same public API we have for example to config, like `ActiveModel::Serializer.config.adapter` and remove the `ActiveModelSerializers.logger` API. * Define the logger on the load of the AMS, following the Rails convention on Railties [1], [2] and [3]. This way on non Rails apps we have a default logger and on Rails apps we will use the `Rails.logger` the same way that Active Job do [4]. [1]: https://github.com/rails/rails/blob/2ad9afe4ff2c12d1a9e4a00d9009d040e636c9b0/activejob/lib/active_job/railtie.rb#L9-L11 [2]: https://github.com/rails/rails/blob/2ad9afe4ff2c12d1a9e4a00d9009d040e636c9b0/activerecord/lib/active_record/railtie.rb#L75-L77 [3]: https://github.com/rails/rails/blob/2ad9afe4ff2c12d1a9e4a00d9009d040e636c9b0/actionview/lib/action_view/railtie.rb#L19-L21 [4]: https://github.com/rails/rails/blob/2ad9afe4ff2c12d1a9e4a00d9009d040e636c9b0/activejob/lib/active_job/logging.rb#L10-L11 Performance tunning on LogSubscriber#render Move the definition of locals to inside the `info` block this way the code is executed only when the logger is called. Remove not needed check on SerializableResource Use SerializableResource on ActionController integration On the ActionController was using a adapter, and since the instrumentation is made on the SerializableResource we need to use the SerializableResource over the adapter directly. Otherwise the logger is not called on a Rails app. Use SerializableResource on the ActionController, since this is the main interface to create and call a serializer. Using always the SerializableResource we can keep the adapter code more easy to mantain since no Adapter will need to call the instrumentation, only the SerializableResource care about this. Add docs about logging Add a CHANGELOG entry Keep the ActiveModelSerializers.logger Better wording on Logging docs [ci skip] Add doc about instrumentation [ci skip] Use ActiveModel::Callbacks on the SerializableResource

bf4 · 2015-11-10T09:28:40Z

@maurogeorge updated with two incremental commits responding to your feedback, for easy review

maurogeorge · 2015-11-10T14:09:15Z

@bf4 I am ok with that 👍

NullVoxPopuli · 2015-11-10T14:19:57Z

docs/general/instrumentation.md

@@ -0,0 +1,19 @@
+# Instrumentation
+
+ActiveModelSerializers uses the ActiveSupport::Notification API, which


should there be an example here of how to tie this in to the Notification API?

NullVoxPopuli · 2015-11-10T14:22:55Z

lib/active_model_serializers/logging.rb

+      end
+    end
+
+    # Macros that can be used to customize the logging of class or instance methods,


this is some solid documentation :-)

Copied pretty much :)

B mobile phone

On Nov 10, 2015, at 8:22 AM, L. Preston Sego III notifications@github.com wrote:

In lib/active_model_serializers/logging.rb:

instrument_rendering

end

module ClassMethods

def instrument_rendering

around_render do |args, block|

tag_logger do

notify_render do

block.call(args)

end

end

end

end

end

Macros that can be used to customize the logging of class or instance methods,

this is some solid documentation :-)

—
Reply to this email directly or view it on GitHub.

bf4 · 2015-11-13T13:53:31Z

Appveyour failure is transitory

wagenet · 2015-11-17T22:10:31Z

Is this waiting on anything? Would love to get to work on Skylight integration once this is official :)

bf4 · 2015-11-19T08:12:18Z

@wagenet not sure. Have you tried ~~dogfooding~~ using it? @NullVoxPopuli ok? @maurogeorge ?

Otherwise, I just need to rebase it

maurogeorge · 2015-11-19T10:57:40Z

@bf4 I am ok with that
Please just keep my single commit bf4@5142496 ❤️

per NullVoxPopuli comment

Instrumenting rendering of resources

bf4 · 2015-11-19T19:01:48Z

Merged

maurogeorge · 2015-11-19T19:39:25Z

Thanks 🎉 🎊

wagenet · 2015-11-23T20:41:13Z

Thanks! Haven't had a chance to use it yet, but hope to soon :)

wagenet · 2015-11-24T16:09:41Z

Started to play around with this some. Looks good to me so far!

bf4 force-pushed the maurogeorge-patch-10 branch from 600974d to 2f4bb4a Compare November 8, 2015 22:09

bf4 reviewed Nov 8, 2015
View reviewed changes

bf4 force-pushed the maurogeorge-patch-10 branch 3 times, most recently from 2df3f23 to e39dce9 Compare November 9, 2015 01:38

bf4 added C: Feature V: 0.10.x S: Needs Code Review S: Ready for PR S: Changelog Missing labels Nov 9, 2015

maurogeorge mentioned this pull request Nov 9, 2015

Add Logging #1291

Closed

maurogeorge reviewed Nov 9, 2015
View reviewed changes

maurogeorge and others added 6 commits November 10, 2015 03:09

Keep Logging in the ActiveModelSerializers namespace

21bb306

Clean up notification code with some meta-prog

360ecc8

Use null logger in test; keep track of original logger

84c3b11

Refactor callbacks in ams::logging

e8efc4e

Separate out callbacks per ActiveJob pattern

b36cc42

bf4 force-pushed the maurogeorge-patch-10 branch from fcb8b05 to d4fc811 Compare November 10, 2015 09:28

maurogeorge mentioned this pull request Nov 10, 2015

Proposal: Create assert_response_schema test helper #1270

Closed

NullVoxPopuli reviewed Nov 10, 2015
View reviewed changes

maurogeorge mentioned this pull request Nov 13, 2015

RFC: Primary namespace: ActiveModel::Serializer, ActiveModel::Serializers, vs. ActiveModelSerializers #1310

Merged

bf4 added 2 commits November 19, 2015 12:41

Update for review per maurogeorge feedback

d1c44c7

Rename event from AMS to active_model_serializers

733f5bc

per NullVoxPopuli comment

bf4 force-pushed the maurogeorge-patch-10 branch from d4fc811 to 733f5bc Compare November 19, 2015 18:44

bf4 added a commit that referenced this pull request Nov 19, 2015

Merge pull request #1322 from bf4/maurogeorge-patch-10

aa43848

Instrumenting rendering of resources

bf4 merged commit aa43848 into rails-api:master Nov 19, 2015

bf4 deleted the maurogeorge-patch-10 branch November 19, 2015 19:01

maurogeorge mentioned this pull request Dec 22, 2015

Bring back assert_serializer #1099

Closed

bf4 removed the S: Changelog Missing label Jan 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrumenting rendering of resources #1322

Instrumenting rendering of resources #1322

bf4 commented Nov 8, 2015

bf4 Nov 8, 2015

maurogeorge Nov 9, 2015

beauby commented Nov 8, 2015

maurogeorge Nov 9, 2015

bf4 Nov 9, 2015

maurogeorge Nov 9, 2015

bf4 commented Nov 10, 2015

maurogeorge commented Nov 10, 2015

NullVoxPopuli Nov 10, 2015

NullVoxPopuli Nov 10, 2015

bf4 Nov 10, 2015

Macros that can be used to customize the logging of class or instance methods,

bf4 commented Nov 13, 2015

wagenet commented Nov 17, 2015

bf4 commented Nov 19, 2015

maurogeorge commented Nov 19, 2015

bf4 commented Nov 19, 2015

maurogeorge commented Nov 19, 2015

wagenet commented Nov 23, 2015

wagenet commented Nov 24, 2015

		@@ -0,0 +1,19 @@
		# Instrumentation

		ActiveModelSerializers uses the ActiveSupport::Notification API, which

Instrumenting rendering of resources #1322

Instrumenting rendering of resources #1322

Conversation

bf4 commented Nov 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Nov 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Nov 10, 2015

maurogeorge commented Nov 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Macros that can be used to customize the logging of class or instance methods,

bf4 commented Nov 13, 2015

wagenet commented Nov 17, 2015

bf4 commented Nov 19, 2015

maurogeorge commented Nov 19, 2015

bf4 commented Nov 19, 2015

maurogeorge commented Nov 19, 2015

wagenet commented Nov 23, 2015

wagenet commented Nov 24, 2015