Collect measure and distribution metrics even on return or exception

[Bringer128]

Duplicate of #111 (except no conflicts) and fixes #133

This is a fix for when doing a return inside a StatsD.measure or StatsD.distribution call, where the metrics would not be collected.

Note that as discussed in #111 if exceptions happen you will likely get a different distribution of performance. At the very least we should consider documenting this behaviour.

Keen to get this merged as I lost a day to this problem (existing code using a return, and I added a block to measure performance).

[pallan]

cc @tjoyal @wvanbergen

[tjoyal]

I am good with small steps as this feels like a good direction (not dropping events in the first place). Discussing how to possibly "flag" exit scenarios can be discussed elsewhere.

[Bringer128]

I'll try get some time to look into those tests tomorrow

[Bringer128]

If I can get a 👍 from @tjoyal I think we can merge this?

[tjoyal]

I'm good with this and it fixes part of the problem. Since it can be ship in isolation I think it should.

My worry is that now "what was discarded will now be recorded" which is to some extent the desire. From then on the "exceptions" data will be folded into the current data and this could lead to some inconsistencies in dashes (eg.: tracking timing out http call exception could greatly increase the average call time for events).

I'm not saying to not ship this, but it need to be documented so when the gem get bumped people are aware (This could trip pagers).

On that note, I think we should rapidly come up with a way to distinguish "success vs failures" so that dashes can have a more rich representation?

We need to go though this harsh migration if we want to have a better future. Thanks for owning the gem!

[Bringer128]

I'm not saying to not ship this, but it need to be documented so when the gem get bumped people are aware (This could trip pagers).

If this is a more significant change should we modify the minor or even major version? While it's not a breaking change at the code level (re: semver) it is potentially a breaking behaviour change that people should be aware of when they upgrade.

[tjoyal]

If this is a more significant change should we modify the minor or even major version?

I don't really have a strong opinion on this, I'm happy with a log entry and a version change.

I'm a big fan of simple, ship fast and iterate. This changeset might be a problem for some, but for others it might surface some unknown problem. I'd say let's keep the train going and simply ship & iterate.

In the end, you should always be reading at least the changelog when updating a gem. Weither it's a minor or major version number change, since we do not manage multiple version in parallel, you can only pick 2 choices: upgrade or not.

Change in the end always risk breaking someone experience, as long that we make it better for most I think we should keep going and rectify on feedback if there is any.

[Bringer128]

👍 Can you review my changelog? I wanted to give a quick warning in there related to our discussions