Feature - Emit internal metrics using StatsD

[elliotfehr]

[elliotfehr]

@carlhoerberg @dentarg Curious if there are any thoughts for this feature addition?

[dentarg]

I like the general idea of supporting StatsD, but I'm not sure how we want to do it. Carl probably has some opinions, and maybe @jage has some ideas too?

[dentarg]

We probably want statsd as an optional dependency

[elliotfehr]

I'm pretty new to crystal lang and shards and was wondering if you have any tips for making the statsd dependency optional?

[dentarg]

Sorry, I was thinking too much like a Ruby developer when I said we could make statsd an optional dependency :) Here we compile a binary so it makes sense to include statsd like you have done if we want to have this feature. Can we add a spec for it so we know it is working? Could take some inspiration from the upstream lib

[elliotfehr]

Sorry, I was thinking too much like a Ruby developer when I said we could make statsd an optional dependency :) Here we compile a binary so it makes sense to include statsd like you have done if we want to have this feature. Can we add a spec for it so we know it is working? Could take some inspiration from the upstream lib

@dentarg added a test for metrics in https://github.com/cloudamqp/amqproxy/pull/68/commits/38a4b8d71b1b8d2fcbd1a58cf82b62557bb08946

[dentarg]

Nice, if you rebase with master CI should run

[elliotfehr]

@dentarg just rebased on master but am not seeing CI kicking off. Guessing this is due to GH actions being degraded at the moment: https://www.githubstatus.com/

Edit: Looks like they are running now!

[elliotfehr]

I'm not too sure how to resolve these CI errors: https://github.com/cloudamqp/amqproxy/runs/5591692534?check_suite_focus=true

On my fork it appears there are also linting errors unrelated to changes in this PR: https://github.com/elliotfehr/amqproxy/runs/5591765809?check_suite_focus=true

[dentarg]

Yes, the linting issues hasn't been solved in master yet. I've excluded that step now, if the event that triggers the CI workflow comes from a fork, so if you could rebase one more time 🙏 :)

[elliotfehr]

@dentarg CI passed in https://github.com/cloudamqp/amqproxy/pull/68/commits/3e7de7c362c3351f4e6d371f985804e65e72fb8d! Let me know if there are any other changes you'd like to see!

[dentarg]

Great work @elliotfehr

@carlhoerberg WDYT?

[elliotfehr]

@carlhoerberg Curious if you have any thoughts on this?

[elliotfehr]

@carlhoerberg pushed an update in 6edb7e29ffe8afd4df884f8f3675d0b43851dd7c to fix merge conflicts and add support for the new ini config system.

[elliotfehr]

Bumping this. Have been running this change for a while now and have not seen any regressions or issues. Let me know if there is anything you'd like to see updated!

[dentarg]

Nice work @elliotfehr, just tried it out locally. I'm thinking a nice touch would be to print on upstart that StatsD reporting will be used (if it has been configured), similar to what we already, printing the upstream and listening address

$ bin/amqproxy -c config/example.ini
2022-06-01 08:00:25 +02:00: Proxy upstream: localhost:5672
2022-06-01 08:00:25 +02:00: Proxy listening on 127.0.0.1:5673

What do you think about that?

[elliotfehr]

@dentarg sounds useful! I just added that in https://github.com/cloudamqp/amqproxy/pull/68/commits/7b40d42c5503b0f2dfbbead3e327ec4d7e260639. It required moving the logging initialization outside of the server init so we can pass it around to other classes. Let me know what you think!

[carlhoerberg]

the branch is locked, so can't push a rebase

[dentarg]

@elliotfehr mind resolving the conflicts? GitHub didn't let me commit to the branch

[elliotfehr]

@dentarg I believe all conflicts have been resolved. It would be greatly appreciated if we can get a quick review before any more large changes are made to the default branch as this PR has been ongoing for close to 5 months now and the conflicts introduced have been non-trivial to resolve.

Would also appreciate if we can get some eyes on https://github.com/cloudamqp/amqproxy/pull/71 as well. As a side note, I've been running both of these additions in production for quite some time now without any issue.

[dentarg]

Sorry about that, and thanks for fixing the conflicts. LGTM but I'll let @carlhoerberg merge it.

[carlhoerberg]

I contributed to statds.cr so that a constructor that took a an IPAddress directly could be used: https://github.com/miketheman/statsd.cr/releases/tag/v0.5.0

[elliotfehr]

I contributed to statds.cr so that a constructor that took a an IPAddress directly could be used: https://github.com/miketheman/statsd.cr/releases/tag/v0.5.0

@carlhoerberg I may be missing something, but I do not see how this resolves the issue in https://github.com/cloudamqp/amqproxy/pull/68#discussion_r892110076. If we want to support passing a DNS name (which I personally would like to support since I am using both DNS names and IP address directly in my setup), we still need to resolve the statsd host to an IP to pass to the statsd lib. Do you mind submitting a pull request that targets this branch with the changes you would like to see? Ideally, it would be great to see this PR merged and potentially someone could follow up with an enhancement to upgrade the statds lib with the changes you contributed.

[carlhoerberg]

fixed the merge conflicts but the branch is protected somehow..

remote: error: GH006: Protected branch update failed for refs/heads/feature/statsd-sink.
remote: error: Cannot force-push to this protected branch
To https://github.com/elliotfehr/amqproxy.git
 ! [remote rejected] feature/statsd-sink -> feature/statsd-sink (protected branch hook declined)

[elliotfehr]

@carlhoerberg pushed up a change to resolve the merge conflicts!

[elliotfehr]

Going to close this out since it doesn't seem like there's a strong interest in this feature.