-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't hijack statsd #384
Don't hijack statsd #384
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments + this needs a rebase, but approach looks great!
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
dcc7ddb
to
e9eac54
Compare
7d82372
to
7119eb9
Compare
Rebased on master. This is ready for a final round of 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, the dashboards will need to be updated before we deploy this.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the default behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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::StatsD
simply 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::StatsD
without 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