boundary / folsom

Expose Erlang Events and Metrics
Apache License 2.0
585 stars 166 forks source link

Added grouped metrics feature #51

Closed viveshok closed 11 years ago

viveshok commented 11 years ago

This pull request introduces three new API calls available to users who want to group or tag metrics monitoring a common task together:

    > folsom_metrics:tag_metric(Name, Tag).
    > folsom_metrics:get_metrics_value(Tag).
    > folsom_metrics:get_metrics_value(Tag, Type).

Please review and comment, I'll make any correction needed for merging requested.

Thanks!

russelldb commented 11 years ago

I like it, and it works. I've made a couple of comments.

Adding tags after creation like this makes me wonder why we don't add tags at metric creation time. Since we can add tags during a metric's life, why not remove? I just like symmetric apis.

All that said, as a folsom user, I have no hesitation take this as is.

viveshok commented 11 years ago

I like these suggestions, I'll get working on them

pmembrey commented 11 years ago

Looks good to me and hits my main pain points. I'd still like aggregation, but that's a more complicated and I feel separate issue. This is certainly good enough for my needs.

Once the ability to remove has been added etc and the changes are accepted by @joewilliams I would be happy to release the $100 bounty :)

viveshok commented 11 years ago

@russelldb made four good suggestions to improve this pull request, below is a list with my comments:

_(i) Use an ets:select in folsom_ets:get_group_values instead of reinventing the wheel and using many local functions_

Sadly the set of possible guards in ets:select is rather narrow and doesn't allow guards as complex as "check if the set inside the tuple contains element X". This said the suggestion did lead me to use ets:match with some improvement in cleanliness.

_(ii) There's a possible race condition in created by the two new functions folsom_ets:add_tag/2 and folsom_ets:rm_tag/2_

This is true. I might be missing something but to the extent of my knowledge the only way to fix this would be to make the module folsom_ets a gen_server. This is perhaps a good idea but requires a refactoring of the code too invasive for me to be comfortable doing.

(iii) Tagging metrics at creation rather than the tedious two-steps create+tag process

It's very easy to do this in a clean way for some types of metric (counters, gauges, meters, meter_readers, spirals) but trickier for types taking variable # of arguments (histograms, histories, durations). I couldn't think of a neat way to do it, so I didn't...

(iv) Give the possibility to untag metrics for API symmetry

Done.

Once more, all comments welcomed!

pmembrey commented 11 years ago

I'm happy with this...

Any comments from anyone else?

pmembrey commented 11 years ago

Anyone? I'd really like to start using this... :)

russelldb commented 11 years ago

Sorry, yes +1 from me. Nice work.

joewilliams commented 11 years ago

Thanks to everyone for contributing and +1'ing this patch!

@pmembrey @AlexandreBeaulne please work together to settle up.

pmembrey commented 11 years ago

No problem. @AlexandreBeaulne please email me your payment details and I'll get the funds transferred right away :)

viveshok commented 11 years ago

Thanks everyone involved :)

For the records, despite some initial reluctance on my part, @pmembrey insisted on transferring me a USD 100 bounty for this ticket.