Replace custom lifecycle with AS::Notifications #138
Conversation
|
Are we not registering for any Lifecycle events within protobuf? That seems odd to me. Also, I'm not sure it's a good idea to just remove the Lifecycle class entirely seeing as external classes will depend on the type being present. Perhaps the right solution is to leave the class and have it simply wrap AS::Notifications, possibly with a deprecation warning. |
| @@ -1,5 +1,3 @@ | |||
| # encoding: utf-8 | |||
There was a problem hiding this comment.
Please revert this removal. It's a change that shouldn't go out.
Not sure why this got deleted. It was not intentional
This will warn users that Lifecycle events are being deprecated in favor of active support notifications.
|
@localshred, I added a friendly transition path with deprecation warnings. What thinkest thou? |
| @@ -244,7 +243,7 @@ def start_server | |||
| "pid #{::Process.pid} -- #{@runner_mode} RPC Server listening at #{options.host}:#{options.port}" | |||
| } | |||
|
|
|||
| ::Protobuf::Lifecycle.trigger( "after_server_bind" ) | |||
| ::ActiveSupport::Notifications.instrument( "after_server_bind") | |||
|
So, did you want to open this against 2-8-stable, 2-9-dev, or master? |
|
Well, I meant to open it against 2-8-stable. Is that what I should do? |
|
If you want this in the next release, then yes. |
|
Closed in favor of #139 |
By using ActiveSupport::Notificaitons, we can take advantage other tools already available that work with its interface. This will also make it easier for persons already familiar with ActiveSupport::Notifications to add logging, statistics, etc. for protobuf. After this change, I'd like to continue by adding additional instrumentation hooks throughout the code, so I can easily integrate it with logstash and statsd.
I know the increased dependency on activesupport is not ideal, but I think the benefits of having a uniform instrumentation interface is worth it. Rails (starting in 3.0 I believe) started using AS::Notifications for all of its logging, so I feel confident suggesting it is production hardened code.
Thoughts?
@abrandoned @localshred @liveh2o