Instrument with ActiveSupport::Notifications #1086

wagenet · 2015-08-03T14:23:54Z

Since Grape already requires ActiveSupport, adding a couple of instrumentation points with ActiveSupport::Notifications requires minimal changes.

I did have to make some small changes to run_filters to provide "by type" instrumentation. However, the second parameter is optional so nothing bad should happen if other gems override this method with the old signature. In the worst case, all filters would report as being of type "other" or just not get reported at all.

About ActiveSupport::Notifications

AS::N is intended to be a generic instrumenter for Ruby applications and is very light-weight. It provides a great way for library authors to allow other tools (e.g. Skylight, NewRelic) to instrument the library in intelligent ways without having to monkeypatch. When nothing is observing a specific instrumentation the code is essentially a pass-through adding negligible overhead.

For more information see the documentation.

To Do

Document the instrumentation points (in the vein of http://edgeguides.rubyonrails.org/active_support_instrumentation.html)
Maybe write some tests to ensure that the instrumentation gives us what we expect.

wagenet · 2015-08-03T14:25:23Z

@dblock we discussed this a bit and I figured I'd just send in something simple to show the sort of thing I have in mind. The specific hook points I picked here are ones that I believe would be useful for Skylight, though again I believe they would be useful elsewhere. If we're not able to get first class support for these hooks, we'll have to resort to monkey patching.

wagenet · 2015-08-03T14:35:49Z

To expand further, I don't see how we could get this level of hooking with just a gem. The gem would also have to monkeypatch to get to the depths that these methods do. Alternatively, we could try splitting up the existing Grape methods more to allow for a relatively straight-forward alias + override flow. IMO, the minimal weight of instrumentation merits support in Grape itself, but this isn't my project :)

dblock · 2015-08-04T11:39:53Z

I'll leave this open for comments. I would prefer some refactoring in Grape where methods that make calls are more explicit and a separate gem that patches or overrides them. But I'd like others to comment.

dblock · 2015-08-04T11:40:18Z

This needs README documentation for sure if it were to get merged, you should write that and squash the commits.

wagenet · 2015-08-04T13:50:43Z

@dblock I'm definitely happy to add a README, tests, and anything else necessary. I just didn't want to spend too much time on something that was going to be dead on arrival.

wagenet · 2015-08-04T13:55:41Z

@dblock Are there any specific rules on when method signatures are changed? I know Grape hasn't reached 1.0 yet, but is the plan to be semver? Would this apply to all public class methods? documented methods? something else?

If we don't do this as a first class thing and instead override methods, I'd feel a lot more comfortable if I knew that API wouldn't change out from under me until a major release.

dblock · 2015-08-04T14:22:21Z

We don't have hard rules. I think it's definitely possible that the API would change underneath you, but the way I've done this in the past was to have a good battery of tests against specific versions of the upstream library, and make dependent gems compatible with a fixed number of releases. At the very least if there's an explicit method like run that does 1 thing it's unlikely that it will change.

So rough consensus and working code.

wagenet · 2015-08-04T18:10:19Z

@dblock I realized that Grape already requires ActiveSupport so I updated this PR to just use AS::N directly. This actually makes the code simpler than what I was originally proposing. I've updated the description accordingly.

tchak · 2015-08-04T21:05:01Z

This PR looks very useful. I hope it will be merged. Instrumentation should be first class citizen in any framework :)

dblock · 2015-08-04T21:06:34Z

lib/grape/endpoint.rb

-      @env = env
-      @header = {}
+      ActiveSupport::Notifications.instrument('run.grape', endpoint: self, env: env) do
+        @env = env


Lets refactor the body into a method? Maybe run should call instrumented_run or run_instrumented (as in run_filters) something like that?

@dblock I guess I'm confused as to how this helps. The implication here is that there could also be a run without instrumentation. What value does this provide?

Only some code clarity, shorter methods, easier to grok. No strong opinions.

Fair enough, looks like the whole run method has potential for splitting up further.

On further thought, refactoring seems good for a different PR.

dblock · 2015-08-04T21:07:16Z

I'd like to see a README for this and tests. I am open to this being part of Grape.

wagenet · 2015-08-04T21:09:01Z

@dblock I'll add some docs for this shortly.

wagenet · 2015-08-04T22:07:59Z

@dblock docs added and CHANGELOG updated. I'm still happy to clean up the run method a bit, but as I mentioned elsewhere I feel like that should be a separate PR.

dblock · 2015-08-05T15:28:48Z

README.md

