Don't hijack statsd #384
Don't hijack statsd #384
Conversation
timothysmith0609
force-pushed
the
dont_hijack_statsd
branch
from
9113037
to
140172d
Compare
|
This PR is currently blocked by statsd-instrument #148, which allows for multiple prefixes. |
|
statsd-instrument #148 has been shipped and released as |
remove accidental file, fix line length Extract prefix to const simpler scoping for StatsD.build call Don't need to build StatsD in test_helper include StatsDHelper, not class method policial rebase fix rebase fix rebase fix centralize adding prefix more fix more fix
timothysmith0609
force-pushed
the
dont_hijack_statsd
branch
from
dcc7ddb
to
e9eac54
Compare
timothysmith0609
force-pushed
the
dont_hijack_statsd
branch
from
7d82372
to
7119eb9
Compare
|
Rebased on master. This is ready for a final round of 👀 |
lib/kubernetes-deploy/statsd.rb
Outdated
| def self.duration(start_time) | ||
| (Time.now.utc - start_time).round(1) | ||
| end | ||
|
|
||
| def self.build | ||
| ::StatsD.default_sample_rate = 1.0 | ||
| ::StatsD.prefix = "KubernetesDeploy" | ||
| self.default_sample_rate = 1.0 |
There was a problem hiding this comment.
Isn't this the default behavior?
There was a problem hiding this comment.
Actually this suffers from the same problem as StatsD.prefix in that it's statically referenced in the underlying library so won't have an effect here (`options[:sample_rate] is used to override if ever we need). I'll remove this line as well.
Perhaps it's my unfamiliarity with DataDog, but what do we need to change, exactly? |
|
Aggregations on distribution metrics aren't done automatically, so we need to configure our new metrics in the Datadog UI. The graphs then apparently need to be adjusted as well, because the format isn't quite the same even once the aggregations exist. Danny was working on both, but I'm intending to pick it up from him tomorrow since he's busy with KubeCon. If you'd like to own that instead though, just let me know! I haven't started yet. 😄 |
Problem
Currently, if KubernetesDeploy is used as part of a library, it will hijack the global StatsD configuration and, among other things, will prefix all metrics with
KubernetesDeploy. Ideally, KubernetesDeploy should not mutate global config like this and its StatsD configuration should be scoped more tightly to itself.Closes #336.
Proposed solution
Instead of
KubernetesDeploy::StatsDsimply acting as an interface to the global StatsD configuration, it will instead extend it and become a singleton in its own right. This allows us to specify config parameters forKubernetesDeploy::StatsDwithout overriding the global configuration.Issues
Unfortunately, some methods in the StatsD library make statically scoped calls to the StatsD gem itself. Thankfully this only seems to occur when initializing new metrics. After perusing the statsd-instrument repo, I noticed that we aren't the first to come across this and the proposed solution was pretty much what I had in mind. The PR is here. Testing locally with the changes to the gem yields successful test passes Edit: this has been shipped