Permit apps to send messages to Statsd over TCP #180
Conversation
There was a problem hiding this comment.
Nice job Bill - hope you get your change into Statsd.
One thing I'm not totally clear on is if it's a good idea to use Statsd over TCP - I was under the impression that the UDP nature of it meant there was much less overhead for an app to send metrics - acting somewhat fire and forget. It's not my forté though so I don't know if the overhead is negligible in practice.
govuk_app_config.gemspec
Outdated
| @@ -22,7 +22,7 @@ Gem::Specification.new do |spec| | |||
|
|
|||
| spec.add_dependency "logstasher", ">= 1.2.2", "< 2.2.0" | |||
| spec.add_dependency "sentry-raven", "~> 3.1.1" | |||
| spec.add_dependency "statsd-ruby", "~> 1.4.0" | |||
| spec.add_dependency "statsd-ruby-tcp", "0.0.0" | |||
There was a problem hiding this comment.
This seems an odd version of a gem? I gather this is one we've published. I'd have thought we should release it as 0.1 and set this constraint as ~> 0.1.0
There was a problem hiding this comment.
Thanks, yeah I don't remember why I set the version to 0.0.0 - I've repushed it as 0.1.0 and updated this commit.
lib/govuk_app_config/govuk_statsd.rb
Outdated
| @@ -8,7 +8,7 @@ module GovukStatsd | |||
|
|
|||
| def self.client | |||
| @client ||= begin | |||
| statsd_client = ::Statsd.new("localhost") | |||
| statsd_client = ::Statsd.new(ENV["STATSD_HOST"] || "localhost", 8125, ENV["STATSD_PROTOCOL"]&.to_sym || :udp) | |||
There was a problem hiding this comment.
It looks like we've already got a precedent for a prefix of GOVUK_ to statsd env vars (see line below) so given they're set for this gem rather than the underlying statsd gem.
Should the Statsd class in the fork we've created (statsd-ruby-tcp) have a different name to ::Statsd - It strikes me that if any other apps that use this gem also have a dependency on Statsd we'll have a namespace collision - It's a shame we can't point to a git branch at a dependency of a gem like we'd do in an app.
It does seem a little odd that we don't have an env var for PORT. I guess it's a point of need thing, but it ends up reading strange on this utility.
I had a somewhat whacky idea that could maybe use an env var of GOVUK_STATSD_URI that could be set and then parse that for args
For example:
statsd_uri = URI.parse(ENV.fetch("GOVUK_STATSD_URI", "localhost"))
statsd_client = ::Statsd.new(statsd_uri.host, stats.port || 8125, statsd_uri.scheme&.to_sym || :udp)
There was a problem hiding this comment.
Thanks! I updated the env var names.
I am open to enabling apps to configure the port, but I don't think we need this yet given our apps aren't using Statsd's admin port 8126 - if they do need to, we could make this configurable. Would that be OK?
we'll have a namespace collision
Thanks for mentioning that. Search API uses both govuk_app_config and statsd-ruby.
We could stop wrapping the Statsd client with GovukStatsd, but that's a bit nuclear. I've changed the name in the temporary gem to StatsdTcp, since I'm hoping I'll get my PR merged in soon.
There was a problem hiding this comment.
Nice one. No love for my single env var idea? 😢
Thanks for re-releasing the Gem with the new class name 👍
There was a problem hiding this comment.
No love for my single env var idea? 😢
It's a neat idea! I can definitely imagine a scenario where an app would want to send messages to the admin port (8126). Setting GOVUK_STATSD_URI=localhost:8126 via puppet might be neat.
But setting GOVUK_STATSD_URI=localhost:8125 might be problematic if an app also wants to send messages to the stats port (8125). We might have to add GOVUK_STATSD_ADMIN_URI=localhost:8126, duplicating the host.
Another reason is that I don't foresee the port numbers we'd use for Statsd changing on a per-app basis, since we'd also have to make that change at Statsd.
This is useful since Statsd running in Fargate listens on a TCP port, not over UDP (which is the default and what we have running in production).
bilbof
force-pushed
the
bilbof/statsd-tcp
branch
from
c1fb3aa to
d90bdf9
Compare
bilbof
force-pushed
the
bilbof/statsd-tcp
branch
from
d90bdf9 to
9a7631e
Compare
|
@kevindew thanks very much for your review. Let me know if I have addressed your comments.
Yes, it's a shame we can't use UDP - the metrics we're sending aren't really essential so it doesn't matter if they get lost. It's an AWS limitation driving this more than anything else. We're also planning on migrating away from Statsd / Graphite to Prometheus, since this is a stepping stone for the migration to ECS, it's a bit moot :) For reference these are the changes in the new client gem: reinh/statsd@master...bilbof:bilbof/rename-class |
Super - yeah statsd all feels rather old hat now so I do look forward to the move forward. |
|
Thanks again @kevindew, I'll release a new version of the gem tomorrow. |
This enables apps to specify both the host and protocol to use when sending metrics to Statsd.
This change is required for apps running in ECS, since Statsd won't run on localhost and apps won't send messages to Statsd over UDP.
This should have no effect on apps currently running in EC2. The defaults
"localhost"and:udparen't required, since they duplicate the defaults in the Statsd gem - I've put them in for clarity.This also temporarily drops in a new Statsd client. This gem (published by me) is my fork of our current client but with this PR merged in - I'll switch the gem back once my PR is merged in.