@@ -2628,6 +2628,34 @@ See [StackOverflow #3282655](http://stackoverflow.com/questions/3282655/ruby-on-

 ## Performance Monitoring

+### Active Support Instrumentation


There's a TOC above, I think this needs to be added there.

dblock · 2015-08-05T15:29:12Z

Agreed re: run and a separate PR.

dblock · 2015-08-05T15:29:55Z

I thought about this more. If you write tests I will merge this.

wagenet · 2015-08-05T15:41:54Z

@dblock sounds great! I'll try to get the tests done today.

wagenet · 2015-08-05T21:09:39Z

@dblock I've added tests. They're a little unusual so if have suggestions for improving them, let me know.

dblock · 2015-08-06T13:11:15Z

README.md

+* *filters* - The filters being executed
+* *type* - The type of filters (before, before_validation, after_validation, after)
+
+See the [ActiveSupport::Notifications documentation](http://api.rubyonrails.org/classes/ActiveSupport/Notifications.html] for information on how to subscripe to these events.


Typo: subscriBe.

dblock · 2015-08-06T13:13:39Z

Fixed typo and merged via 94f52e4. Thank you.

wagenet · 2015-08-06T13:54:57Z

@dblock thanks!

wagenet changed the title ~~Basic AS::N compatible instrumentation~~ Instrument with ActiveSupport::Notifications Aug 4, 2015

wagenet force-pushed the instrumentation branch from 5353f01 to cb27105 Compare August 4, 2015 18:05

wagenet force-pushed the instrumentation branch from cb27105 to 29b1ac4 Compare August 4, 2015 18:15

dblock reviewed Aug 4, 2015
View reviewed changes

wagenet force-pushed the instrumentation branch from 29b1ac4 to 94a9989 Compare August 4, 2015 22:01

wagenet force-pushed the instrumentation branch 2 times, most recently from 7c7a1f5 to f1f2405 Compare August 5, 2015 14:54

dblock reviewed Aug 5, 2015
View reviewed changes

wagenet force-pushed the instrumentation branch from f1f2405 to ab0b5af Compare August 5, 2015 15:41

wagenet force-pushed the instrumentation branch from ab0b5af to 1669ab3 Compare August 5, 2015 21:09

Instrument with ActiveSupport::Notifications

a7441ce

wagenet force-pushed the instrumentation branch from 1669ab3 to a7441ce Compare August 6, 2015 00:42

dblock reviewed Aug 6, 2015
View reviewed changes

dblock closed this Aug 6, 2015

wagenet deleted the instrumentation branch August 6, 2015 17:12

dm1try mentioned this pull request Aug 20, 2015

ActiveSupport::Notifications hook points #1084

Closed

danielmorrison mentioned this pull request Oct 15, 2023

Instrument with ActiveSupport::Notifications collectiveidea/twirp-rails#9

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrument with ActiveSupport::Notifications #1086

Instrument with ActiveSupport::Notifications #1086

wagenet commented Aug 3, 2015

wagenet commented Aug 3, 2015

wagenet commented Aug 3, 2015

dblock commented Aug 4, 2015

dblock commented Aug 4, 2015

wagenet commented Aug 4, 2015

wagenet commented Aug 4, 2015

dblock commented Aug 4, 2015

wagenet commented Aug 4, 2015

tchak commented Aug 4, 2015

dblock Aug 4, 2015

wagenet Aug 4, 2015

dblock Aug 4, 2015

wagenet Aug 4, 2015

wagenet Aug 4, 2015

dblock commented Aug 4, 2015

wagenet commented Aug 4, 2015

wagenet commented Aug 4, 2015

dblock Aug 5, 2015

wagenet Aug 5, 2015

dblock commented Aug 5, 2015

dblock commented Aug 5, 2015

wagenet commented Aug 5, 2015

wagenet commented Aug 5, 2015

dblock Aug 6, 2015

dblock commented Aug 6, 2015

wagenet commented Aug 6, 2015

		@@ -2628,6 +2628,34 @@ See [StackOverflow #3282655](http://stackoverflow.com/questions/3282655/ruby-on-

		## Performance Monitoring

		### Active Support Instrumentation

Instrument with ActiveSupport::Notifications #1086

Instrument with ActiveSupport::Notifications #1086

Conversation

wagenet commented Aug 3, 2015

About ActiveSupport::Notifications

To Do

wagenet commented Aug 3, 2015

wagenet commented Aug 3, 2015

dblock commented Aug 4, 2015

dblock commented Aug 4, 2015

wagenet commented Aug 4, 2015

wagenet commented Aug 4, 2015

dblock commented Aug 4, 2015

wagenet commented Aug 4, 2015

tchak commented Aug 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Aug 4, 2015

wagenet commented Aug 4, 2015

wagenet commented Aug 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Aug 5, 2015

dblock commented Aug 5, 2015

wagenet commented Aug 5, 2015

wagenet commented Aug 5, 2015

Choose a reason for hiding this comment

dblock commented Aug 6, 2015

wagenet commented Aug 6, 2015