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

Instrument with ActiveSupport::Notifications #1086

Closed
wants to merge 1 commit into from

Conversation

@wagenet
Copy link
Contributor

@wagenet wagenet commented Aug 3, 2015

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

@wagenet
Copy link
Contributor Author

@wagenet wagenet commented Aug 3, 2015

@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
Copy link
Contributor Author

@wagenet wagenet commented Aug 3, 2015

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
Copy link
Member

@dblock dblock commented Aug 4, 2015

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
Copy link
Member

@dblock dblock commented Aug 4, 2015

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

@wagenet
Copy link
Contributor Author

@wagenet wagenet commented Aug 4, 2015

@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
Copy link
Contributor Author

@wagenet wagenet commented Aug 4, 2015

@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
Copy link
Member

@dblock dblock commented Aug 4, 2015

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 wagenet changed the title Basic AS::N compatible instrumentation Instrument with ActiveSupport::Notifications Aug 4, 2015
@wagenet wagenet force-pushed the tildeio:instrumentation branch from 5353f01 to cb27105 Aug 4, 2015
@wagenet
Copy link
Contributor Author

@wagenet wagenet commented Aug 4, 2015

@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.

@wagenet wagenet force-pushed the tildeio:instrumentation branch from cb27105 to 29b1ac4 Aug 4, 2015
@tchak
Copy link

@tchak tchak commented Aug 4, 2015

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

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

This comment has been minimized.

@dblock

dblock Aug 4, 2015
Member

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

This comment has been minimized.

@wagenet

wagenet Aug 4, 2015
Author Contributor

@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?

This comment has been minimized.

@dblock

dblock Aug 4, 2015
Member

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

This comment has been minimized.

@wagenet

wagenet Aug 4, 2015
Author Contributor

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

This comment has been minimized.

@wagenet

wagenet Aug 4, 2015
Author Contributor

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

@dblock
Copy link
Member

@dblock dblock commented Aug 4, 2015

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

@wagenet
Copy link
Contributor Author

@wagenet wagenet commented Aug 4, 2015

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

@wagenet wagenet force-pushed the tildeio:instrumentation branch from 29b1ac4 to 94a9989 Aug 4, 2015
@wagenet
Copy link
Contributor Author

@wagenet wagenet commented Aug 4, 2015

@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.

@wagenet wagenet force-pushed the tildeio:instrumentation branch 2 times, most recently from 7c7a1f5 to f1f2405 Aug 5, 2015
@@ -2628,6 +2628,34 @@ See [StackOverflow #3282655](http://stackoverflow.com/questions/3282655/ruby-on-

## Performance Monitoring

### Active Support Instrumentation

This comment has been minimized.

@dblock

dblock Aug 5, 2015
Member

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

This comment has been minimized.

@wagenet

wagenet Aug 5, 2015
Author Contributor

Done.

@dblock
Copy link
Member

@dblock dblock commented Aug 5, 2015

Agreed re: run and a separate PR.

@dblock
Copy link
Member

@dblock dblock commented Aug 5, 2015

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

@wagenet wagenet force-pushed the tildeio:instrumentation branch from f1f2405 to ab0b5af Aug 5, 2015
@wagenet
Copy link
Contributor Author

@wagenet wagenet commented Aug 5, 2015

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

@wagenet wagenet force-pushed the tildeio:instrumentation branch from ab0b5af to 1669ab3 Aug 5, 2015
@wagenet
Copy link
Contributor Author

@wagenet wagenet commented Aug 5, 2015

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

@wagenet wagenet force-pushed the tildeio:instrumentation branch from 1669ab3 to a7441ce Aug 6, 2015
* *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.

This comment has been minimized.

@dblock

dblock Aug 6, 2015
Member

Typo: subscriBe.

@dblock
Copy link
Member

@dblock dblock commented Aug 6, 2015

Fixed typo and merged via 94f52e4. Thank you.

@dblock dblock closed this Aug 6, 2015
@wagenet
Copy link
Contributor Author

@wagenet wagenet commented Aug 6, 2015

@dblock thanks!

@wagenet wagenet deleted the tildeio:instrumentation branch Aug 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.