[RFC] Exception handling and api normalization

[maximebedard]

Fixes: #110

Follow-up on #111, #112

To improve consistency of this library and try to minimize unexpected side-effects from developers, I have a few proposal I'd like to make.

changes to .statsd_count_if

The README currently states the following:

This will only increment the given key if the method executes successfully. So now, if GoogleBase#insert raises an exception or returns false (ie. result == false), we won't increment the key. If you want to define what success means for a given method you can pass a block that takes the result of the method.

I propose that instead of comparing against a false value that we use the ruby if directly to evaluate the truthiness/falsyness of the value. If the method raises, no metric will be pushed.

changes to .statsd_count_success

I propose that we evaluate the condition to deliver the metric exactly the same way defined in .statsd_count_if. However, instead of creating 2 buckets for success and failure, I propose we add a 3rd bucket error for situations where the method raises.

changes to measure .statsd_measure

I propose that this method would push metrics for all situations (normal execution, raise). In other words, it would behave exactly the same way as .statsd_count.

add .statsd_measure_success, .statsd_measure_if

I propose we match the same API we do for .statsd_count_*. This would allow us to give more flexibility as to which metrics we want to observe in the end.

changes to #measure

I suggest we create 2 distinct methods:

• measure (push a single metrics in all cases -- even when the method raises)
• measure_success (push a metric in 3 distinct buckets success, failure, error)

Note: measure_success would have to accept a proc in order to allow the consumer to define what success means. By default it will simply check truthy/falsy values.

changes to tags/automatic suffixes

For StatsD implementations that supports tags, instead of pushing a metric with a suffix MetricName.success|failure|error, we could push them as a tag with the key status:#{status}.

Any thoughts/concerns? ❤️

@tjoyal @simon-shopify @wvanbergen @alexsnaps @dnlserrano

[simon-shopify]

I think this makes a lot of sense. IMO the comparison to false is a major trap and I'm sure we have code that is broken atm because of that.

[tjoyal]

Big 👍 for me.

Assuming your backend support tags, I'm not sure I would ever use anything except statsd_*_success from there on and that's maybe what I'm not 100% sure about the "metric name" not representing the "tracking method" used anymore, but maybe that's implementation details we can discuss in the specific change attached to tags (nothing to do with re-orienting the method reporting logic)

It makes "everything possible" so I'm happy with the direction.

[dnlserrano]

Hi guys! 😄

I generally agree with the proposed changes. Just 2 things I'd like to mention:

1. IMHO, this shouldn't be a blocker for Send measure metric even when the block raises #111 to get merged, but take it with a grain of salt given this is coming from someone who needs this ASAP. 😜

2. The third bucket error can be a pain in terms of retro-compatibility. I can imagine a lot of angry devs that suddenly need to change all their dashboards to sum failure and error to have the same behaviour (i.e., stats) as before.

[maximebedard]

The third bucket error can be a pain in terms of retro-compatibility. I can imagine a lot of angry devs that suddenly need to change all their dashboards to sum failure and error to have the same behaviour (i.e., stats) as before.

Ideally I would do a major version bump since there's quite a bit of changes that devs will need to be aware of and quite a bit of behaviour changes to handle. IMO, if we are able to split the buckets into tags instead, they would all get collapse into the same bucket by default and allow the user to slice for the preferred dimension without loosing anything.

[wvanbergen]

This would require a major version bump. However, the way you have proposed your changes makes for a very bad upgrade path, because you have to do it in a big bang. That's going to be very painful for many of our own apps, let alone others.

I've never been a big fan of these metaprogramming methods. I rather deprecate them, and add new ones that are better. I started something with #104 that I think is a more flexible design. Maybe we can continue that work, and build new metaprogramming methods on top of it?

I think we only need one generic metaprogramming method, a measure (which creates a bunch of metrics including a count), tagged with whether the method was successful or not (successful = no exception). This is the 95% use case, and could work something like:

MyClass.prepend StatsD::Instrument.measure_method(:my_method)

For every other use case, what we mainly need is a way to edit the StatsD::Instrument::Metric object after the the method call but before it is submitted. Not quite sure what API I would prefer for that, but it is the most flexible way to customize things based on return value, exceptions, parameters, etc.

# I don't like this API but you get the idea
MyClass.prepend StatsD.instrument_method(:my_method) do |metric, parameters, return_value, exception|
  metric.name = "#{metric.name}.#parameters[0]"
  metric.tags << exception.class.name if exception
end

Not too sure of a good API for the second use case. Maybe anyone has good ideas?

[wvanbergen]

Maybe we should discuss a new major release.

1. What things would we like to change that breaks the current API.
2. Is there a decent upgrade path available where we temporarily make the old way and new way of doing things available, so people don't have to do a big bang upgrade?

For me, these things have always annoyed me, and I'd like to fix:

• Use keyword arguments rather than option hashes, and drop support for positional arguments for most of the API.
• Get rid of the singleton by introducing a proper Client object.
• Drop support for Ruby < 2.1

[maximebedard]

I had mostly the same annoyance as you :) I'm pretty eager to fix all of these and expose a new major version. I would also propose a few changes for the UdpBackend, expose a per-implementation strategy instead of a primitive symbol like in #109.

I think if we go we the ModuleBuilder, we can expose new methods and mark the other as deprecated. As of instance methods such as #measure, #count, ..., these one are going to be more tricky. But the change to this were mostly around #measure and provide hooks to augment the other methods, so they are not technically all breaking changes.

[wvanbergen]

We could separate the measure variant that takes a value from the measure variant that takes a block (e.g. as measure_block). Use measure_block for the new block behavior, and deprecate calling measure with a block.